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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 30, 2021 at 01:32:28PM +0200, Christian König wrote:
> 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.

Hm yeah. I guess

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But this poll stuff just massively confuses me.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch