Re: [PATCH v2] nvme/tcp: Add support to set the tcp worker cpu affinity

[Li Feng]

On Fri, Apr 14, 2023 at 4:36 PM Hannes Reinecke <hare@xxxxxxx> wrote:
>
> On 4/13/23 15:29, Li Feng wrote:
> > The default worker affinity policy is using all online cpus, e.g. from 0
> > to N-1. However, some cpus are busy for other jobs, then the nvme-tcp will
> > have a bad performance.
> >
> > This patch adds a module parameter to set the cpu affinity for the nvme-tcp
> > socket worker threads.  The parameter is a comma separated list of CPU
> > numbers.  The list is parsed and the resulting cpumask is used to set the
> > affinity of the socket worker threads.  If the list is empty or the
> > parsing fails, the default affinity is used.
> >
> > Signed-off-by: Li Feng <fengli@xxxxxxxxxx>
> > ---
> >
> > V2 - Fix missing static reported by lkp
> >
> >   drivers/nvme/host/tcp.c | 54 ++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 49c9e7bc9116..47748de5159b 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -31,6 +31,18 @@ static int so_priority;
> >   module_param(so_priority, int, 0644);
> >   MODULE_PARM_DESC(so_priority, "nvme tcp socket optimize priority");
> >
> > +/* Support for specifying the CPU affinity for the nvme-tcp socket worker
> > + * threads.  This is a comma separated list of CPU numbers.  The list is
> > + * parsed and the resulting cpumask is used to set the affinity of the
> > + * socket worker threads.  If the list is empty or the parsing fails, the
> > + * default affinity is used.
> > + */
> > +static char *cpu_affinity_list;
> > +module_param(cpu_affinity_list, charp, 0644);
> > +MODULE_PARM_DESC(cpu_affinity_list, "nvme tcp socket worker cpu affinity list");
> > +
> > +static struct cpumask cpu_affinity_mask;
> > +
> >   #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >   /* lockdep can detect a circular dependency of the form
> >    *   sk_lock -> mmap_lock (page fault) -> fs locks -> sk_lock
> > @@ -1483,6 +1495,41 @@ static bool nvme_tcp_poll_queue(struct nvme_tcp_queue *queue)
> >                         ctrl->io_queues[HCTX_TYPE_POLL];
> >   }
> >
> > +static ssize_t update_cpu_affinity(const char *buf)
> > +{
> > +     cpumask_var_t new_value;
> > +     cpumask_var_t dst_value;
> > +     int err = 0;
> > +
> > +     if (!zalloc_cpumask_var(&new_value, GFP_KERNEL))
> > +             return -ENOMEM;
> > +
> > +     err = bitmap_parselist(buf, cpumask_bits(new_value), nr_cpumask_bits);
> > +     if (err)
> > +             goto free_new_cpumask;
> > +
> > +     if (!zalloc_cpumask_var(&dst_value, GFP_KERNEL)) {
> > +             err = -ENOMEM;
> > +             goto free_new_cpumask;
> > +     }
> > +
> > +     /*
> > +      * If the new_value does not have any intersection with the cpu_online_mask,
> > +      * the dst_value will be empty, then keep the cpu_affinity_mask as cpu_online_mask.
> > +      */
> > +     if (cpumask_and(dst_value, new_value, cpu_online_mask))
> > +             cpu_affinity_mask = *dst_value;
> > +
> > +     free_cpumask_var(dst_value);
> > +
> > +free_new_cpumask:
> > +     free_cpumask_var(new_value);
> > +     if (err)
> > +             pr_err("failed to update cpu affinity mask, bad affinity list [%s], err %d\n",
> > +                     buf, err);
> > +     return err;
> > +}
> > +
> >   static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
> >   {
> >       struct nvme_tcp_ctrl *ctrl = queue->ctrl;
> > @@ -1496,7 +1543,12 @@ static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
> >       else if (nvme_tcp_poll_queue(queue))
> >               n = qid - ctrl->io_queues[HCTX_TYPE_DEFAULT] -
> >                               ctrl->io_queues[HCTX_TYPE_READ] - 1;
> > -     queue->io_cpu = cpumask_next_wrap(n - 1, cpu_online_mask, -1, false);
> > +
> > +     if (!cpu_affinity_list || update_cpu_affinity(cpu_affinity_list) != 0) {
> > +             // Set the default cpu_affinity_mask to cpu_online_mask
> > +             cpu_affinity_mask = *cpu_online_mask;
> > +     }
> > +     queue->io_cpu = cpumask_next_wrap(n - 1, &cpu_affinity_mask, -1, false);
> >   }
> >
> >   static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid)
>
> I am not in favour of this.
> NVMe-over-Fabrics has _virtual_ queues, which really have no
> relationship to the underlying hardware.
> So trying to be clever here by tacking queues to CPUs sort of works if
> you have one subsystem to talk to, but if you have several where each
> exposes a _different_ number of queues you end up with a quite
> suboptimal setting (ie you rely on the resulting cpu sets to overlap,
> but there is no guarantee that they do).

Thanks for your comment.
The current io-queues/cpu map method is not optimal.
It is stupid, and just starts from 0 to the last CPU, which is not configurable.

> Rather leave it to the hardware to sort things out, and rely on the
> blk-mq CPU mapping to get I/O aligned to CPUs.
>
> Cheers,
>
> Hannes
>
The nvme-tcp currently doesn't support a *clever* method to bind the
io-queue and cpu,
it's decided at the allocation and doesn't have a method to change it.
E.g. One subsystem's first io queue binds to CPU0, the next io queue
binds to CPU1, and
if the NIC is located on the other NUMA node 2(CPU24 - CPU36), and
binds the fio to NUMA node 2,
the nvme-tcp will still have poor performance,so how should I tune the
performance?
I have to change the nic irq affinity, but it will hurt other numa
node2 application's performance.
We should maximize one subsystem performance, then maximize multiple
subsystems performance.

This patch gives a chance to adapt the nic and cpu load.
Before and after patch, on my aarch64 server with 4 numa nodes, the
256k read throughput ups
from 1GB/s to 1.4GB/s.

Thanks.