Re: [PATCH/RFC 1/5] Mem Policy: fix reference counting

[mel@skynet.ie (Mel Gorman)]

On (11/09/07 14:12), Lee Schermerhorn didst pronounce:
> On Tue, 2007-09-11 at 19:48 +0100, Mel Gorman wrote:
> > You know this stuff better than I do. Take suggestions here with a large
> > grain of salt.
> 
> Your comments are on the mark.  See responses below.
> 

Great.

> > 
> > On Thu, 2007-08-30 at 14:51 -0400, Lee Schermerhorn wrote:
> <patch description snipped>
> > > Index: Linux/mm/mempolicy.c
> > > ===================================================================
> > > --- Linux.orig/mm/mempolicy.c	2007-08-29 10:05:19.000000000 -0400
> > > +++ Linux/mm/mempolicy.c	2007-08-29 13:31:42.000000000 -0400
> > > @@ -1083,21 +1083,37 @@ asmlinkage long compat_sys_mbind(compat_
> > >  
> > >  #endif
> > >  
> > > -/* Return effective policy for a VMA */
> > > +/*
> > > + * get_vma_policy(@task, @vma, @addr)
> > > + * @task - task for fallback if vma policy == default
> > > + * @vma   - virtual memory area whose policy is sought
> > > + * @addr  - address in @vma for shared policy lookup
> > > + *
> > > + * Returns effective policy for a VMA at specified address.
> > > + * Falls back to @task or system default policy, as necessary.
> > > + * Returned policy has extra reference count if shared, vma,
> > > + * or some other task's policy [show_numa_maps() can pass
> > > + * @task != current].  It is the caller's responsibility to
> > > + * free the reference in these cases.
> > > + */
> > >  static struct mempolicy * get_vma_policy(struct task_struct *task,
> > >  		struct vm_area_struct *vma, unsigned long addr)
> > >  {
> > >  	struct mempolicy *pol = task->mempolicy;
> > > +	int shared_pol = 0;
> > >  
> > >  	if (vma) {
> > > -		if (vma->vm_ops && vma->vm_ops->get_policy)
> > > +		if (vma->vm_ops && vma->vm_ops->get_policy) {
> > >  			pol = vma->vm_ops->get_policy(vma, addr);
> > > -		else if (vma->vm_policy &&
> > > +			shared_pol = 1;	/* if pol non-NULL, that is */
> > 
> > What do you mean here by "pol non-NULL, that is". Where do you check
> > that vm_ops->get_policy() returned a non-NULL value?
> > 
> > Should the comment be 
> > 
> > /* Policy if set is shared, check later */
> > 
> > and rename the variable to check_shared_pol?
> 
> You interpret my cryptic comment correctly.  However, your suggested fix
> doesn't quite capture my way of looking at it.  Would it work for you if
> I change it to:  /* if pol non-NULL, add ref below */  ???  That fits in
> 80 columns ;-)!
> 
> > 
> > > +		} else if (vma->vm_policy &&
> > >  				vma->vm_policy->policy != MPOL_DEFAULT)
> > >  			pol = vma->vm_policy;
> > >  	}
> > >  	if (!pol)
> > >  		pol = &default_policy;
> > > +	else if (!shared_pol && pol != current->mempolicy)
> > > +		mpol_get(pol);	/* vma or other task's policy */
> > >  	return pol;
> > >  }
> > >  
> > > @@ -1213,19 +1229,45 @@ static inline unsigned interleave_nid(st
> > >  }
> > >  
> > >  #ifdef CONFIG_HUGETLBFS
> > > -/* Return a zonelist suitable for a huge page allocation. */
> > > +/*
> > > + * huge_zonelist(@vma, @addr, @gfp_flags, @mpol)
> > > + * @vma = virtual memory area whose policy is sought
> > > + * @addr = address in @vma for shared policy lookup and interleave policy
> > > + * @gfp_flags = for requested zone
> > > + * @mpol = pointer to mempolicy pointer for reference counted 'BIND policy
> > > + *
> > > + * Returns a zonelist suitable for a huge page allocation.
> > > + * If the effective policy is 'BIND, returns pointer to policy's zonelist.
> > 
> > This comment here becomes redundant if applied on top of one-zonelist as
> > you suggest you will be doing later. The zonelist returned for MPOL_BIND
> > is the nodes zonelist but it is filtered based on a nodemask.
> 
> Agreed.  When I get around to rebasing atop your patches [under the
> assumption they'll hit the mm tree before these] I'll fix this up.  For
> now, I've added myself a 'TODO' comment.
> 

I'm hoping they will hit -mm soon. They have dragged on a long time and the
last change was more cosmetic than anything else. Their last real test is
from you really as you catch policy bugs way better than me.

> Note, however, that unless we take a copy of the policy's nodemask,
> we'll still need to hold the reference over the allocation, I think.
> Haven't looked that closely, yet.
> 

Whatever you have to do for MPOL_PREFERRED is likely the same as what
you'll have to do for MPOL_BIND.

> > 
> > > + * If it is also a policy for which get_vma_policy() returns an extra
> > > + * reference, we must hold that reference until after allocation.
> > > + * In that case, return policy via @mpol so hugetlb allocation can drop
> > > + * the reference.  For non-'BIND referenced policies, we can/do drop the
> > > + * reference here, so the caller doesn't need to know about the special case
> > > + * for default and current task policy.
> > > + */
> > >  struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
> > > -							gfp_t gfp_flags)
> > > +				gfp_t gfp_flags, struct mempolicy **mpol)
> > >  {
> > >  	struct mempolicy *pol = get_vma_policy(current, vma, addr);
> > > +	struct zonelist *zl;
> > >  
> > > +	*mpol = NULL;		/* probably no unref needed */
> > >  	if (pol->policy == MPOL_INTERLEAVE) {
> > >  		unsigned nid;
> > >  
> > >  		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
> > > +		__mpol_free(pol);
> > 
> > So, __mpol_free() here acts as a put on the get_vma_policy() right?
> > Either that needs commenting or __mpol_free() needs to be renamed to
> > __mpol_put() assuming that when the count reaches 0, it really gets
> > free.
> 
> Yes, the '__' version of mpol_free() takes a non-NULL policy pointer and
> decrements the reference.  [w/o the '__', a NULL policy pointer is a
> no-op.]  If the resulting count is zero, the policy structure, and any
> attached zonelist [or nodemask, if we make those remote, as discussed in
> Cambridge] is freed.   The 'free notation is Andi's original naming.
> For now, rather than change that throughout the code, I'll comment this
> instance.
> 

Or have a standalone patch that just does the renaming for clarity. The
__func convention in almost every other case refers to a lockless
version of func() or an internal helper of some sort. __func being a
reference count helper for func() is confusing to me and while it's not
your fault, you might as well fix it up while you're here.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>