Re: [RESEND PATCH v2 2/4] nvme-tcp: limit I/O queue count based on NIC queue count

On 07/31, Nilay Shroff wrote:
> NVMe-TCP currently provisions I/O queues based primarily on the number
> of online CPUs. On systems where the CPU count significantly exceeds the
> number of NIC hardware queues, multiple NVMe-TCP I/O queues end up
> sharing the same NIC TX/RX queues. This increases lock contention,
> cacheline bouncing, and inter-processor interrupts (IPIs), reducing I/O
> efficiency.
> 
> Limit the number of NVMe-TCP default I/O queues to the smaller of the
> number of online CPUs and the number of NIC hardware queues. Aligning
> the number of NVMe-TCP I/O queues with the NIC queue topology reduces
> queue sharing, improves locality, and can improve throughput while
> reducing tail latency.
> 
> The number of NVMe-TCP I/O queues is now limited to:
> 
>     min(num_online_cpus, num_nic_queues)
> 
> Signed-off-by: Nilay Shroff <[email protected]>
> ---
>  drivers/nvme/host/tcp.c | 61 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index ba5c7b3e2a7c..a2110287099e 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1774,6 +1774,50 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>  	return ret;
>  }
>  
> +static struct net_device *nvme_tcp_get_netdev(struct nvme_ctrl *ctrl,
> +		netdevice_tracker *tracker, gfp_t gfp)
> +{
> +	struct net_device *dev = NULL;
> +
> +	if (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)
> +		dev = netdev_get_by_name(&init_net, ctrl->opts->host_iface,
> +				tracker, gfp);
> +	else {
> +		struct nvme_tcp_ctrl *tctrl = to_tcp_ctrl(ctrl);
> +		struct sockaddr_storage *src = NULL, *dest = NULL;
> +
> +		if (ctrl->opts->mask & NVMF_OPT_HOST_TRADDR)
> +			src = &tctrl->src_addr;
> +
> +		dest = &tctrl->addr;
> +
> +		dev = netdev_get_by_addr(&init_net, src, dest, tracker, gfp);
> +	}
> +	return dev;
> +}
> +
> +/*
> + * Returns number of active NIC queues (min of TX/RX), or 0 if device cannot
> + * be determined.
> + */
> +static int nvme_tcp_get_netdev_current_queue_count(struct nvme_ctrl *ctrl)
> +{
> +	struct net_device *dev;
> +	int tx_queues, rx_queues;
> +	netdevice_tracker tracker;
> +
> +	dev = nvme_tcp_get_netdev(ctrl, &tracker, GFP_KERNEL);
> +	if (!dev)
> +		return 0;
> +
> +	tx_queues = dev->real_num_tx_queues;
> +	rx_queues = dev->real_num_rx_queues;
> +
> +	netdev_put(dev, &tracker);
> +
> +	return min(tx_queues, rx_queues);
> +}
> +
>  static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  				key_serial_t pskid)
>  {
> @@ -2165,6 +2209,23 @@ static int nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
>  	unsigned int nr_io_queues;
>  	int ret;

[..]
 
> +	if (!(ctrl->opts->mask & NVMF_OPT_NR_IO_QUEUES)) {
> +		int nr_hw_queues;

Looks like the userspace can already pass the preferred number of queues,
so in this case, why not do all this netdev resolution and queue
estimation in the userspace? Presumably most or the users you care
about always go through nvme-cli, right?