* [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize()
@ 2022-02-17 11:09 Miaohe Lin
2022-02-17 13:32 ` Matthew Wilcox
2022-02-17 15:50 ` Jason Gunthorpe
0 siblings, 2 replies; 5+ messages in thread
From: Miaohe Lin @ 2022-02-17 11:09 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, linmiaohe
Use helper function mmu_notifier_synchronize() to ensure all mmu_notifiers
are freed. Minor readability improvement.
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
mm/mmu_notifier.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 459d195d2ff6..159f70c20236 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -334,15 +334,15 @@ static void mn_hlist_release(struct mmu_notifier_subscriptions *subscriptions,
srcu_read_unlock(&srcu, id);
/*
- * synchronize_srcu here prevents mmu_notifier_release from returning to
- * exit_mmap (which would proceed with freeing all pages in the mm)
- * until the ->release method returns, if it was invoked by
- * mmu_notifier_unregister.
+ * mmu_notifier_synchronize here prevents mmu_notifier_release from
+ * returning to exit_mmap (which would proceed with freeing all pages
+ * in the mm) until the ->release method returns, if it was invoked
+ * by mmu_notifier_unregister.
*
* The notifier_subscriptions can't go away from under us because
* one mm_count is held by exit_mmap.
*/
- synchronize_srcu(&srcu);
+ mmu_notifier_synchronize();
}
void __mmu_notifier_release(struct mm_struct *mm)
@@ -851,7 +851,7 @@ void mmu_notifier_unregister(struct mmu_notifier *subscription,
* Wait for any running method to finish, of course including
* ->release if it was run by mmu_notifier_release instead of us.
*/
- synchronize_srcu(&srcu);
+ mmu_notifier_synchronize();
BUG_ON(atomic_read(&mm->mm_count) <= 0);
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize()
2022-02-17 11:09 [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize() Miaohe Lin
@ 2022-02-17 13:32 ` Matthew Wilcox
2022-02-17 13:44 ` Miaohe Lin
2022-02-17 15:50 ` Jason Gunthorpe
1 sibling, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2022-02-17 13:32 UTC (permalink / raw)
To: Miaohe Lin; +Cc: akpm, linux-mm, linux-kernel
On Thu, Feb 17, 2022 at 07:09:48PM +0800, Miaohe Lin wrote:
> Use helper function mmu_notifier_synchronize() to ensure all mmu_notifiers
> are freed. Minor readability improvement.
Is it though?
> @@ -334,15 +334,15 @@ static void mn_hlist_release(struct mmu_notifier_subscriptions *subscriptions,
> srcu_read_unlock(&srcu, id);
>
> /*
> - * synchronize_srcu here prevents mmu_notifier_release from returning to
> - * exit_mmap (which would proceed with freeing all pages in the mm)
> - * until the ->release method returns, if it was invoked by
> - * mmu_notifier_unregister.
> + * mmu_notifier_synchronize here prevents mmu_notifier_release from
> + * returning to exit_mmap (which would proceed with freeing all pages
> + * in the mm) until the ->release method returns, if it was invoked
> + * by mmu_notifier_unregister.
> *
> * The notifier_subscriptions can't go away from under us because
> * one mm_count is held by exit_mmap.
> */
> - synchronize_srcu(&srcu);
> + mmu_notifier_synchronize();
We just read_unlocked the &srcu. Now I have to jump to the definition
of mmu_notifier_synchronize() to find out that it's now waiting for the
very same srcu. I think this abstraction makes the code harder to read,
not easier.
> }
>
> void __mmu_notifier_release(struct mm_struct *mm)
> @@ -851,7 +851,7 @@ void mmu_notifier_unregister(struct mmu_notifier *subscription,
> * Wait for any running method to finish, of course including
> * ->release if it was run by mmu_notifier_release instead of us.
> */
> - synchronize_srcu(&srcu);
> + mmu_notifier_synchronize();
Same here.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize()
2022-02-17 13:32 ` Matthew Wilcox
@ 2022-02-17 13:44 ` Miaohe Lin
0 siblings, 0 replies; 5+ messages in thread
From: Miaohe Lin @ 2022-02-17 13:44 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: akpm, linux-mm, linux-kernel
On 2022/2/17 21:32, Matthew Wilcox wrote:
> On Thu, Feb 17, 2022 at 07:09:48PM +0800, Miaohe Lin wrote:
>> Use helper function mmu_notifier_synchronize() to ensure all mmu_notifiers
>> are freed. Minor readability improvement.
>
> Is it though?
>
>> @@ -334,15 +334,15 @@ static void mn_hlist_release(struct mmu_notifier_subscriptions *subscriptions,
>> srcu_read_unlock(&srcu, id);
>>
>> /*
>> - * synchronize_srcu here prevents mmu_notifier_release from returning to
>> - * exit_mmap (which would proceed with freeing all pages in the mm)
>> - * until the ->release method returns, if it was invoked by
>> - * mmu_notifier_unregister.
>> + * mmu_notifier_synchronize here prevents mmu_notifier_release from
>> + * returning to exit_mmap (which would proceed with freeing all pages
>> + * in the mm) until the ->release method returns, if it was invoked
>> + * by mmu_notifier_unregister.
>> *
>> * The notifier_subscriptions can't go away from under us because
>> * one mm_count is held by exit_mmap.
>> */
>> - synchronize_srcu(&srcu);
>> + mmu_notifier_synchronize();
>
> We just read_unlocked the &srcu. Now I have to jump to the definition
> of mmu_notifier_synchronize() to find out that it's now waiting for the
> very same srcu. I think this abstraction makes the code harder to read,
> not easier.
>
From this point of view, this helper would disturb the understanding of the code.
Many thanks for pointing this out. Sorry for my mindlessness.
>> }
>>
>> void __mmu_notifier_release(struct mm_struct *mm)
>> @@ -851,7 +851,7 @@ void mmu_notifier_unregister(struct mmu_notifier *subscription,
>> * Wait for any running method to finish, of course including
>> * ->release if it was run by mmu_notifier_release instead of us.
>> */
>> - synchronize_srcu(&srcu);
>> + mmu_notifier_synchronize();
>
> Same here.
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize()
2022-02-17 11:09 [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize() Miaohe Lin
2022-02-17 13:32 ` Matthew Wilcox
@ 2022-02-17 15:50 ` Jason Gunthorpe
2022-02-18 1:49 ` Miaohe Lin
1 sibling, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2022-02-17 15:50 UTC (permalink / raw)
To: Miaohe Lin; +Cc: akpm, linux-mm, linux-kernel
On Thu, Feb 17, 2022 at 07:09:48PM +0800, Miaohe Lin wrote:
> Use helper function mmu_notifier_synchronize() to ensure all mmu_notifiers
> are freed. Minor readability improvement.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> mm/mmu_notifier.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
I'm not keen on this, the internal synchronize_srcu's don't have the
same usage model as described in the comment for
mmu_notifier_synchronize(). Instead they are doing what their comments
say.
Yes, it is the same code, but the purpose is different.
Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize()
2022-02-17 15:50 ` Jason Gunthorpe
@ 2022-02-18 1:49 ` Miaohe Lin
0 siblings, 0 replies; 5+ messages in thread
From: Miaohe Lin @ 2022-02-18 1:49 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: akpm, linux-mm, linux-kernel
On 2022/2/17 23:50, Jason Gunthorpe wrote:
> On Thu, Feb 17, 2022 at 07:09:48PM +0800, Miaohe Lin wrote:
>> Use helper function mmu_notifier_synchronize() to ensure all mmu_notifiers
>> are freed. Minor readability improvement.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>> mm/mmu_notifier.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> I'm not keen on this, the internal synchronize_srcu's don't have the
> same usage model as described in the comment for
> mmu_notifier_synchronize(). Instead they are doing what their comments
> say.
>
> Yes, it is the same code, but the purpose is different.
>
Yep, this is my overlook. Many thanks for reply.
> Jason
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-18 1:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-17 11:09 [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize() Miaohe Lin
2022-02-17 13:32 ` Matthew Wilcox
2022-02-17 13:44 ` Miaohe Lin
2022-02-17 15:50 ` Jason Gunthorpe
2022-02-18 1:49 ` Miaohe Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).