Skip to contentSkip to conversation

Question: cpuset_update_task_memory_state() and mmap_sem ???

DISCUSSION Lee Schermerhorn 2 participants 4 messages Aug 13, 2007 – Aug 16, 2007 Memory management

lore
Share & tools
lore
#0 lore raw

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#1 lore raw
Quoted 10 lines · Lee

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?

Signature · 9 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.925.600.0401

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#2 lore raw
Quoted 29 lines · Paul Jackson
Quoted 10 lines · Lee

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.

Quoted 5 lines

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

Quoted 1 line

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

#3 lore raw

Lee, responding to pj:

Quoted 8 lines
Quoted 5 lines

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

Signature · 9 lines
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.925.600.0401

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