Netdev Archive on lore.kernel.org
 help / color / Atom feed
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
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.


  reply index

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 publically 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 \
    /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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox