* [PATCH v2] mmap_lock: Change trace and locking order
@ 2021-09-03 2:21 Liam Howlett
2021-09-03 10:55 ` Matthew Wilcox
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Liam Howlett @ 2021-09-03 2:21 UTC (permalink / raw)
To: linux-mm, linux-kernel, Andrew Morton
Cc: Steven Rostedt, Michel Lespinasse, Vlastimil Babka, Matthew Wilcox
Print to the trace log before releasing the lock to avoid racing with
other trace log printers of the same lock type.
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/mmap_lock.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 0540f0156f58..b179f1e3541a 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -101,14 +101,14 @@ static inline bool mmap_write_trylock(struct mm_struct *mm)
static inline void mmap_write_unlock(struct mm_struct *mm)
{
- up_write(&mm->mmap_lock);
__mmap_lock_trace_released(mm, true);
+ up_write(&mm->mmap_lock);
}
static inline void mmap_write_downgrade(struct mm_struct *mm)
{
- downgrade_write(&mm->mmap_lock);
__mmap_lock_trace_acquire_returned(mm, false, true);
+ downgrade_write(&mm->mmap_lock);
}
static inline void mmap_read_lock(struct mm_struct *mm)
@@ -140,8 +140,8 @@ static inline bool mmap_read_trylock(struct mm_struct *mm)
static inline void mmap_read_unlock(struct mm_struct *mm)
{
- up_read(&mm->mmap_lock);
__mmap_lock_trace_released(mm, false);
+ up_read(&mm->mmap_lock);
}
static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
@@ -155,8 +155,8 @@ static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
static inline void mmap_read_unlock_non_owner(struct mm_struct *mm)
{
- up_read_non_owner(&mm->mmap_lock);
__mmap_lock_trace_released(mm, false);
+ up_read_non_owner(&mm->mmap_lock);
}
static inline void mmap_assert_locked(struct mm_struct *mm)
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] mmap_lock: Change trace and locking order
2021-09-03 2:21 [PATCH v2] mmap_lock: Change trace and locking order Liam Howlett
@ 2021-09-03 10:55 ` Matthew Wilcox
2021-09-06 15:00 ` Vlastimil Babka
2021-09-06 15:11 ` David Hildenbrand
2 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2021-09-03 10:55 UTC (permalink / raw)
To: Liam Howlett
Cc: linux-mm, linux-kernel, Andrew Morton, Steven Rostedt,
Michel Lespinasse, Vlastimil Babka
On Fri, Sep 03, 2021 at 02:21:05AM +0000, 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.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmap_lock: Change trace and locking order
2021-09-03 2:21 [PATCH v2] mmap_lock: Change trace and locking order Liam Howlett
2021-09-03 10:55 ` Matthew Wilcox
@ 2021-09-06 15:00 ` Vlastimil Babka
2021-09-07 16:53 ` Steven Rostedt
2021-09-06 15:11 ` David Hildenbrand
2 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2021-09-06 15:00 UTC (permalink / raw)
To: Liam Howlett, linux-mm, linux-kernel, Andrew Morton
Cc: Steven Rostedt, Michel Lespinasse, Matthew Wilcox
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?
> 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmap_lock: Change trace and locking order
2021-09-06 15:00 ` Vlastimil Babka
@ 2021-09-07 16:53 ` Steven Rostedt
2021-09-07 17:57 ` Liam Howlett
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2021-09-07 16:53 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Liam Howlett, linux-mm, linux-kernel, Andrew Morton,
Michel Lespinasse, Matthew Wilcox
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] mmap_lock: Change trace and locking order
2021-09-07 16:53 ` Steven Rostedt
@ 2021-09-07 17:57 ` Liam Howlett
0 siblings, 0 replies; 6+ messages in thread
From: Liam Howlett @ 2021-09-07 17:57 UTC (permalink / raw)
To: Steven Rostedt
Cc: Vlastimil Babka, linux-mm, linux-kernel, Andrew Morton,
Michel Lespinasse, Matthew Wilcox
* Steven Rostedt <rostedt@goodmis.org> [210907 12:53]:
> 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).
Yes, I agree this change is useful.
Let me know if you want a respin of the patch instead.
Thanks,
Liam
>
> 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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmap_lock: Change trace and locking order
2021-09-03 2:21 [PATCH v2] mmap_lock: Change trace and locking order Liam Howlett
2021-09-03 10:55 ` Matthew Wilcox
2021-09-06 15:00 ` Vlastimil Babka
@ 2021-09-06 15:11 ` David Hildenbrand
2 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-09-06 15:11 UTC (permalink / raw)
To: Liam Howlett, linux-mm, linux-kernel, Andrew Morton
Cc: Steven Rostedt, Michel Lespinasse, Vlastimil Babka, Matthew Wilcox
On 03.09.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.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> include/linux/mmap_lock.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 0540f0156f58..b179f1e3541a 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -101,14 +101,14 @@ static inline bool mmap_write_trylock(struct mm_struct *mm)
>
> static inline void mmap_write_unlock(struct mm_struct *mm)
> {
> - up_write(&mm->mmap_lock);
> __mmap_lock_trace_released(mm, true);
> + up_write(&mm->mmap_lock);
> }
>
> static inline void mmap_write_downgrade(struct mm_struct *mm)
> {
> - downgrade_write(&mm->mmap_lock);
> __mmap_lock_trace_acquire_returned(mm, false, true);
> + downgrade_write(&mm->mmap_lock);
> }
>
> static inline void mmap_read_lock(struct mm_struct *mm)
> @@ -140,8 +140,8 @@ static inline bool mmap_read_trylock(struct mm_struct *mm)
>
> static inline void mmap_read_unlock(struct mm_struct *mm)
> {
> - up_read(&mm->mmap_lock);
> __mmap_lock_trace_released(mm, false);
> + up_read(&mm->mmap_lock);
> }
>
> static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
> @@ -155,8 +155,8 @@ static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
>
> static inline void mmap_read_unlock_non_owner(struct mm_struct *mm)
> {
> - up_read_non_owner(&mm->mmap_lock);
> __mmap_lock_trace_released(mm, false);
> + up_read_non_owner(&mm->mmap_lock);
> }
>
> static inline void mmap_assert_locked(struct mm_struct *mm)
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-07 17:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 2:21 [PATCH v2] mmap_lock: Change trace and locking order Liam Howlett
2021-09-03 10:55 ` Matthew Wilcox
2021-09-06 15:00 ` Vlastimil Babka
2021-09-07 16:53 ` Steven Rostedt
2021-09-07 17:57 ` Liam Howlett
2021-09-06 15:11 ` David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.