Re: [RFC PATCH 1/4] nvme-tcp: optionally limit I/O queue count based on NIC queues

[Christoph Hellwig <hch@lst.de>]

> In such configurations, limiting the number of NVMe-TCP I/O queues to
> the number of NIC hardware queues can improve performance by reducing
> contention and improving locality. Aligning NVMe-TCP worker threads with
> NIC queue topology may also help reduce tail latency.

Yes, this sounds useful.

> 
> Add a new transport option "match_hw_queues" to allow users to
> optionally limit the number of NVMe-TCP I/O queues to the number of NIC
> TX/RX queues. When enabled, the number of I/O queues is set to:
> 
>     min(num_online_cpus, num_nic_queues)
> 
> This behavior is opt-in and does not change existing defaults.

Any good reason for that?  For PCI and RDMA we try to do the right
thing by default.

> +static struct net_device *nvme_tcp_get_netdev(struct nvme_ctrl *ctrl)
> +{
> +	struct net_device *dev = NULL;
> +
> +	if (ctrl->opts->mask & NVMF_OPT_HOST_IFACE)
> +		dev = dev_get_by_name(&init_net, ctrl->opts->host_iface);

Return early here instead of the giant indentation for the new options.

> +	else {
> +		struct nvme_tcp_ctrl *tctrl = to_tcp_ctrl(ctrl);
> +
> +		if (tctrl->addr.ss_family == AF_INET) {

And then split each address family into a helper.  And to me those
look like something that should be in net/.

> +
> +/*
> + * 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)

drop _current to make this a bit more readable?

> @@ -2144,6 +2243,24 @@ 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) &&
> +			(ctrl->opts->mask & NVMF_OPT_MATCH_HW_QUEUES)) {

The more readable formatting would be:

	if (!(ctrl->opts->mask & NVMF_OPT_NR_IO_QUEUES) &&
	    (ctrl->opts->mask & NVMF_OPT_MATCH_HW_QUEUES)) {

> +		int nr_hw_queues;
> +
> +		nr_hw_queues = nvme_tcp_get_netdev_current_queue_count(ctrl);
> +		if (nr_hw_queues <= 0)
> +			goto init_queue;
> +
> +		ctrl->opts->nr_io_queues = min(nr_hw_queues, num_online_cpus());
> +
> +		if (ctrl->opts->nr_io_queues < num_online_cpus())
> +			dev_info(ctrl->device,
> +				"limiting I/O queues to %u (NIC queues %d, CPUs %u)\n",
> +				ctrl->opts->nr_io_queues, nr_hw_queues,
> +				num_online_cpus());
> +	}

And splitting this into a helper would help keeping the flow sane.