On Thu 27-05-21 20:06:42, Feng Tang wrote:
> 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
Please don't and handle the above and those below in a single patch.
> 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
--
Michal Hocko
SUSE Labs
