[PATCH V3 0/3] Add/expose control features for the NTB perf test module

[PATCH V3 0/3] Add/expose control features for the NTB perf test module

[Gary R Hook]

The goal of this series is to add and expose some control mechanisms for the
performance test module:

 - Provide a switch to allow memory and threads to be anywhere, as
   opposed to only the NTB node.
 - The debug filesystem entries do not need to be retained after they
   are set up. Remove the variables from the context structure and make
   them automatic,
 - Create a debug-fs entry for every module parameter. This allows test
   parameters to be changed, and additional tests to be run, without
   unloading the ntb_perf module each time.

Changes since V2:
 - Use on_node to decide if -anything- is required to be on the NTB node.

Changes since V1:
 - Split patch into appropriate pieces.

---

Gary R Hook (3):
      ntb: Add a module option to control affinity of DMA channels
      ntb: Remove debug-fs variables from the context structure
      ntb: Add more debugfs support for ntb_perf testing options


 drivers/ntb/test/ntb_perf.c |   72 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 18 deletions(-)

--
You can't fix stupid. But you can fix ignorance.

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

[Gary R Hook]

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, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 42756a98a728..0ec9f56f6999 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;
 }
 
@@ -362,7 +370,7 @@ static int ntb_perf_thread(void *data)
 
 	pr_debug("kthread %s starting...\n", current->comm);
 
