Re: [PATCH v1 3/4] mm/mempolicy: don't handle MPOL_LOCAL like a fake MPOL_PREFERRED policy

[Michal Hocko <mhocko@suse.com>]

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@intel.com>
> > 
> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> 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