Re: [PATCH] dmatest: add CPU binding parameter

[Alexander Fomichev <a.fomichev@yadro.com>]

Hi Dave,

Thanks for the comment. Maybe it makes sense to add a note (or comment) that choosing CPU from the same NUMA node as the DMA channel is recommended.
Anyway we can assign a different CPU for every DMA channel.

On Mon, Aug 22, 2022 at 08:40:35AM -0700, Dave Jiang wrote:
> 
> I don't think this would work for multi-socket systems given when it's
> on you bind all threads to a single CPU even if you may have channels on
> a different NUMA node. One possible way to do this is perhaps using
> on_cpu for Nth core for the NUMA node the channel is on? Just throwing
> out ideas.
> 
> 
> > 
> > Signed-off-by: Alexander Fomichev <a.fomichev@yadro.com>
> > ---
> >   drivers/dma/dmatest.c | 23 +++++++++++++++++++++--
> >   1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > index f696246f57fd..c91cbc9e5d1a 100644
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -89,6 +89,10 @@ static bool polled;
> >   module_param(polled, bool, S_IRUGO | S_IWUSR);
> >   MODULE_PARM_DESC(polled, "Use polling for completion instead of interrupts");
> > 
> > +static int on_cpu = -1;
> > +module_param(on_cpu, int, 0644);
> > +MODULE_PARM_DESC(on_cpu, "Bind CPU to run threads on (default: auto scheduled (-1))");
> > +
> >   /**
> >    * struct dmatest_params - test parameters.
> >    * @buf_size:               size of the memcpy test buffer
> > @@ -237,6 +241,7 @@ struct dmatest_thread {
> >   struct dmatest_chan {
> >       struct list_head        node;
> >       struct dma_chan         *chan;
> > +     int                                     cpu;
> >       struct list_head        threads;
> >   };
> > 
> > @@ -602,6 +607,7 @@ static int dmatest_func(void *data)
> >       ret = -ENOMEM;
> > 
> >       smp_rmb();
> > +
> 
> 
> Stray blank line
> 
> >       thread->pending = false;
> >       info = thread->info;
> >       params = &info->params;
> > @@ -1010,6 +1016,7 @@ static int dmatest_add_channel(struct dmatest_info *info,
> >       struct dmatest_chan     *dtc;
> >       struct dma_device       *dma_dev = chan->device;
> >       unsigned int            thread_count = 0;
> > +     char    cpu_str[20];
> >       int cnt;
> > 
> >       dtc = kmalloc(sizeof(struct dmatest_chan), GFP_KERNEL);
> > @@ -1018,6 +1025,13 @@ static int dmatest_add_channel(struct dmatest_info *info,
> >               return -ENOMEM;
> >       }
> > 
> > +     memset(cpu_str, 0, sizeof(cpu_str));
> > +     if (on_cpu >= nr_cpu_ids || on_cpu < -1)
> > +             on_cpu = -1;
> > +     dtc->cpu = on_cpu;
> > +     if (dtc->cpu != -1)
> > +             snprintf(cpu_str, sizeof(cpu_str) - 1, " on CPU #%d", dtc->cpu);
> > +
> >       dtc->chan = chan;
> >       INIT_LIST_HEAD(&dtc->threads);
> > 
> > @@ -1050,8 +1064,8 @@ static int dmatest_add_channel(struct dmatest_info *info,
> >               thread_count += cnt > 0 ? cnt : 0;
> >       }
> > 
> > -     pr_info("Added %u threads using %s\n",
> > -             thread_count, dma_chan_name(chan));
> > +     pr_info("Added %u threads using %s%s\n",
> > +             thread_count, dma_chan_name(chan), cpu_str);
> > 
> >       list_add_tail(&dtc->node, &info->channels);
> >       info->nr_channels++;
> > @@ -1125,6 +1139,11 @@ static void run_pending_tests(struct dmatest_info *info)
> > 
> >               thread_count = 0;
> >               list_for_each_entry(thread, &dtc->threads, node) {
> > +                     if (dtc->cpu != -1) {
> > +                             if (!thread->pending)
> > +                                     continue;
> > +                             kthread_bind(thread->task, dtc->cpu);
> > +                     }
> >                       wake_up_process(thread->task);
> >                       thread_count++;
> >               }

All the best,
Alexander.