Re: [PATCH v2] mm/mempolicy: fix mpol_rebind_nodemask() for MPOL_F_NUMA_BALANCING

["Huang, Ying" <ying.huang@linux.alibaba.com>]

Hi, Jinjiang,

Sorry, I found the patch description is still confusing for me.

Jinjiang Tu <tujinjiang@huawei.com> writes:

> commit bda420b98505 ("numa balancing: migrate on fault among multiple
> bound nodes") adds new flag MPOL_F_NUMA_BALANCING to enable NUMA balancing
> for MPOL_BIND memory policy.

Is the following description better?  At least, I think we should
emphasize that MPOL_F_NUMA_BALANCING is set while both
MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES are cleared in the mode
parameter.

When an application calls set_mempolicy() with MPOL_F_NUMA_BALANCING set
but both MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES cleared,
mempolicy.w.cpuset_mems_allowed should be set to
cpuset_current_mems_allowed nodemask.  However, due to a bug in its
current implementation, mpol_store_user_nodemask() wrongly returns true,
causing mempolicy->w.user_nodemask to be incorrectly set to the
user-specified nodemask (or an empty nodemask).  Later, when the cpuset
of the application changes, mpol_rebind_nodemask() ends up rebinding
based on the user-specified nodemask rather than the cpuset_mems_allowed
nodemask as intended.

> when the cpuset of tasks changes, the mempolicy of the task is rebound
> by mpol_rebind_nodemask(). The intended rebinding behavior of
> MPOL_F_NUMA_BALANCING was the same as when neither MPOL_F_STATIC_NODES nor
> MPOL_F_RELATIVE_NODES flags are set. However, this commit breaks it.
>
> struct mempolicy has a union member as bellow:
>
>    union {
>        nodemask_t cpuset_mems_allowed; /* relative to these nodes */
>        nodemask_t user_nodemask;       /* nodemask passed by user */
>    } w;
>
> w.cpuset_mems_allowed and w.user_nodemask are both nodemask type and their
> difference is only what type of nodemask is stored. mpol_set_nodemask()
> initializes the union like below:
>
>    static int mpol_set_nodemask(...)
>    {
>         if (mpol_store_user_nodemask(pol))
>                 pol->w.user_nodemask = *nodes;
>         else
>                 pol->w.cpuset_mems_allowed = cpuset_current_mems_allowed;
>    }
>
> mpol_store_user_nodemask() returns true for MPOL_F_NUMA_BALANCING
> incorrectly and the union stores user-passed nodemask. Consequently,
> mpol_rebind_nodemask() ends up rebinding based on the user-passed nodemask
> rather than the cpuset_mems_allowed nodemask as intended.
>
> To fix this, only store the user nodemask if MPOL_F_STATIC_NODES or
> MPOL_F_RELATIVE_NODES is present.
>
> Fixes: bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes")
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>

[snip]

---
Best Regards,
Huang, Ying