On 6/24/2019 9:06 PM, Jason Gunthorpe wrote:
On Mon, Jun 24, 2019 at 07:55:32PM +0300, Yishai Hadas wrote:
+ /* Explicit filtering to kernel events which may occur frequently */
+ if (event_type == MLX5_EVENT_TYPE_CMD ||
+ event_type == MLX5_EVENT_TYPE_PAGE_REQUEST)
+ return NOTIFY_OK;
+
+ table = container_of(nb, struct mlx5_devx_event_table, devx_nb.nb);
+ dev = container_of(table, struct mlx5_ib_dev, devx_event_table);
+ is_unaffiliated = is_unaffiliated_event(dev->mdev, event_type);
+
+ if (!is_unaffiliated)
+ obj_type = get_event_obj_type(event_type, data);
+ event = xa_load(&table->event_xa, event_type | (obj_type << 16));
+ if (!event)
+ return NOTIFY_DONE;
event should be in the rcu as well
Do we really need this ? I didn't see a flow that really requires
that.
I think there are no frees left? Even so it makes much more sense to
include the event in the rcu as if we ever did need to kfree it would
have to be via rcu
OK
+ while (list_empty(&ev_queue->event_list)) {
+ spin_unlock_irq(&ev_queue->lock);
+
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ if (wait_event_interruptible(ev_queue->poll_wait,
+ (!list_empty(&ev_queue->event_list) ||
+ ev_queue->is_destroyed))) {
+ return -ERESTARTSYS;
+ }
+
+ if (list_empty(&ev_queue->event_list) &&
+ ev_queue->is_destroyed)
+ return -EIO;
All these tests should be under the lock.
We can't call wait_event_interruptible() above which may sleep under the
lock, correct ? are you referring to the list_empty() and
is_destroyed ?
yes
By the way looking in uverb code [1], similar code which is not done under
the lock as of here..
[1] https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L244
Also not a good idea
Why don't we return EIO as soon as is-destroyed happens? What is the
point of flushing out the accumulated events?
It follows the above uverb code/logic that returns existing events even in
that case, also the async command events in this file follows that logic, I
suggest to stay consistent.
Don't follow broken uverbs stuff...
May it be that there is some event that we still want to deliver post
unbind/hot-unplug ? for example IB_EVENT_DEVICE_FATAL in uverbs and
others from the driver code.
Not sure that we want to change this logic.
What do you think ?
Maybe the event should be re-added on error? Tricky.
What will happen if another copy_to_user may then fail again (loop ?) ...
not sure that we want to get into this tricky handling ...
As of above, It follows the logic from uverbs at that area.
https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L267
again it is wrong...
There is no loop if you just stick the item back on the head of the
list and exit, which is probably the right thing to do..
What if copy_to_user() will fail again just later on ? we might end-up
with loop of read(s) that always find an event as it was put back.
I suggest to leave this flow as it's now, at least for this series
submission.
Agree ?
@@ -2374,6 +2705,17 @@ static int devx_hot_unplug_async_cmd_event_file(struct ib_uobject *uobj,
static int devx_hot_unplug_async_event_file(struct ib_uobject *uobj,
enum rdma_remove_reason why)
{
+ struct devx_async_event_file *ev_file =
+ container_of(uobj, struct devx_async_event_file,
+ uobj);
+ struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
+
+ spin_lock_irq(&ev_queue->lock);
+ ev_queue->is_destroyed = 1;
+ spin_unlock_irq(&ev_queue->lock);
+
+ if (why == RDMA_REMOVE_DRIVER_REMOVE)
+ wake_up_interruptible(&ev_queue->poll_wait);
Why isn't this wakeup always done?
Maybe you are right and this can be always done to wake up any readers as
the 'is_destroyed' was set.
By the way, any idea why it was done as such in uverbs [1] for similar flow
? also the command events follows that.
I don't know, it is probably pointless too.
If we don't need it here then we shouldn't have it.
These random pointless ifs bother me as we have to spend time trying
to figure out that they are pointless down the road.
OK, will drop this if.
