Re: [PATCH V2 1/3] ntb: Add a module option to control affinity of DMA channels

[Dave Jiang <dave.jiang@intel.com>]

On 05/04/2017 02:00 PM, Gary R Hook wrote:
> On 05/04/2017 03:52 PM, Gary R Hook wrote:
>> On 05/04/2017 03:28 PM, Dave Jiang wrote:
>>>
>>>
>>> On 05/04/2017 01:21 PM, Gary R Hook wrote:
>>>> The DMA channel(s)/memory used to transfer data to an NTB device
>>>> may not be required to be on the same node as the device. Add a
>>>> module parameter that allows any candidate channel (aside from
>>>> node assocation) and allocated memory to be used.
>>>>
>>>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>>>> ---
>>>>  drivers/ntb/test/ntb_perf.c |   16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
>>>> index 42756a98a728..bfceae0e6477 100644
>>>> --- a/drivers/ntb/test/ntb_perf.c
>>>> +++ b/drivers/ntb/test/ntb_perf.c
>>>> @@ -101,6 +101,10 @@
>>>>  module_param(use_dma, bool, 0644);
>>>>  MODULE_PARM_DESC(use_dma, "Using DMA engine to measure performance");
>>>>
>>>> +static bool on_node = true; /* default to 1 */
>>>> +module_param(on_node, bool, 0644);
>>>> +MODULE_PARM_DESC(on_node, "Run threads only on NTB device node
>>>> (default: true)");
>>>> +
>>>>  struct perf_mw {
>>>>      phys_addr_t    phys_addr;
>>>>      resource_size_t    phys_size;
>>>> @@ -345,6 +349,10 @@ static int perf_move_data(struct pthr_ctx *pctx,
>>>> char __iomem *dst, char *src,
>>>>
>>>>  static bool perf_dma_filter_fn(struct dma_chan *chan, void *node)
>>>>  {
>>>> +    /* Is the channel required to be on the same node as the
>>>> device? */
>>>> +    if (!on_node)
>>>> +        return true;
>>>> +
>>>>      return dev_to_node(&chan->dev->device) == (int)(unsigned
>>>> long)node;
>>>>  }
>>>>
>>>> @@ -682,7 +690,8 @@ static ssize_t debugfs_run_write(struct file
>>>> *filp, const char __user *ubuf,
>>>>          pr_info("Fix run_order to %u\n", run_order);
>>>>      }
>>>>
>>>> -    node = dev_to_node(&perf->ntb->pdev->dev);
>>>> +    node = on_node ? dev_to_node(&perf->ntb->pdev->dev)
>>>> +               : NUMA_NO_NODE;
>>>>      atomic_set(&perf->tdone, 0);
>>>>
>>>>      /* launch kernel thread */
>>>> @@ -781,7 +790,10 @@ static int perf_probe(struct ntb_client *client,
>>>> struct ntb_dev *ntb)
>>>>
>>>>      node = dev_to_node(&pdev->dev);
>>>>
>>>> -    perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
>>>> +    if (on_node)
>>>> +        perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
>>>> +    else
>>>> +        perf = kzalloc(sizeof(*perf), GFP_KERNEL);
>>>
>>> Is this necessary when NUMA_NO_NODE is passed in?
>>
>> Excellent question. I traced kzalloc_node down to slab_alloc_node(), and
>> it does
>> indeed check (parameter) nodeid against NUMA_NO_NODE. So that would be
>> an option
>> for the node variable above.
>>
>> However, since the code above, as written, doesn't allow for
>> NUMA_NO_NODE, some
>> additional work will be required to properly handle the option. Look for
>> a V3 of
>> this series.
> 
> Actually, I think this engenders some further consideration. The buffer
> allocation code around line 390 also wants to be node-aware. Would it
> make sense to have a switch the completely ignores any NUMA concerns for
> threads and memory? A switch for threads, and a switch for memory? There
> are a number of combinations that may be of interest here, but I'm not
> sure how far I want to/should go at this point. Maybe just a single
> comprehensive switch to turn node-sensitivity on/off (i.e. have on_node
> cover all the bases)?
> 

That's probably ok if we want to just ignore all NUMA awareness for
testing.