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

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Tue, Jun 22, 2021 at 3:07 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Crap, hit enter to early before adding a cover letter.
>
> This is the same patch as before, but as requested I'm keeping the
> exclusive fence handling as it is for now.
>
> Daniel can you double check this and/or make sure that it is tested?
>
> I only smoke tested it and the code is so complicated that I'm not sure
> I catched all side effects.

So I've thought about this some more, and we actually have docs for
how this is supposed to work:

https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#implicit-fence-poll-support

Docs are pretty clear that we want both read and write for EPOLLOUT or
well both exclusive and shared fences. So I guess back to your actual
thing, but maybe we should get some acks from userspace people for it
(Michel, Pekka, Simon probably usual suspects).

The other thing I brought up and I haven't seen you reply to (maybe
missed it) is whether we shouldn't just use dma_resv_get_fences(). We
need to do the refcounting anyway, and this avoids us having to
open-code this very nasty code. Finally, and imo most important, this
is what's also used in drm_gem_fence_array_add_implicit(), which many
drivers use to handle their implicit in-fences. So would be nice to
have exactly matching code between that and what dma-buf poll does for
"can I read" and "can I write".

Thoughts?
-Daniel

>
> Regards,
> Christian.
>
> Am 22.06.21 um 15:04 schrieb Christian König:
> > 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.
> >
> > 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.
> >
> > 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
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > CC: stable@vger.kernel.org
> > ---
> >   drivers/dma-buf/dma-buf.c | 133 ++++++++++++++++----------------------
> >   include/linux/dma-buf.h   |   2 +-
> >   2 files changed, 55 insertions(+), 80 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > index eadd1eaa2fb5..e97c3a9d98d5 100644
> > --- a/drivers/dma-buf/dma-buf.c
> > +++ b/drivers/dma-buf/dma-buf.c
> > @@ -72,7 +72,7 @@ static void dma_buf_release(struct dentry *dentry)
> >        * If you hit this BUG() it means someone dropped their ref to the
> >        * dma-buf while still having pending operation to the buffer.
> >        */
> > -     BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active);
> > +     BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
> >
> >       dmabuf->ops->release(dmabuf);
> >
> > @@ -202,16 +202,20 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> >       wake_up_locked_poll(dcb->poll, dcb->active);
> >       dcb->active = 0;
> >       spin_unlock_irqrestore(&dcb->poll->lock, flags);
> > +     dma_fence_put(fence);
> >   }
> >
> >   static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> >   {
> > +     struct dma_buf_poll_cb_t *dcb;
> >       struct dma_buf *dmabuf;
> >       struct dma_resv *resv;
> >       struct dma_resv_list *fobj;
> >       struct dma_fence *fence_excl;
> > -     __poll_t events;
> >       unsigned shared_count, seq;
> > +     struct dma_fence *fence;
> > +     __poll_t events;
> > +     int r, i;
> >
> >       dmabuf = file->private_data;
> >       if (!dmabuf || !dmabuf->resv)
> > @@ -225,99 +229,70 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
> >       if (!events)
> >               return 0;
> >
> > +     dcb = events & EPOLLOUT ? &dmabuf->cb_out : &dmabuf->cb_in;
> > +
> > +     /* Only queue a new one if we are not still waiting for the old one */
> > +     spin_lock_irq(&dmabuf->poll.lock);
> > +     if (dcb->active)
> > +             events = 0;
> > +     else
> > +             dcb->active = events;
> > +     spin_unlock_irq(&dmabuf->poll.lock);
> > +     if (!events)
> > +             return 0;
> > +
> >   retry:
> >       seq = read_seqcount_begin(&resv->seq);
> >       rcu_read_lock();
> >
> >       fobj = rcu_dereference(resv->fence);
> > -     if (fobj)
> > +     if (fobj && events & EPOLLOUT)
> >               shared_count = fobj->shared_count;
> >       else
> >               shared_count = 0;
> > -     fence_excl = rcu_dereference(resv->fence_excl);
> > -     if (read_seqcount_retry(&resv->seq, seq)) {
> > -             rcu_read_unlock();
> > -             goto retry;
> > -     }
> >
> > -     if (fence_excl && (!(events & EPOLLOUT) || shared_count == 0)) {
> > -             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_excl;
> > -             __poll_t pevents = EPOLLIN;
> > -
> > -             if (shared_count == 0)
> > -                     pevents |= EPOLLOUT;
> > -
> > -             spin_lock_irq(&dmabuf->poll.lock);
> > -             if (dcb->active) {
> > -                     dcb->active |= pevents;
> > -                     events &= ~pevents;
> > -             } else
> > -                     dcb->active = pevents;
> > -             spin_unlock_irq(&dmabuf->poll.lock);
> > -
> > -             if (events & pevents) {
> > -                     if (!dma_fence_get_rcu(fence_excl)) {
> > -                             /* force a recheck */
> > -                             events &= ~pevents;
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                     } else if (!dma_fence_add_callback(fence_excl, &dcb->cb,
> > -                                                        dma_buf_poll_cb)) {
> > -                             events &= ~pevents;
> > -                             dma_fence_put(fence_excl);
> > -                     } else {
> > -                             /*
> > -                              * No callback queued, wake up any additional
> > -                              * waiters.
> > -                              */
> > -                             dma_fence_put(fence_excl);
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                     }
> > +     for (i = 0; i < shared_count; ++i) {
> > +             fence = rcu_dereference(fobj->shared[i]);
> > +             fence = dma_fence_get_rcu(fence);
> > +             if (!fence || read_seqcount_retry(&resv->seq, seq)) {
> > +                     /* Concurrent modify detected, force re-check */
> > +                     dma_fence_put(fence);
> > +                     rcu_read_unlock();
> > +                     goto retry;
> >               }
> > -     }
> >
> > -     if ((events & EPOLLOUT) && shared_count > 0) {
> > -             struct dma_buf_poll_cb_t *dcb = &dmabuf->cb_shared;
> > -             int i;
> > -
> > -             /* Only queue a new callback if no event has fired yet */
> > -             spin_lock_irq(&dmabuf->poll.lock);
> > -             if (dcb->active)
> > -                     events &= ~EPOLLOUT;
> > -             else
> > -                     dcb->active = EPOLLOUT;
> > -             spin_unlock_irq(&dmabuf->poll.lock);
> > -
> > -             if (!(events & EPOLLOUT))
> > +             r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > +             if (!r) {
> > +                     /* Callback queued */
> > +                     events = 0;
> >                       goto out;
> > +             }
> > +             dma_fence_put(fence);
> > +     }
> >
> > -             for (i = 0; i < shared_count; ++i) {
> > -                     struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
> > -
> > -                     if (!dma_fence_get_rcu(fence)) {
> > -                             /*
> > -                              * fence refcount dropped to zero, this means
> > -                              * that fobj has been freed
> > -                              *
> > -                              * call dma_buf_poll_cb and force a recheck!
> > -                              */
> > -                             events &= ~EPOLLOUT;
> > -                             dma_buf_poll_cb(NULL, &dcb->cb);
> > -                             break;
> > -                     }
> > -                     if (!dma_fence_add_callback(fence, &dcb->cb,
> > -                                                 dma_buf_poll_cb)) {
> > -                             dma_fence_put(fence);
> > -                             events &= ~EPOLLOUT;
> > -                             break;
> > -                     }
> > +     fence = dma_resv_get_excl(resv);
> > +     if (fence && shared_count == 0) {
> > +             fence = dma_fence_get_rcu(fence);
> > +             if (!fence || read_seqcount_retry(&resv->seq, seq)) {
> > +                     /* Concurrent modify detected, force re-check */
> >                       dma_fence_put(fence);
> > +                     rcu_read_unlock();
> > +                     goto retry;
> > +
> >               }
> >
> > -             /* No callback queued, wake up any additional waiters. */
> > -             if (i == shared_count)
> > -                     dma_buf_poll_cb(NULL, &dcb->cb);
> > +             r = dma_fence_add_callback(fence, &dcb->cb, dma_buf_poll_cb);
> > +             if (!r) {
> > +                     /* Callback queued */
> > +                     events = 0;
> > +                     goto out;
> > +             }
> > +             dma_fence_put(fence_excl);
> >       }
> >
> > +     /* No callback queued, wake up any additional waiters. */
> > +     dma_buf_poll_cb(NULL, &dcb->cb);
> > +
> >   out:
> >       rcu_read_unlock();
> >       return events;
> > @@ -562,8 +537,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
> >       dmabuf->owner = exp_info->owner;
> >       spin_lock_init(&dmabuf->name_lock);
> >       init_waitqueue_head(&dmabuf->poll);
> > -     dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
> > -     dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
> > +     dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
> > +     dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
> >
> >       if (!resv) {
> >               resv = (struct dma_resv *)&dmabuf[1];
> > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> > index efdc56b9d95f..7e747ad54c81 100644
> > --- a/include/linux/dma-buf.h
> > +++ b/include/linux/dma-buf.h
> > @@ -329,7 +329,7 @@ struct dma_buf {
> >               wait_queue_head_t *poll;
> >
> >               __poll_t active;
> > -     } cb_excl, cb_shared;
> > +     } cb_in, cb_out;
> >   };
> >
> >   /**
>


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