Messages in this thread | | | Subject | Re: [PATCH] mm/mmu_notifiers: use helper function mmu_notifier_synchronize() | From | Miaohe Lin <> | Date | Thu, 17 Feb 2022 21:44:13 +0800 |
| |
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. > > . >
| |