From: Yishai Hadas <yishaih@dev.mellanox.co.il> To: Jason Gunthorpe <jgg@mellanox.com> Cc: Leon Romanovsky <leon@kernel.org>, Doug Ledford <dledford@redhat.com>, Leon Romanovsky <leonro@mellanox.com>, RDMA mailing list <linux-rdma@vger.kernel.org>, Yishai Hadas <yishaih@mellanox.com>, Saeed Mahameed <saeedm@mellanox.com>, linux-netdev <netdev@vger.kernel.org> Subject: Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event Date: Tue, 25 Jun 2019 17:41:49 +0300 [thread overview] Message-ID: <a2380ea6-4542-c72c-96f7-e68786847ccc@dev.mellanox.co.il> (raw) In-Reply-To: <20190624180558.GL7418@mellanox.com> 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.
next prev parent reply other threads:[~2019-06-25 14:41 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-18 17:15 [PATCH rdma-next v1 00/12] DEVX asynchronous events Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 01/12] net/mlx5: Fix mlx5_core_destroy_cq() error flow Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 02/12] net/mlx5: Use event mask based on device capabilities Leon Romanovsky 2019-06-27 0:23 ` Saeed Mahameed 2019-06-18 17:15 ` [PATCH mlx5-next v1 03/12] net/mlx5: Expose the API to register for ANY event Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 04/12] net/mlx5: mlx5_core_create_cq() enhancements Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 05/12] net/mlx5: Report a CQ error event only when a handler was set Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 06/12] net/mlx5: Report EQE data upon CQ completion Leon Romanovsky 2019-06-18 17:15 ` [PATCH mlx5-next v1 07/12] net/mlx5: Expose device definitions for object events Leon Romanovsky 2019-06-18 17:15 ` [PATCH rdma-next v1 08/12] IB/mlx5: Introduce MLX5_IB_OBJECT_DEVX_ASYNC_EVENT_FD Leon Romanovsky 2019-06-24 11:51 ` Jason Gunthorpe 2019-06-24 13:25 ` Yishai Hadas 2019-06-24 14:30 ` Jason Gunthorpe 2019-06-18 17:15 ` [PATCH rdma-next v1 09/12] IB/mlx5: Register DEVX with mlx5_core to get async events Leon Romanovsky 2019-06-24 11:52 ` Jason Gunthorpe 2019-06-24 13:36 ` Yishai Hadas 2019-06-24 14:30 ` Jason Gunthorpe 2019-06-18 17:15 ` [PATCH rdma-next v1 10/12] IB/mlx5: Enable subscription for device events over DEVX Leon Romanovsky 2019-06-24 11:57 ` Jason Gunthorpe 2019-06-24 16:13 ` Yishai Hadas 2019-06-24 17:56 ` Jason Gunthorpe 2019-06-18 17:15 ` [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event Leon Romanovsky 2019-06-24 12:03 ` Jason Gunthorpe 2019-06-24 16:55 ` Yishai Hadas 2019-06-24 18:06 ` Jason Gunthorpe 2019-06-25 14:41 ` Yishai Hadas [this message] 2019-06-25 20:23 ` Jason Gunthorpe 2019-06-18 17:15 ` [PATCH rdma-next v1 12/12] IB/mlx5: Add DEVX support for CQ events Leon Romanovsky 2019-06-24 12:04 ` Jason Gunthorpe 2019-06-24 17:03 ` Yishai Hadas 2019-06-24 18:06 ` Jason Gunthorpe 2019-06-18 18:51 ` [PATCH rdma-next v1 00/12] DEVX asynchronous events Saeed Mahameed 2019-06-19 4:45 ` Leon Romanovsky 2019-06-24 21:57 ` Saeed Mahameed 2019-06-30 8:53 ` Leon Romanovsky
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a2380ea6-4542-c72c-96f7-e68786847ccc@dev.mellanox.co.il \ --to=yishaih@dev.mellanox.co.il \ --cc=dledford@redhat.com \ --cc=jgg@mellanox.com \ --cc=leon@kernel.org \ --cc=leonro@mellanox.com \ --cc=linux-rdma@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=saeedm@mellanox.com \ --cc=yishaih@mellanox.com \ --subject='Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).