On 9/12/2018 5:10 PM, Jason Gunthorpe wrote:
> 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?
Sorry, I misread 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
>
