Re: [PATCH v5 2/3] mm/mempolicy: add set_mempolicy_home_node syscall

[Michal Hocko <mhocko@suse.com>]

On Mon 29-11-21 16:16:05, Aneesh Kumar K.V wrote:
> Michal Hocko <mhocko@suse.com> writes:
> 
> > On Tue 16-11-21 12:12:37, Aneesh Kumar K.V wrote:
[...]
> >> +SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, len,
> >> +		unsigned long, home_node, unsigned long, flags)
> >> +{
> >> +	struct mm_struct *mm = current->mm;
> >> +	struct vm_area_struct *vma;
> >> +	struct mempolicy *new;
> >> +	unsigned long vmstart;
> >> +	unsigned long vmend;
> >> +	unsigned long end;
> >> +	int err = -ENOENT;
> >> +
> >> +	if (start & ~PAGE_MASK)
> >> +		return -EINVAL;
> >> +	/*
> >> +	 * flags is used for future extension if any.
> >> +	 */
> >> +	if (flags != 0)
> >> +		return -EINVAL;
> >> +
> >> +	if (!node_online(home_node))
> >> +		return -EINVAL;
> >
> > You really want to check the home_node before dereferencing the mask.
> >
> 
> Any reason why we want to check for home node first?

Because the given node is an index to node_states[N_ONLINE] bitmap. I do
not think we do range checking there.

> >> +	len = (len + PAGE_SIZE - 1) & PAGE_MASK;
> >> +	end = start + len;
> >> +
> >> +	if (end < start)
> >> +		return -EINVAL;
> >> +	if (end == start)
> >> +		return 0;
> >> +	mmap_write_lock(mm);
> >> +	vma = find_vma(mm, start);
> >> +	for (; vma && vma->vm_start < end;  vma = vma->vm_next) {
> >> +
> >> +		vmstart = max(start, vma->vm_start);
> >> +		vmend   = min(end, vma->vm_end);
> >> +		new = mpol_dup(vma_policy(vma));
> >> +		if (IS_ERR(new)) {
> >> +			err = PTR_ERR(new);
> >> +			break;
> >> +		}
> >> +		/*
> >> +		 * Only update home node if there is an existing vma policy
> >> +		 */
> >> +		if (!new)
> >> +			continue;
> >
> > Your changelog only mentions MPOL_BIND and MPOL_PREFERRED_MANY as
> > supported but you seem to be applying the home node to all existing
> > policieso
> 
> 
> The restriction is done in policy_node. 
> 
> @@ -1801,6 +1856,11 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy, int nd)
> 		WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
> 	}
> 
> +	if ((policy->mode == MPOL_BIND ||
> +	     policy->mode == MPOL_PREFERRED_MANY) &&
> +	    policy->home_node != NUMA_NO_NODE)
> +		return policy->home_node;
> +
> 	return nd;
>  }

But you do allow to set the home node also for other policies and that
means that a default MPOL_INTERLEAVE would be different from the one
with home_node set up even though they behave exactly the same.
-- 
Michal Hocko
SUSE Labs