Re: [PATCH v2] mmap_lock: Change trace and locking order

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 6 Sep 2021 17:00:18 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:

> On 9/3/21 04:21, Liam Howlett wrote:
> > Print to the trace log before releasing the lock to avoid racing with
> > other trace log printers of the same lock type.  
> 
> That description could use more details maybe?

Agreed, perhaps add something like this:

Due to the tracing of taking the mmap_lock happened outside the lock
itself, the trace can become confusing, making it look like more than one
task has the same lock:

     task-749     [006] ....     14437980: mmap_lock_acquire_returned: mm=00000000c94d28b8 memcg_path= write=true success=true
     task-750     [007] ....     14437981: mmap_lock_acquire_returned: mm=00000000c94d28b8 memcg_path= write=true success=true
     task-749     [006] ....     14437983: mmap_lock_released: mm=00000000c94d28b8 memcg_path= write=true

When in actuality the following occurred:

   task-749 [006] - take mmap_lock
   task-749 [006] - trace taking of mmap_lock
   task-749 [006] - release mmap_lock

   task-750 [007] - take mmap_lock
   task-750 [007] - trace taking of mmap_lock

   task-749 [006] - trace releasing of mmap_lock

Although the mmap_lock was taken and released then taken again by another
process, the lack of protection around the tracing of the activity caused
it to show the events out of order. If the tracing of the taking and
releasing of the mmap_lock is done inside the lock, it will protect this
from happening.

Andrew, I see you took this into your tree. Not sure if you sent it to
Linus yet. Perhaps add the above to the patch if you have not yet sent it
(with Liam's permission of course).

Seeing that the patch has this as a link in the commit, at the very least,
the above explanation will be archived.

-- Steve



> 
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>