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
> > > + 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...
> > 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..
> > > @@ -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.
Jason
