Question: cpuset_update_task_memory_state() and mmap_sem ???

Question: cpuset_update_task_memory_state() and mmap_sem ???

[Lee Schermerhorn]

In the comment block for the subject function in cpuset.c, it notes that
"This routine also might acquire callback_mutex and
current->mm->mmap_sem."

Is this is a stale comment?  I can't find any path from this function to
a down_{read|write}() on the caller's mmap_sem [in 23-rc2-mm2].  I
suspect that one would have noticed, as
cpuset_update_task_memory_state() is called from
alloc_page_vma() which, according to its comment block, can only be
called with the mmap_sem held [for read, at least].

Lee

--
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>

Re: Question: cpuset_update_task_memory_state() and mmap_sem ???

[Paul Jackson]

Lee wrote:
> In the comment block for the subject function in cpuset.c, it notes that
> "This routine also might acquire callback_mutex and
> current->mm->mmap_sem."
> 
> Is this is a stale comment?  I can't find any path from this function to
> a down_{read|write}() on the caller's mmap_sem [in 23-rc2-mm2].  I
> suspect that one would have noticed, as
> cpuset_update_task_memory_state() is called from
> alloc_page_vma() which, according to its comment block, can only be
> called with the mmap_sem held [for read, at least].

Hmmm ... you may be right  But I'm not sure.

Obviously, the callback_mutex mention in the comment is correct,
but the current->mm->mmap_sem mention seems bogus.

The routine mpol_rebind_task() is called from the last line of
cpuset_update_task_memory_state().  Whatever mmap_sem is taken
would be within that call.  But I can't find any taking of
mmap_sem within or below mpol_rebind_task(), and all the code
paths in mm/mempolicy.c that do take mmap_sem locks seem to be
on unrelated code paths.

I tried looking in a few old versions of kernel/cpuset.c and
mm/mempolicy.c to see if the mention of current->mm->mmap_sem
made more sense in some old version, but didn't see any version
of code that justified that comment.

... would you like to propose a patch, nuking the phrase:

   and current->mm->mmap_sem

from that comment?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

--
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>

Re: Question: cpuset_update_task_memory_state() and mmap_sem ???

[Lee Schermerhorn]

On Wed, 2007-08-15 at 23:06 -0700, Paul Jackson wrote:
> Lee wrote:
> > In the comment block for the subject function in cpuset.c, it notes that
> > "This routine also might acquire callback_mutex and
> > current->mm->mmap_sem."
> > 
> > Is this is a stale comment?  I can't find any path from this function to
> > a down_{read|write}() on the caller's mmap_sem [in 23-rc2-mm2].  I
> > suspect that one would have noticed, as
> > cpuset_update_task_memory_state() is called from
> > alloc_page_vma() which, according to its comment block, can only be
> > called with the mmap_sem held [for read, at least].
> 
> Hmmm ... you may be right  But I'm not sure.
> 
> Obviously, the callback_mutex mention in the comment is correct,
> but the current->mm->mmap_sem mention seems bogus.
> 
> The routine mpol_rebind_task() is called from the last line of
> cpuset_update_task_memory_state().  Whatever mmap_sem is taken
> would be within that call.  But I can't find any taking of
> mmap_sem within or below mpol_rebind_task(), and all the code
> paths in mm/mempolicy.c that do take mmap_sem locks seem to be
> on unrelated code paths.
> 
> I tried looking in a few old versions of kernel/cpuset.c and
> mm/mempolicy.c to see if the mention of current->mm->mmap_sem
> made more sense in some old version, but didn't see any version
> of code that justified that comment.
> 
> ... would you like to propose a patch, nuking the phrase:
> 
>    and current->mm->mmap_sem
> 
> from that comment?

If that's the correct thing to do, sure.  Just wanted to check with you
whether I was missing something.   

Lee
> 

--
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>

Re: Question: cpuset_update_task_memory_state() and mmap_sem ???

[Paul Jackson]

Lee, responding to pj:
> > ... would you like to propose a patch, nuking the phrase:
> > 
> >    and current->mm->mmap_sem
> > 
> > from that comment?
> 
> If that's the correct thing to do, sure.  Just wanted to check with you
> whether I was missing something.   

As best as I can tell, it's the correct thing to do, yes.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

--
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>