[PATCH 1/2] mm/vmscan: add sync_shrinkers function v2

[Daniel Vetter]

On Thu, Aug 26, 2021 at 04:58:06PM +0200, Christian König wrote:
> Am 26.08.21 um 15:28 schrieb Daniel Vetter:
> > On Thu, Aug 26, 2021 at 03:27:30PM +0200, Daniel Vetter wrote:
> > > On Fri, Aug 20, 2021 at 02:05:27PM +0200, Christian König wrote:
> > > > From: Christian König <ckoenig.leichtzumerken at gmail.com>
> > > > 
> > > > While unplugging a device the TTM shrinker implementation
> > > > needs a barrier to make sure that all concurrent shrink
> > > > operations are done and no other CPU is referring to a
> > > > device specific pool any more.
> > > > 
> > > > Taking and releasing the shrinker semaphore on the write
> > > > side after unmapping and freeing all pages from the device
> > > > pool should make sure that no shrinker is running in
> > > > paralell.
> > > > 
> > > > This allows us to avoid the contented mutex in the TTM pool
> > > > implementation for every alloc/free operation.
> > > > 
> > > > v2: rework the commit message to make clear why we need this
> > > > 
> > > > Signed-off-by: Christian König <christian.koenig at amd.com>
> > > > Acked-by: Huang Rui <ray.huang at amd.com>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > ---
> > > >   include/linux/shrinker.h |  1 +
> > > >   mm/vmscan.c              | 10 ++++++++++
> > > >   2 files changed, 11 insertions(+)
> > > > 
> > > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > > > index 9814fff58a69..1de17f53cdbc 100644
> > > > --- a/include/linux/shrinker.h
> > > > +++ b/include/linux/shrinker.h
> > > > @@ -93,4 +93,5 @@ extern void register_shrinker_prepared(struct shrinker *shrinker);
> > > >   extern int register_shrinker(struct shrinker *shrinker);
> > > >   extern void unregister_shrinker(struct shrinker *shrinker);
> > > >   extern void free_prealloced_shrinker(struct shrinker *shrinker);
> > > > +extern void sync_shrinkers(void);
> > > >   #endif
> > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > index 4620df62f0ff..fde1aabcfa7f 100644
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -638,6 +638,16 @@ void unregister_shrinker(struct shrinker *shrinker)
> > > >   }
> > > >   EXPORT_SYMBOL(unregister_shrinker);
> > > > +/**
> > > > + * sync_shrinker - Wait for all running shrinkers to complete.
> > > I think it would be good to add a bit more text here maybe:
> > > 
> > > "This is equivalent to calling unregister_shrink() and
> > > register_shrinker(), but atomically and with less overhead. This is useful
> > > to guarantee that all shrinker invocations have seen an update, before
> > > freeing memory, similar to rcu."
> > > 
> > > Also a bit a bikeshed, but if we look at the equivalent in irq land it's
> > > called synchronize_irq() and synchronize_hardirq(). I think it'd be good
> > > to bikeshed that for more conceptual consistency.
> > Oh also synchronize_*rcu* also spells them all out, so even more reasons
> > to do the same.
> 
> I will just go with the explanation above.
> 
> The synchronize_rcu() explanation is so extensive that most people will
> probably stop reading after the first paragraph.

Ack, my comment was only about the function name (spelled out instead of
abbreviated), not about pulling the entire kerneldoc in from these.
-Daniel

> 
> Thanks,
> Christian.
> 
> > -Daniel
> > 
> > > > + */
> > > > +void sync_shrinkers(void)
> > > > +{
> > > > +	down_write(&shrinker_rwsem);
> > > > +	up_write(&shrinker_rwsem);
> > > > +}
> > > > +EXPORT_SYMBOL(sync_shrinkers);
> > > > +
> > > >   #define SHRINK_BATCH 128
> > > >   static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> > > > -- 
> > > > 2.25.1
> > > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch