On Tue, Sep 11, 2018 at 05:18:32PM -0500, Steve Wise wrote:
>
>
> On 9/11/2018 4:25 PM, Jason Gunthorpe wrote:
> > On Tue, Sep 11, 2018 at 04:20:50PM -0500, Steve Wise wrote:
> >> Ignore the old date...
> >>
> >> Jason, is this good to go?
> >
> > I'm not sure, I think your old patch might have been the right
> > one.. But I haven't checked it all through again yet.
> >
> > But I'm still not sure if the kref should be allowed to work like
> > this at all..
> >
> > Leaving a long term dangling filep in the object pointer is not how
> > this was really intended to work.
> >
> > Jason
> >
>
>
> Looking at the original commit (1e7710f3f656),
> ib_uverbs_async_event_close() sets is_closed, yet
> ib_uverbs_comp_event_close() does not. Seems like an oversight in that
> commit maybe? But as for the whole design of this part of the rdma
> core, I'm not to familiar with how it "should work" to comment further.
> I just saw the evidence in the dump and tracked down the issue. :)
Hmm...
Okay, the other reason this is so confusing is because the is_closed
in uverbs_hot_unplug_completion_event_file() is not doing what it
needs to do ..
With the below it starts to make more sense and has sensible
locking.. Once the HW disassociates is_closed becomes set and poll
indicates readable and read returns EIO, which is the standard
semantic for this sort of case.
So, yes, this is the right version of this, we should be keeping
the is_closed in uverbs_hot_unplug_completion_event_file()..
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 50152c1b100452..4a9d84038762fb 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -281,25 +281,20 @@ static ssize_t ib_uverbs_event_read(struct ib_uverbs_event_queue *ev_queue,
spin_lock_irq(&ev_queue->lock);
while (list_empty(&ev_queue->event_list)) {
+ bool closed = ev_queue->is_closed;
+
spin_unlock_irq(&ev_queue->lock);
+ if (closed)
+ return -EIO;
if (filp->f_flags & O_NONBLOCK)
return -EAGAIN;
if (wait_event_interruptible(ev_queue->poll_wait,
(!list_empty(&ev_queue->event_list) ||
- /* The barriers built into wait_event_interruptible()
- * and wake_up() guarentee this will see the null set
- * without using RCU
- */
- !uverbs_file->device->ib_dev)))
+ ev_queue->is_closed)))
return -ERESTARTSYS;
- /* If device was disassociated and no event exists set an error */
- if (list_empty(&ev_queue->event_list) &&
- !uverbs_file->device->ib_dev)
- return -EIO;
-
spin_lock_irq(&ev_queue->lock);
}
@@ -361,7 +356,7 @@ static __poll_t ib_uverbs_event_poll(struct ib_uverbs_event_queue *ev_queue,
poll_wait(filp, &ev_queue->poll_wait, wait);
spin_lock_irq(&ev_queue->lock);
- if (!list_empty(&ev_queue->event_list))
+ if (!list_empty(&ev_queue->event_list) || ev_queue->is_closed)
pollflags = EPOLLIN | EPOLLRDNORM;
spin_unlock_irq(&ev_queue->lock);
