[PATCH 2/4] drm/shmem: Use mutex_trylock in drm_gem_shmem_purge

[Daniel Vetter]

On Wed, Aug 21, 2019 at 11:03:55AM -0500, Rob Herring wrote:
> On Wed, Aug 21, 2019 at 3:23 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> > On Tue, Aug 20, 2019 at 07:35:47AM -0500, Rob Herring wrote:
> > > On Tue, Aug 20, 2019 at 4:05 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> > > >
> > > > On Mon, Aug 19, 2019 at 11:12:02AM -0500, Rob Herring wrote:
> > > > > Lockdep reports a circular locking dependency with pages_lock taken in
> > > > > the shrinker callback. The deadlock can't actually happen with current
> > > > > users at least as a BO will never be purgeable when pages_lock is held.
> > > > > To be safe, let's use mutex_trylock() instead and bail if a BO is locked
> > > > > already.
> 
> [...]
> 
> > > > > -void drm_gem_shmem_purge(struct drm_gem_object *obj)
> > > > > +bool drm_gem_shmem_purge(struct drm_gem_object *obj)
> > > > >  {
> > > > >       struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > > >
> > > > > -     mutex_lock(&shmem->pages_lock);
> > > > > +     if (!mutex_trylock(&shmem->pages_lock))
> > > >
> > > > Did you see my ping about cutting all the locking over to dma_resv?
> > >
> > > Yes, but you didn't reply to Rob C. about it. I guess I'll have to go
> > > figure out how reservation objects work...
> >
> > msm was the last driver that still used struct_mutex. It's a long-term
> > dead-end, and I think with all the effort recently to create helpers for
> > rendering drivers (shmem, vram, ttm refactoring) we should make a solid
> > attempt to get aligned. Or did you mean that Rob Clark had some
> > reply/questions that I didn' respond to because it fell through cracks?
> 
> I'm not using struct_mutex, so I'm confused as to why you keep
> mentioning it. The list of BOs for the shrinker is protected with a
> mutex for the list. That list head, list mutex, and the shrinker
> instance all have to live at the driver level, so they can't be moved
> into shmem as you suggested. Agreed?

struct_mutex is just the historical baggage.

Wrt shrinker/lru, why not? We've talked about maybe moving that to make it
easier to share ...

> Then there is the pages_lock within the shmem BO. I assume that is
> what you are suggesting converting to dma_resv? I'm not really sure
> what that would look like. You're going to have to spell it out for
> me. In my brief look at it, it seems like added complexity and it's
> not clear to me what that buys. Also, I think it would mostly be an
> internal implementation detail of shmem helpers, though there is one
> spot in panfrost that takes the lock (2 before this series). So it's
> kind of orthogonal to this series.

The issue roughly is that having multiple per-bo locks gets fun, once you
add in multiple drivers and dynamic dma-buf sharing. Maybe that's never
going to be an issue for drivers using shmem helpers, but who knows. The
cross-driver per-bo lock to untangle that maze is dma_resv, and if you
then also have your own per-bo locks it can get rather interesting. Best
case you end up with two locks nesting, and your own per-bo lock being
fully redundant. Worst case you get different nesting depending whether
you import or export. So that's roughly the context.

Of course fixing locking is going to be easier the fewer users you have.
Once there's lots of code and users of it out there, it's pretty much
impossible.

So yeah it would be a 1:1 replacement with all the per-bo locks you have
now, and seeing how badly it bites.

> Also, I think getting more drivers using shmem is more beneficial than
> aligning the implementations of the GEM helpers. We should at least be
> able to convert vgem and vkms I would think. Various KMS drivers too,
> but there's an issue around kernel mappings (or lack of). There really
> should be little reason for most KMS drivers to have a custom BO as
> CMA or shmem helpers should work.

Yeah agreed on this, I just want to make sure we're not doing this
multiple times ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch