Re: [PATCH] dma-buf: fix and rework dma_buf_poll v7

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 30.09.21 um 11:00 schrieb Daniel Vetter:
> On Wed, Sep 22, 2021 at 01:08:44PM +0200, Christian König wrote:
>> Totally forgotten to ping once more about that.
>>
>> Michel has tested this now and I think we should push it ASAP. So can I get
>> an rb?
> 		spin_lock_irq(&dmabuf->poll.lock);
> 		if (dcb->active)
> 			events &= ~EPOLLIN;
> 		else
> 			dcb->active = EPOLLIN;
> 		spin_unlock_irq(&dmabuf->poll.lock);
>
>
> This pattern (and same for EPOLLOUT) makes no sense to me. I guess the
> intent is that this filters out events for which we're already listening,
> but:
>
> - it checks for any active event, not the specific one

Which is correct. We now use one dcb for EPOLLIN and another one for 
EPOLLOUT.

We could make that a boolean instead if that makes it cleaner.

> - if we're waiting already and new fences have been added, wont we miss
>    them?

No, when we are already waiting the callback will sooner or later fire 
and result in a re-check.

> Or does this all work because the poll machinery restarts everything
> again?

Yes, exactly that. Otherwise waiting for multiple fences wouldn't work 
either.

Regards,
Christian.

>
> I'm totally confused here for sure. The other changes in the patch look
> good though.
> -Daniel
>
>> Thanks,
>> Christian.
>>
>> Am 23.07.21 um 10:04 schrieb Michel Dänzer:
>>> On 2021-07-20 3:11 p.m., Christian König wrote:
>>>> Daniel pointed me towards this function and there are multiple obvious problems
>>>> in the implementation.
>>>>
>>>> First of all the retry loop is not working as intended. In general the retry
>>>> makes only sense if you grab the reference first and then check the sequence
>>>> values.
>>>>
>>>> Then we should always also wait for the exclusive fence.
>>>>
>>>> It's also good practice to keep the reference around when installing callbacks
>>>> to fences you don't own.
>>>>
>>>> And last the whole implementation was unnecessary complex and rather hard to
>>>> understand which could lead to probably unexpected behavior of the IOCTL.
>>>>
>>>> Fix all this by reworking the implementation from scratch. Dropping the
>>>> whole RCU approach and taking the lock instead.
>>>>
>>>> Only mildly tested and needs a thoughtful review of the code.
>>>>
>>>> v2: fix the reference counting as well
>>>> v3: keep the excl fence handling as is for stable
>>>> v4: back to testing all fences, drop RCU
>>>> v5: handle in and out separately
>>>> v6: add missing clear of events
>>>> v7: change coding style as suggested by Michel, drop unused variables
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> CC: stable@vger.kernel.org
>>> Working fine with https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880
>>>
>>> Tested-by: Michel Dänzer <mdaenzer@redhat.com>
>>>
>>>