| Message ID | 20181105192955.26305-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/mmu_notifier: rename mmu_notifier_synchronize() to <...>_barrier() | expand |
On Mon, 5 Nov 2018 11:29:55 -0800 Sean Christopherson <sean.j.christopherson@intel.com> wrote: > ...and update its comment to explicitly reference its association with > mmu_notifier_call_srcu(). > > Contrary to its name, mmu_notifier_synchronize() does not synchronize > the notifier's SRCU instance, but rather waits for RCU callbacks to > finished, i.e. it invokes rcu_barrier(). The RCU documentation is > quite clear on this matter, explicitly calling out that rcu_barrier() > does not imply synchronize_rcu(). The misnomer could lean an unwary > developer to incorrectly assume that mmu_notifier_synchronize() can > be used in conjunction with mmu_notifier_unregister_no_release() to > implement a variation of mmu_notifier_unregister() that synchronizes > SRCU without invoking ->release. A Documentation-allergic and hasty > developer could be further confused by the fact that rcu_barrier() is > indeed a pass-through to synchronize_rcu()... in tiny SRCU. Fair enough. > --- a/mm/mmu_notifier.c > +++ b/mm/mmu_notifier.c > @@ -35,12 +35,12 @@ void mmu_notifier_call_srcu(struct rcu_head *rcu, > } > EXPORT_SYMBOL_GPL(mmu_notifier_call_srcu); > > -void mmu_notifier_synchronize(void) > +void mmu_notifier_barrier(void) > { > - /* Wait for any running method to finish. */ > + /* Wait for any running RCU callbacks (see above) to finish. */ > srcu_barrier(&srcu); > } > -EXPORT_SYMBOL_GPL(mmu_notifier_synchronize); > +EXPORT_SYMBOL_GPL(mmu_notifier_barrier); > > /* > * This function can't run concurrently against mmu_notifier_register But as it has no callers, why retain it?
On Mon, Nov 05, 2018 at 12:18:33PM -0800, Andrew Morton wrote: > On Mon, 5 Nov 2018 11:29:55 -0800 Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > ...and update its comment to explicitly reference its association with > > mmu_notifier_call_srcu(). > > > > Contrary to its name, mmu_notifier_synchronize() does not synchronize > > the notifier's SRCU instance, but rather waits for RCU callbacks to > > finished, i.e. it invokes rcu_barrier(). The RCU documentation is > > quite clear on this matter, explicitly calling out that rcu_barrier() > > does not imply synchronize_rcu(). The misnomer could lean an unwary > > developer to incorrectly assume that mmu_notifier_synchronize() can > > be used in conjunction with mmu_notifier_unregister_no_release() to > > implement a variation of mmu_notifier_unregister() that synchronizes > > SRCU without invoking ->release. A Documentation-allergic and hasty > > developer could be further confused by the fact that rcu_barrier() is > > indeed a pass-through to synchronize_rcu()... in tiny SRCU. > > Fair enough. > > > --- a/mm/mmu_notifier.c > > +++ b/mm/mmu_notifier.c > > @@ -35,12 +35,12 @@ void mmu_notifier_call_srcu(struct rcu_head *rcu, > > } > > EXPORT_SYMBOL_GPL(mmu_notifier_call_srcu); > > > > -void mmu_notifier_synchronize(void) > > +void mmu_notifier_barrier(void) > > { > > - /* Wait for any running method to finish. */ > > + /* Wait for any running RCU callbacks (see above) to finish. */ > > srcu_barrier(&srcu); > > } > > -EXPORT_SYMBOL_GPL(mmu_notifier_synchronize); > > +EXPORT_SYMBOL_GPL(mmu_notifier_barrier); > > > > /* > > * This function can't run concurrently against mmu_notifier_register > > But as it has no callers, why retain it? I was hesitant to remove it altogether since it was explicitly added to complement mmu_notifier_call_srcu()[1] even though the initial user of mmu_notifier_call_srcu() didn't use mmu_notifier_synchronize()[2]. I assume there was a good reason for adding the barrier function, but maybe that's a bad assumption. [1] b972216e27d1 ("mmu_notifier: add call_srcu and sync function for listener to delay call and sync") [2] https://lore.kernel.org/patchwork/patch/515318/
On Mon, Nov 05, 2018 at 12:18:33PM -0800, Andrew Morton wrote: > > +++ b/mm/mmu_notifier.c > > But as it has no callers, why retain it? ... and this patch missed the declaration of mmu_notifier_synchronize in include/linux/mmu_notifier.h (whether we delete it or rename it, that mention of it needs to be fixed)
On Mon 05-11-18 12:18:33, Andrew Morton wrote: > On Mon, 5 Nov 2018 11:29:55 -0800 Sean Christopherson <sean.j.christopherson@intel.com> wrote: [...] > > -EXPORT_SYMBOL_GPL(mmu_notifier_synchronize); > > +EXPORT_SYMBOL_GPL(mmu_notifier_barrier); > > > > /* > > * This function can't run concurrently against mmu_notifier_register > > But as it has no callers, why retain it? Exported symbols are not freed and if this is not used by any in-kernel code then I would just remove it.
On Mon, 2018-11-05 at 13:14 -0800, Matthew Wilcox wrote: > On Mon, Nov 05, 2018 at 12:18:33PM -0800, Andrew Morton wrote: > > > > > > > > +++ b/mm/mmu_notifier.c > > But as it has no callers, why retain it? > ... and this patch missed the declaration of mmu_notifier_synchronize > in include/linux/mmu_notifier.h (whether we delete it or rename it, > that mention of it needs to be fixed) Doh. I'll remove the function and send v2.
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c index 5119ff846769..46ebea6483bf 100644 --- a/mm/mmu_notifier.c +++ b/mm/mmu_notifier.c @@ -35,12 +35,12 @@ void mmu_notifier_call_srcu(struct rcu_head *rcu, } EXPORT_SYMBOL_GPL(mmu_notifier_call_srcu); -void mmu_notifier_synchronize(void) +void mmu_notifier_barrier(void) { - /* Wait for any running method to finish. */ + /* Wait for any running RCU callbacks (see above) to finish. */ srcu_barrier(&srcu); } -EXPORT_SYMBOL_GPL(mmu_notifier_synchronize); +EXPORT_SYMBOL_GPL(mmu_notifier_barrier); /* * This function can't run concurrently against mmu_notifier_register
...and update its comment to explicitly reference its association with mmu_notifier_call_srcu(). Contrary to its name, mmu_notifier_synchronize() does not synchronize the notifier's SRCU instance, but rather waits for RCU callbacks to finished, i.e. it invokes rcu_barrier(). The RCU documentation is quite clear on this matter, explicitly calling out that rcu_barrier() does not imply synchronize_rcu(). The misnomer could lean an unwary developer to incorrectly assume that mmu_notifier_synchronize() can be used in conjunction with mmu_notifier_unregister_no_release() to implement a variation of mmu_notifier_unregister() that synchronizes SRCU without invoking ->release. A Documentation-allergic and hasty developer could be further confused by the fact that rcu_barrier() is indeed a pass-through to synchronize_rcu()... in tiny SRCU. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- mm/mmu_notifier.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)