Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian KÃnig]

Am 15.06.19 um 15:56 schrieb Daniel Vetter:
> On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> > On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian KÃnig wrote:
> > > Am 14.06.19 um 20:24 schrieb Daniel Vetter:
> > > > On Fri, Jun 14, 2019 at 8:10 PM Christian KÃnig <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
> > > > > [SNIP]
> > > > > WW_MUTEX_LOCK_BEGIN()
> > > > >
> > > > > lock(lru_lock);
> > > > >
> > > > > while (bo = list_first(lru)) {
> > > > >    if (kref_get_unless_zero(bo)) {
> > > > >            unlock(lru_lock);
> > > > >            WW_MUTEX_LOCK(bo->ww_mutex);
> > > > >            lock(lru_lock);
> > > > >    } else {
> > > > >            /* bo is getting freed, steal it from the freeing process
> > > > >             * or just ignore */
> > > > >    }
> > > > > }
> > > > > unlock(lru_lock)
> > > > >
> > > > > WW_MUTEX_LOCK_END;
> > > Ah, now I know what you mean. And NO, that approach doesn't work.
> > >
> > > See for the correct ww_mutex dance we need to use the iterator multiple
> > > times.
> > >
> > > Once to give us the BOs which needs to be locked and another time to give us
> > > the BOs which needs to be unlocked in case of a contention.
> > >
> > > That won't work with the approach you suggest here.
> > A right, drat.
> >
> > Maybe give up on the idea to make this work for ww_mutex in general, and
> > just for drm_gem_buffer_object? I'm just about to send out a patch series
> > which makes sure that a lot more drivers set gem_bo.resv correctly (it
> > will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
> > list_head to gem_bo (won't really matter, but not something we can do with
> > ww_mutex really), so that the unlock walking doesn't need to reuse the
> > same iterator. That should work I think ...
> >
> > Also, it would almost cover everything you want to do. For ttm we'd need
> > to make ttm_bo a subclass of gem_bo (and maybe not initialize that
> > embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).
> >
> > Just some ideas, since copypasting the ww_mutex dance into all drivers is
> > indeed not great.
> Even better we don't need to force everyone to use drm_gem_object, the
> hard work is already done with the reservation_object. We could add a
> list_head there for unwinding, and then the locking helpers would look
> a lot cleaner and simpler imo. reservation_unlock_all() would even be
> a real function! And if we do this then I think we should also have a
> reservation_acquire_ctx, to store the list_head and maybe anything
> else.
>
> Plus all the code you want to touch is dealing with
> reservation_object, so that's all covered. And it mirros quite a bit
> what we've done with struct drm_modeset_lock, to wrap ww_mutex is
> something easier to deal with for kms.

That's a rather interesting idea.

I wouldn't use a list_head cause that has a rather horrible caching 
footprint for something you want to use during CS, but apart from that 
the idea sounds like it would also solve a bunch of problem during eviction.

Going to give that a try,
Christian.

> -Daniel