Re: [PATCH v2] nvme-tcp: send quota for nvme_tcp_send_all()

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 10/24/22 08:03, Daniel Wagner wrote:
> Add a send quota in nvme_tcp_send_all() to avoid stalls when sending
> large amounts of requests.
> 
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> 
> IMO, this patch might still be a good idea to add. At least in my test
> setup where I only have one ethernet port it makes a big difference
> when accessing the system via ssh. When nvme-tcp is pushing a lot of
> data via the network, the ssh session is completely blocked by the
> storage traffic. With it, the ssh session stays responsive. >

I'm not sure whether it is possible but is there a way to gather
some form of quantitative data and present it here to we all know
exactly which aspect is improving by this patch in context of
"ssd session is completely blocked" ?

> I suspect a proper storage setup would use more than one ethernet
> port.
> 
> Daniel
> 
> v1:
>    https://lore.kernel.org/linux-nvme/20220519062617.39715-4-hare@suse.de/
> 
>   drivers/nvme/host/tcp.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 3e7b29d07c71..84a66ca208c8 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -308,12 +308,23 @@ static inline void nvme_tcp_advance_req(struct nvme_tcp_request *req,
>   
>   static inline void nvme_tcp_send_all(struct nvme_tcp_queue *queue)
>   {
> -	int ret;
> +	unsigned long deadline = jiffies + msecs_to_jiffies(1);

We need to provide some flexibility to set this value as one specified
value may not work with all the setups and H/W.

Can we make it tunable and not statically coded ?

>   
>   	/* drain the send queue as much as we can... */
>   	do {
> -		ret = nvme_tcp_try_send(queue);
> -	} while (ret > 0);
> +		bool pending = false;
> +		int result;
> +
> +		result = nvme_tcp_try_send(queue);
> +		if (result > 0)
> +			pending = true;
> +		else if (unlikely(result < 0))
> +			return;
> +
> +		if (!pending)
> +			return;
> +
> +	} while (!time_after(jiffies, deadline)); /* quota is exhausted */
>   }
>   
>   static inline bool nvme_tcp_queue_more(struct nvme_tcp_queue *queue)

-ck