Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: christian.koenig@amd.com
Subject: Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros
To:     Daniel Vetter <daniel@ffwll.ch>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Cc:     Thomas Hellstrom <thellstrom@vmware.com>,
        lima@lists.freedesktop.org, Peter Zijlstra <peterz@infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        The etnaviv authors <etnaviv@lists.freedesktop.org>,
        Qiang Yu <yuq825@gmail.com>,
        Russell King <linux+etnaviv@armlinux.org.uk>
References: <20190614124125.124181-1-christian.koenig@amd.com>
 <20190614124125.124181-4-christian.koenig@amd.com>
 <20190614131916.GQ3436@hirez.programming.kicks-ass.net>
 <20190614152242.GC23020@phenom.ffwll.local>
 <094da0f7-a0f0-9ed4-d2da-8c6e6d165380@gmail.com>
 <CAKMK7uFcDCJ9sozny1RqqRATwcK39doZNq+NZekvrzuO63ap-Q@mail.gmail.com>
 <d97212dc-367c-28e9-6961-9b99110a4d2e@gmail.com>
 <20190614203040.GE23020@phenom.ffwll.local>
 <CAKMK7uFzg+e315h2e5SmDTQwYTAbgAsxB_pc09ztwA1Wa-mzxw@mail.gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>
Message-ID: <16adcdac-b3e7-02f1-e56b-9b4ad68e146e@gmail.com>
Date:   Mon, 17 Jun 2019 11:30:45 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <CAKMK7uFzg+e315h2e5SmDTQwYTAbgAsxB_pc09ztwA1Wa-mzxw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Am 15.06.19 um 15:56 schrieb Daniel Vetter:
> On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@ffwll.ch> 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@gmail.com> 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
>