Re: [PATCH] drm/ttm: optimize the pool shrinker a bit

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 08, 2021 at 02:44:16PM +0200, Christian König wrote:
> Am 08.04.21 um 13:31 schrieb Daniel Vetter:
> > On Thu, Apr 08, 2021 at 01:17:32PM +0200, Christian König wrote:
> > > Am 08.04.21 um 13:08 schrieb Daniel Vetter:
> > > > On Thu, Apr 01, 2021 at 03:54:13PM +0200, Christian König wrote:
> > > > > [SNIP]
> > > > >    EXPORT_SYMBOL(unregister_shrinker);
> > > > > +/**
> > > > > + * sync_shrinker - Wait for all running shrinkers to complete.
> > > > > + */
> > > > > +void sync_shrinkers(void)
> > > > This one should probably be in its own patch, with a bit more commit
> > > > message about why we need it and all that. I'd assume that just
> > > > unregistering the shrinker should sync everything we needed to sync
> > > > already, and for other sync needs we can do locking within our own
> > > > shrinker?
> > > Correct. Reason why we need the barrier is that we need to destroy the
> > > device (during hotplug) before the shrinker is unregistered (during module
> > > unload).
> > > 
> > > Going to separate that, write something up in the commit message and send it
> > > to the appropriate audience.
> > Hm why do we need that?
> 
> When the shrinker runs in parallel with (for example) a hotplug event and
> unmaps pages from the devices IOMMU I must make sure that you can't destroy
> the device or pool structure at the same time.
> 
> Previously holding the mutex while updating the IOMMU would take care of
> that, but now we need to prevent this otherwise.
> 
> Could be that this is also handled somewhere else, but I'm better save than
> sorry here and grabbing/releasing write side of the shrinker_rwsem is rather
> lightweight.

I forgot that we don't have a per-pool (or at least per-device) shrinker,
but one global one for all ttm device. So yeah with that design a
sync_shrinker is needed.
-Daniel

> 
> > Either way sounds like an orthogonal series for
> > the hotunplug work, not just shrinker optimization.
> 
> It is unrelated to the hotplug work in general.
> 
> Regards,
> Christian.
> 
> > -Daniel
> > 
> > > Thanks,
> > > Christian.
> > > 
> > > > -Daniel
> > > > 
> > > > > +{
> > > > > +	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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel