On Tue, 12 Feb 2008, Lee Schermerhorn wrote:

quoted text
> What is the benefit of pulling the flags and mode apart at the user
> interface, passing them as separate args to mpol_new(), do_* and
> mpol_shared_policy_init() and then stitching them back together in
> mpol_new()?   Modes passed in via [sys_]{set_mempolicy|mbind}() and
> those stored in the the shmem_sb_info can already have the flags there,
> so this seems like extra work.
> 
> I think this patch and the previous one [1/4] would be simplified if the
> formal parameters were left as an int [mabye, unsigned]  and the flags
> were masked of when necessary in mpol_new() and elsewhere.
> 

Christoph made a similiar suggestion.

quoted text
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -203,18 +203,20 @@ static struct zonelist *bind_zonelist(nodemask_t *nodes)
> >  }
> >  
> >  /* Create a new policy */
> > -static struct mempolicy *mpol_new(enum mempolicy_mode mode, nodemask_t *nodes)
> > +static struct mempolicy *mpol_new(enum mempolicy_mode mode,
> > +				  unsigned short flags, nodemask_t *nodes)
> >  {
> >  	struct mempolicy *policy;
> >  
> > -	pr_debug("setting mode %d nodes[0] %lx\n",
> > -		 mode, nodes ? nodes_addr(*nodes)[0] : -1);
> > +	pr_debug("setting mode %d flags %d nodes[0] %lx\n",
> > +		 mode, flags, nodes ? nodes_addr(*nodes)[0] : -1);
> >  
> >  	if (mode == MPOL_DEFAULT)
> >  		return NULL;
> >  	policy = kmem_cache_alloc(policy_cache, GFP_KERNEL);
> >  	if (!policy)
> >  		return ERR_PTR(-ENOMEM);
> > +	flags &= MPOL_MODE_FLAGS;
> >  	atomic_set(&policy->refcnt, 1);
> >  	switch (mode) {
> >  	case MPOL_INTERLEAVE:
> > @@ -240,7 +242,7 @@ static struct mempolicy *mpol_new(enum mempolicy_mode mode, nodemask_t *nodes)
> >  	default:
> >  		BUG();
> >  	}
> > -	policy->policy = mode;
> > +	policy->policy = mode | flags;
> >  	policy->cpuset_mems_allowed = cpuset_mems_allowed(current);
> >  	return policy;
> >  }
> > @@ -483,19 +485,20 @@ static void mpol_set_task_struct_flag(void)
> >  }
> >  
> >  /* Set the process memory policy */
> > -static long do_set_mempolicy(enum mempolicy_mode mode, nodemask_t *nodes)
> > +static long do_set_mempolicy(enum mempolicy_mode mode, unsigned short flags,
> > +			     nodemask_t *nodes)
> >  {
> >  	struct mempolicy *new;
> >  
> >  	if (mpol_check_policy(mode, nodes))
> >  		return -EINVAL;
> > -	new = mpol_new(mode, nodes);
> > +	new = mpol_new(mode, flags, nodes);
> >  	if (IS_ERR(new))
> >  		return PTR_ERR(new);
> >  	mpol_free(current->mempolicy);
> >  	current->mempolicy = new;
> >  	mpol_set_task_struct_flag();
> > -	if (new && new->policy == MPOL_INTERLEAVE)
> > +	if (new && mpol_mode(new->policy) == MPOL_INTERLEAVE)
> >  		current->il_next = first_node(new->v.nodes);
> >  	return 0;
> >  }
> > @@ -506,7 +509,7 @@ static void get_zonemask(struct mempolicy *p, nodemask_t *nodes)
> >  	int i;
> >  
> >  	nodes_clear(*nodes);
> > -	switch (p->policy) {
> > +	switch (mpol_mode(p->policy)) {
> >  	case MPOL_BIND:
> >  		for (i = 0; p->v.zonelist->zones[i]; i++)
> >  			node_set(zone_to_nid(p->v.zonelist->zones[i]),
> > @@ -588,7 +591,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
> >  				goto out;
> >  			*policy = err;
> >  		} else if (pol == current->mempolicy &&
> > -				pol->policy == MPOL_INTERLEAVE) {
> > +				mpol_mode(pol->policy) == MPOL_INTERLEAVE) {
> >  			*policy = current->il_next;
> >  		} else {
> >  			err = -EINVAL;
> > @@ -785,8 +788,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
> >  #endif
> >  
> >  static long do_mbind(unsigned long start, unsigned long len,
> > -		     enum mempolicy_mode mode, nodemask_t *nmask,
> > -		     unsigned long flags)
> > +		     enum mempolicy_mode mode, unsigned short mode_flags,
> > +		     nodemask_t *nmask, unsigned long flags)
> >  {
> >  	struct vm_area_struct *vma;
> >  	struct mm_struct *mm = current->mm;
> > @@ -818,7 +821,7 @@ static long do_mbind(unsigned long start, unsigned long len,
> >  	if (mpol_check_policy(mode, nmask))
> >  		return -EINVAL;
> >  
> > -	new = mpol_new(mode, nmask);
> > +	new = mpol_new(mode, mode_flags, nmask);
> >  	if (IS_ERR(new))
> >  		return PTR_ERR(new);
> >  
> > @@ -829,8 +832,9 @@ static long do_mbind(unsigned long start, unsigned long len,
> >  	if (!new)
> >  		flags |= MPOL_MF_DISCONTIG_OK;
> >  
> > -	pr_debug("mbind %lx-%lx mode:%d nodes:%lx\n",start,start+len,
> > -		 mode, nmask ? nodes_addr(*nmask)[0] : -1);
> > +	pr_debug("mbind %lx-%lx mode:%d flags:%d nodes:%lx\n",
> > +		 start, start + len, mode, mode_flags,
> > +		 nmask ? nodes_addr(*nmask)[0] : -1);
> >  
> >  	down_write(&mm->mmap_sem);
> >  	vma = check_range(mm, start, end, nmask,
> > @@ -929,13 +933,16 @@ asmlinkage long sys_mbind(unsigned long start, unsigned long len,
> >  {
> >  	nodemask_t nodes;
> >  	int err;
> > +	unsigned short mode_flags;
> >  
> > +	mode_flags = mpol_flags(mode);
> > 
> > 
> I suggest restricting the flags to the defined flags here.  This gives
> us the ability to defined new flags w/o breaking [changing behavior of]
> applications that accidently pass undefined flags.  An error return
> forced the user to fix the application [assuming they check error
> returns].
> 

That looks appropriate.  It will also help in the future if certain flags 
are only defined for set_mempolicy() or only defined for mbind() to either 
mask them out in their respective functions or return -EINVAL.

Thanks for the comments.

		David
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/