Re: [PATCH 06/18] nvme-tcp: call 'queue->data_ready()' in nvme_tcp_data_ready()

Sagi Grimberg
2023-03-21T13:44:14Z

> Call the original data_ready() callback in nvme_tcp_data_ready()
> to avoid a receive stall.

Can you please improve the description to include what is the stall?
For example, does the stall exist today? If it is, I would like to
separate such patches from this set and include them asap.

>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/tcp.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 0e14b1b90855..0512eb289dcf 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -936,12 +936,14 @@ static void nvme_tcp_data_ready(struct sock *sk)
>
> trace_sk_data_ready(sk);
>
> - read_lock_bh(&sk->sk_callback_lock);
> - queue = sk->sk_user_data;
> + rcu_read_lock_bh();

Now I understand your comment from a previous patch.
Can you explain why is this convention needed?

I would prefer to have it as a separate patch with an
explanation to why it is needed.

> + queue = rcu_dereference_sk_user_data(sk);
> + if (queue->data_ready)
> + queue->data_ready(sk);

Is the tls data_ready call sync or async? just for general knowledge?

> if (likely(queue && queue->rd_enabled) &&
> !test_bit(NVME_TCP_Q_POLLING, &queue->flags))
> queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
> - read_unlock_bh(&sk->sk_callback_lock);
> + rcu_read_unlock_bh();
> }
>
> static void nvme_tcp_write_space(struct sock *sk)