Message-ID: <46946ce9-ad21-0550-4522-f580f77eba1a@nvidia.com>
Date:   Sat, 15 Apr 2023 20:21:15 +0000
From:   Chaitanya Kulkarni <chaitanyak@...dia.com>
To:     Li Feng <fengli@...rtx.com>, Hannes Reinecke <hare@...e.de>
CC:     Keith Busch <kbusch@...nel.org>, Jens Axboe <axboe@...com>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        "open list:NVM EXPRESS DRIVER" <linux-nvme@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        "lifeng1519@...il.com" <lifeng1519@...il.com>
Subject: Re: [PATCH v2] nvme/tcp: Add support to set the tcp worker cpu
 affinity

On 4/14/23 02:35, Li Feng wrote:
> On Fri, Apr 14, 2023 at 4:36 PM Hannes Reinecke <hare@...e.de> 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@...rtx.com>
>>> ---
>>>
>>> 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.
>


For patch like this it needs to be backed by the detailed performance
analysis to start with, both with and without this patch to prove it'
usefulness with quantitative data, that will avoid any further questions
and allow reviewers to come to the conclusion faster ...

Also, please make sure to cover all possible general usecases to avoid
further questions such as one subsystem performance vs multiple susbsys
performance ..

-ck

Powered by blists - more mailing lists