| Message ID | 20220217110948.35477-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmu_notifiers: use helper function mmu_notifier_synchronize() | expand |
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.
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. > > . >
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
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 > . >
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);
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(-)