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

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

Am 30.09.21 um 12:26 schrieb Daniel Vetter:
> On Thu, Sep 30, 2021 at 11:48:42AM +0200, Christian König wrote:
>>
>> 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.
> Ha yes I missed that. Boolean sounds much better.
>>> - 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.
> Ok with that clarified can you cut a v8 with booleans and I whack an r-b
> on that? Or just include it, I'm massively burried here and trying to dig
> out :-/
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> (with the booleans)

My bad, boolean won't work because we also use the flags for the call to 
"wake_up_locked_poll(dcb->poll, dcb->active);".

Anyway that doesn't really change anything on the logic and I inherited 
that handling from the existing code, just moved it around a bit.

Christian.

>
> -Daniel
>> 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>
>>>>>
>>>>>