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

[Feng Tang]

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