On Wed, Sep 12, 2018 at 05:04:11PM -0500, Steve Wise wrote:
>
>
> On 9/12/2018 4:33 PM, Jason Gunthorpe wrote:
> > 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
> > +++ 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;
>
> Shouldn't reading ev_queue->is_closed be done inside the lock?
It is is read inside the lock, isn't it?
> What if it is set immediately after reading it and before this code
> acquires the lock?
If it is set after unlock then the wake_up_interruptible() sequence
after the set will cause wait_event_interruptible() to wakeup/not
sleep.
Jason
