Hi Michal,
Many thanks for the reivews!
On Thu, May 27, 2021 at 10:12:15AM +0200, Michal Hocko wrote:
> On Wed 26-05-21 13:01:41, Feng Tang wrote:
> > MPOL_LOCAL policy has been setup as a real policy, but it is still
> > handled like a faked POL_PREFERRED policy with one internal
> > MPOL_F_LOCAL flag bit set, and there are many places having to
> > judge the real 'prefer' or the 'local' policy, which are quite
> > confusing.
> >
> > In current code, there are four cases that MPOL_LOCAL are used:
> > * user specifies 'local' policy
> > * user specifies 'prefer' policy, but with empty nodemask
> > * system 'default' policy is used
> > * 'prefer' policy + valid 'preferred' node with MPOL_F_STATIC_NODES
> > flag set, and when it is 'rebind' to a nodemask which doesn't
> > contains the 'preferred' node, it will add the MPOL_F_LOCAL bit
> > and performs as 'local' policy. In future if it is 'rebind' again
> > with valid nodemask, the policy will be restored back to 'prefer'
> >
> > So for the first three cases, we make 'local' a real policy
> > instead of a fake 'prefer' one, this will reduce confusion for
> > reading code.
> >
> > And next optional patch will kill the 'MPOL_F_LOCAL' bit.
>
> I do like this approach. An additional policy should be much easier to
> grasp than a special casing. This code is quite tricky so another pair
> of eyes would be definitely good for the review.
>
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
Thanks!
> Just few nits.
>
> > static int migrate_page_add(struct page *page, struct list_head *pagelist,
> > @@ -1965,6 +1965,8 @@ unsigned int mempolicy_slab_node(void)
> > &policy->v.nodes);
> > return z->zone ? zone_to_nid(z->zone) : node;
> > }
> > + case MPOL_LOCAL:
> > + return node;
>
> Any reason you haven't removed MPOL_F_LOCAL in this and following
> functions? It would make it much more easier to review this patch if
> there was no actual use of the flag in the code after this patch.
As in the commit log, there are 4 cases using 'prefer' + MPOL_F_LOCAL
to represent 'local' policy.
I'm confident in this patch which handle the case 1/2/3, while not
sure if the solution (patch 4/4) for case 4 is the right method. So
I separte them into 3/4 and 4/4
Thanks,
Feng
> >
> > default:
> > BUG();
> > @@ -2089,6 +2091,11 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
> > *mask = mempolicy->v.nodes;
> > break;
> >
> > + case MPOL_LOCAL:
> > + nid = numa_node_id();
> > + init_nodemask_of_node(mask, nid);
> > + break;
> > +
> > default:
> > BUG();
> > }
> > @@ -2333,6 +2340,8 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
> > if (a->flags & MPOL_F_LOCAL)
> > return true;
> > return a->v.preferred_node == b->v.preferred_node;
> > + case MPOL_LOCAL:
> > + return true;
> > default:
> > BUG();
> > return false;
> > @@ -2476,6 +2485,10 @@ int mpol_misplaced(struct page *page, struct vm_area_struct *vma, unsigned long
> > polnid = pol->v.preferred_node;
> > break;
> >
> > + case MPOL_LOCAL:
> > + polnid = numa_node_id();
> > + break;
> > +
> > case MPOL_BIND:
> > /* Optimize placement among multiple nodes via NUMA balancing */
> > if (pol->flags & MPOL_F_MORON) {
> --
> Michal Hocko
> SUSE Labs