-	node = dev_to_node(&pdev->dev);
+	node = on_node ? dev_to_node(&pdev->dev) : NUMA_NO_NODE;
 
 	if (use_dma && !pctx->dma_chan) {
 		dma_cap_mask_t dma_mask;
@@ -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 */
@@ -779,8 +788,7 @@ static int perf_probe(struct ntb_client *client, struct ntb_dev *ntb)
 	if (ntb_peer_port_count(ntb) != NTB_DEF_PEER_CNT)
 		dev_warn(&ntb->dev, "Multi-port NTB devices unsupported\n");
 
-	node = dev_to_node(&pdev->dev);
-
+	node = on_node ? dev_to_node(&pdev->dev) : NUMA_NO_NODE;
 	perf = kzalloc_node(sizeof(*perf), GFP_KERNEL, node);
 	if (!perf) {
 		rc = -ENOMEM;

[PATCH V3 2/3] ntb: Remove debug-fs variables from the context structure

[Gary R Hook]

The Debug FS entries manage themselves; we don't need to hang onto
them in the context structure.

Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/ntb/test/ntb_perf.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 0ec9f56f6999..60e0dd320aef 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -140,9 +140,6 @@ struct perf_ctx {
 	bool			link_is_up;
 	struct delayed_work	link_work;
 	wait_queue_head_t	link_wq;
-	struct dentry		*debugfs_node_dir;
-	struct dentry		*debugfs_run;
-	struct dentry		*debugfs_threads;
 	u8			perf_threads;
 	/* mutex ensures only one set of threads run at once */
 	struct mutex		run_mutex;
@@ -737,6 +734,9 @@ static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
 static int perf_debugfs_setup(struct perf_ctx *perf)
 {
 	struct pci_dev *pdev = perf->ntb->pdev;
+	struct dentry *debugfs_node_dir;
+	struct dentry *debugfs_run;
+	struct dentry *debugfs_threads;
 
 	if (!debugfs_initialized())
 		return -ENODEV;
@@ -747,21 +747,21 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
 			return -ENODEV;
 	}
 
-	perf->debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
-						    perf_debugfs_dir);
-	if (!perf->debugfs_node_dir)
+	debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
+					      perf_debugfs_dir);
+	if (!debugfs_node_dir)
 		return -ENODEV;
 
-	perf->debugfs_run = debugfs_create_file("run", S_IRUSR | S_IWUSR,
-						perf->debugfs_node_dir, perf,
-						&ntb_perf_debugfs_run);
-	if (!perf->debugfs_run)
+	debugfs_run = debugfs_create_file("run", S_IRUSR | S_IWUSR,
+					  debugfs_node_dir, perf,
+					  &ntb_perf_debugfs_run);
+	if (!debugfs_run)
 		return -ENODEV;
 
-	perf->debugfs_threads = debugfs_create_u8("threads", S_IRUSR | S_IWUSR,
-						  perf->debugfs_node_dir,
-						  &perf->perf_threads);
-	if (!perf->debugfs_threads)
+	debugfs_threads = debugfs_create_u8("threads", S_IRUSR | S_IWUSR,
+					    debugfs_node_dir,
+					    &perf->perf_threads);
+	if (!debugfs_threads)
 		return -ENODEV;
 
 	return 0;

Re: [PATCH V3 2/3] ntb: Remove debug-fs variables from the context structure

[Jon Mason]

On Tue, May 09, 2017 at 09:33:28AM -0500, Gary R Hook wrote:
> The Debug FS entries manage themselves; we don't need to hang onto
> them in the context structure.
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/ntb/test/ntb_perf.c |   28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> index 0ec9f56f6999..60e0dd320aef 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -140,9 +140,6 @@ struct perf_ctx {
>  	bool			link_is_up;
>  	struct delayed_work	link_work;
>  	wait_queue_head_t	link_wq;
> -	struct dentry		*debugfs_node_dir;
> -	struct dentry		*debugfs_run;
> -	struct dentry		*debugfs_threads;
>  	u8			perf_threads;
>  	/* mutex ensures only one set of threads run at once */
>  	struct mutex		run_mutex;
> @@ -737,6 +734,9 @@ static ssize_t debugfs_run_write(struct file *filp, const char __user *ubuf,
>  static int perf_debugfs_setup(struct perf_ctx *perf)
>  {
>  	struct pci_dev *pdev = perf->ntb->pdev;
> +	struct dentry *debugfs_node_dir;
> +	struct dentry *debugfs_run;
> +	struct dentry *debugfs_threads;
>  
>  	if (!debugfs_initialized())
>  		return -ENODEV;
> @@ -747,21 +747,21 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
>  			return -ENODEV;
>  	}
>  
> -	perf->debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
> -						    perf_debugfs_dir);
> -	if (!perf->debugfs_node_dir)
> +	debugfs_node_dir = debugfs_create_dir(pci_name(pdev),
> +					      perf_debugfs_dir);
> +	if (!debugfs_node_dir)
>  		return -ENODEV;
>  
> -	perf->debugfs_run = debugfs_create_file("run", S_IRUSR | S_IWUSR,
> -						perf->debugfs_node_dir, perf,
> -						&ntb_perf_debugfs_run);
> -	if (!perf->debugfs_run)
> +	debugfs_run = debugfs_create_file("run", S_IRUSR | S_IWUSR,
> +					  debugfs_node_dir, perf,
> +					  &ntb_perf_debugfs_run);
> +	if (!debugfs_run)
>  		return -ENODEV;
>  
> -	perf->debugfs_threads = debugfs_create_u8("threads", S_IRUSR | S_IWUSR,
> -						  perf->debugfs_node_dir,
> -						  &perf->perf_threads);
> -	if (!perf->debugfs_threads)
> +	debugfs_threads = debugfs_create_u8("threads", S_IRUSR | S_IWUSR,
> +					    debugfs_node_dir,
> +					    &perf->perf_threads);
> +	if (!debugfs_threads)
>  		return -ENODEV;

I'm going to apply this, since it isn't that different then what
exists now.  However, I would like to see this have an error path that
unrolls the creation if one of them fails.

Thanks,
Jon

>  
>  	return 0;
> 

Re: [PATCH V3 2/3] ntb: Remove debug-fs variables from the context structure

[Gary R Hook]

On 05/14/2017 08:32 AM, Jon Mason wrote:
> On Tue, May 09, 2017 at 09:33:28AM -0500, Gary R Hook wrote:
>> The Debug FS entries manage themselves; we don't need to hang onto
>> them in the context structure.
>>
>> Signed-off-by: Gary R Hook <gary.hook@amd.com>
>> ---

<snip>

>> +	debugfs_threads = debugfs_create_u8("threads", S_IRUSR | S_IWUSR,
>> +					    debugfs_node_dir,
>> +					    &perf->perf_threads);
>> +	if (!debugfs_threads)
>>  		return -ENODEV;
>
> I'm going to apply this, since it isn't that different then what
> exists now.  However, I would like to see this have an error path that
> unrolls the creation if one of them fails.

Yes, that would make sense. Will do.


-- 
Man, I love eighth notes.

[PATCH V3 3/3] ntb: Add more debugfs support for ntb_perf testing options

[Gary R Hook]

The ntb_perf tool uses module parameters to control the
characteristics of its test.  Enable the changing of these
options through debugfs, and eliminating the need to unload
and reload the module to make changes and run additional tests.

Add a new module parameter that forces the DMA channel
selection onto the same node as the NTB device (default: true).

 - seg_order: Size of the NTB memory window; power of 2.
 - run_order: Size of the data buffer; power of 2.
 - use_dma:   Use DMA or memcpy? Default: 0.
 - on_node:   Only use DMA channel(s) on the NTB node. Default: true.


Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
 drivers/ntb/test/ntb_perf.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
index 60e0dd320aef..6fdddf86269e 100644
--- a/drivers/ntb/test/ntb_perf.c
+++ b/drivers/ntb/test/ntb_perf.c
@@ -737,6 +737,10 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
 	struct dentry *debugfs_node_dir;
 	struct dentry *debugfs_run;
 	struct dentry *debugfs_threads;
+	struct dentry *debugfs_seg_order;
+	struct dentry *debugfs_run_order;
+	struct dentry *debugfs_use_dma;
+	struct dentry *debugfs_on_node;
 
 	if (!debugfs_initialized())
 		return -ENODEV;
@@ -764,6 +768,30 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
 	if (!debugfs_threads)
 		return -ENODEV;
 
+	debugfs_seg_order = debugfs_create_u32("seg_order", 0600,
+					       debugfs_node_dir,
+					       &seg_order);
+	if (!debugfs_seg_order)
+		return -ENODEV;
+
+	debugfs_run_order = debugfs_create_u32("run_order", 0600,
+					       debugfs_node_dir,
+					       &run_order);
+	if (!debugfs_run_order)
+		return -ENODEV;
+
+	debugfs_use_dma = debugfs_create_bool("use_dma", 0600,
+					       debugfs_node_dir,
+					       &use_dma);
+	if (!debugfs_use_dma)
+		return -ENODEV;
+
+	debugfs_on_node = debugfs_create_bool("on_node", 0600,
+					      debugfs_node_dir,
+					      &on_node);
+	if (!debugfs_on_node)
+		return -ENODEV;
+
 	return 0;
 }
 

Re: [PATCH V3 3/3] ntb: Add more debugfs support for ntb_perf testing options

[Jon Mason]

On Tue, May 09, 2017 at 09:33:36AM -0500, Gary R Hook wrote:
> The ntb_perf tool uses module parameters to control the
> characteristics of its test.  Enable the changing of these
> options through debugfs, and eliminating the need to unload
> and reload the module to make changes and run additional tests.
> 
> Add a new module parameter that forces the DMA channel
> selection onto the same node as the NTB device (default: true).
> 
>  - seg_order: Size of the NTB memory window; power of 2.
>  - run_order: Size of the data buffer; power of 2.
>  - use_dma:   Use DMA or memcpy? Default: 0.
>  - on_node:   Only use DMA channel(s) on the NTB node. Default: true.
> 
> 
> Signed-off-by: Gary R Hook <gary.hook@amd.com>
> ---
>  drivers/ntb/test/ntb_perf.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> index 60e0dd320aef..6fdddf86269e 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -737,6 +737,10 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
>  	struct dentry *debugfs_node_dir;
>  	struct dentry *debugfs_run;
>  	struct dentry *debugfs_threads;
> +	struct dentry *debugfs_seg_order;
> +	struct dentry *debugfs_run_order;
> +	struct dentry *debugfs_use_dma;
> +	struct dentry *debugfs_on_node;
>  
>  	if (!debugfs_initialized())
>  		return -ENODEV;
> @@ -764,6 +768,30 @@ static int perf_debugfs_setup(struct perf_ctx *perf)
>  	if (!debugfs_threads)
>  		return -ENODEV;
>  
> +	debugfs_seg_order = debugfs_create_u32("seg_order", 0600,
> +					       debugfs_node_dir,
> +					       &seg_order);
> +	if (!debugfs_seg_order)
> +		return -ENODEV;
> +
> +	debugfs_run_order = debugfs_create_u32("run_order", 0600,
> +					       debugfs_node_dir,
> +					       &run_order);
> +	if (!debugfs_run_order)
> +		return -ENODEV;
> +
> +	debugfs_use_dma = debugfs_create_bool("use_dma", 0600,
> +					       debugfs_node_dir,
> +					       &use_dma);
> +	if (!debugfs_use_dma)
> +		return -ENODEV;
> +
> +	debugfs_on_node = debugfs_create_bool("on_node", 0600,
> +					      debugfs_node_dir,
> +					      &on_node);
> +	if (!debugfs_on_node)
> +		return -ENODEV;
> +

Similar to my other comment, I would like to see an error path that
unrolls the creation of these on error.  It is okay to do this in a
follow-on patch.

Thanks,
Jon

>  	return 0;
>  }
>  
> 

Re: [PATCH V3 0/3] Add/expose control features for the NTB perf test module

[Dave Jiang]

On 05/09/2017 07:33 AM, Gary R Hook wrote:
> The goal of this series is to add and expose some control mechanisms for the
> performance test module:
> 
>  - Provide a switch to allow memory and threads to be anywhere, as
>    opposed to only the NTB node.
>  - The debug filesystem entries do not need to be retained after they
>    are set up. Remove the variables from the context structure and make
>    them automatic,
>  - Create a debug-fs entry for every module parameter. This allows test
>    parameters to be changed, and additional tests to be run, without
>    unloading the ntb_perf module each time.
> 
> Changes since V2:
>  - Use on_node to decide if -anything- is required to be on the NTB node.
> 
> Changes since V1:
>  - Split patch into appropriate pieces.
> 
> ---
> 
> Gary R Hook (3):
>       ntb: Add a module option to control affinity of DMA channels
>       ntb: Remove debug-fs variables from the context structure
>       ntb: Add more debugfs support for ntb_perf testing options
> 
> 
>  drivers/ntb/test/ntb_perf.c |   72 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 18 deletions(-)
> 

ack the series

Acked-by: Dave Jiang <dave.jiang@intel.com>