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

[Michal Hocko <mhocko@suse.com>]

On Tue 01-06-21 23:14:51, 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 4 cases that MPOL_LOCAL are used:
> 1. user specifies 'local' policy
> 2. user specifies 'prefer' policy, but with empty nodemask
> 3. system 'default' policy is used
> 4. '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 perform as 'local' policy
> 
> So make 'local' a real policy instead of a fake 'prefer' one, and kill
> MPOL_F_LOCAL bit, which can greatly reduce the confusion for code reading.
> 
> For case 4, the logic of mpol_rebind_preferred() is confusing, as Michal
> Hocko pointed out:
> 
> : I do believe that rebinding preferred policy is just bogus and it should
> : be dropped altogether on the ground that a preference is a mere hint from
> : userspace where to start the allocation.  Unless I am missing something
> : cpusets will be always authoritative for the final placement.  The
> : preferred node just acts as a starting point and it should be really
> : preserved when cpusets changes.  Otherwise we have a very subtle behavior
> : corner cases.
> 
> So dump all the tricky transformation between 'prefer' and 'local',
> and just record the new nodemask of rebinding.
> 
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Feng Tang <feng.tang@intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

But this having another pair of eyes would be definitely helpful.
Still one nit though

> @@ -234,30 +229,27 @@ static int mpol_set_nodemask(struct mempolicy *pol,
>  	/* if mode is MPOL_DEFAULT, pol is NULL. This is right. */
>  	if (pol == NULL)
>  		return 0;
> +
> +	if (pol->mode == MPOL_LOCAL)
> +		return 0;
> +

This would benefit from a comment. The one above pol NULL check is just
desperately unhelpful. I would go with the following
	/*
	 * Default (pol==NULL) resp. local memory policies are not a
	 * subject of any remapping. They also do not need any special
	 * constructor.
	 */
	if (!pol || pol->mode == MPOL_LOCAL)
		return 0;

>  	/* Check N_MEMORY */
>  	nodes_and(nsc->mask1,
>  		  cpuset_current_mems_allowed, node_states[N_MEMORY]);
-- 
Michal Hocko
SUSE Labs