[PATCH] app/dma-perf: calrify incorrect NUMA config

[PATCH] app/dma-perf: calrify incorrect NUMA config

[Vipin Varghese]

Current commit decalres either `source or destination numa is greater
than acture numa` as cause of error. Rephrase as `Source or Destination`
is incorrect numa by checking which is greater than available numa.

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---
 app/test-dma-perf/benchmark.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index 9b1f58c78c..5634ff8bf8 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -313,7 +313,8 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
 	nr_sockets = rte_socket_count();
 	if (cfg->src_numa_node >= nr_sockets ||
 		cfg->dst_numa_node >= nr_sockets) {
-		printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
+		printf("Error: %s numa exceeds the available numa nodes.\n",
+				(cfg->src_numa_node >= nr_sockets) ? "Source" : "Destination");
 		return -1;
 	}
 
-- 
2.40.1

Re: [PATCH] app/dma-perf: calrify incorrect NUMA config

[fengchengwen]

Hi Vipin,

On 2024/3/6 23:02, Vipin Varghese wrote:
> Current commit decalres either `source or destination numa is greater
> than acture numa` as cause of error. Rephrase as `Source or Destination`
> is incorrect numa by checking which is greater than available numa.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> ---
>  app/test-dma-perf/benchmark.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
> index 9b1f58c78c..5634ff8bf8 100644
> --- a/app/test-dma-perf/benchmark.c
> +++ b/app/test-dma-perf/benchmark.c
> @@ -313,7 +313,8 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
>  	nr_sockets = rte_socket_count();
>  	if (cfg->src_numa_node >= nr_sockets ||
>  		cfg->dst_numa_node >= nr_sockets) {
> -		printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
> +		printf("Error: %s numa exceeds the available numa nodes.\n",
> +				(cfg->src_numa_node >= nr_sockets) ? "Source" : "Destination");

There are three cases to invoking:
1) src_numa_node >= nr_sockets
2) dst_numa_node >= nr_sockets
3) both src_numa_node and dst_numa_node >= nr_sockets

It could cover cases 1&2 in your commit, but could not cover case 3.

So I think we should keep original implement.

Thanks

>  		return -1;
>  	}
>  
> 

Re: [PATCH] app/dma-perf: calrify incorrect NUMA config

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 1423 bytes --]


<snipped>

>> -             printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
>> +             printf("Error: %s numa exceeds the available numa nodes.\n",
>> +                             (cfg->src_numa_node >= nr_sockets) ? "Source" : "Destination");

Thank you for comments, please find my reasoning as `change is added to 
address spelling issue of acture numa to available numa`

> There are three cases to invoking:
> 1) src_numa_node >= nr_sockets
> 2) dst_numa_node >= nr_sockets
> 3) both src_numa_node and dst_numa_node >= nr_sockets

As per my testing, following scenarios are explored

1) if src-numa is incorrect, we get error message as `source is 
incorrect numa`

2) if dst-numa is incorrect, we get error message as `destination is 
incorrect numa`

3) if both src-numa and dst-numa are incorrect, we get error as `source 
is incorrect numa`, fixing source and rerunning `destination is 
incorrect numa` (which is expected)

>
> It could cover cases 1&2 in your commit, but could not cover case 3.
I am happy to make changes to reflect the third scenario also. But 
please note as shared, the real intention is to fix ` acture numa` to 
something meaningful.
> So I think we should keep original implement.

I humbly disagree, based on the explanation as shared above. I can share 
v2 patch to address

1. acture numa

2. soruce or destination or src & destination


<snipped>

[-- Attachment #2: Type: text/html, Size: 2605 bytes --]

[PATCH v2] app/dma-perf: calrify incorrect NUMA config

[Vipin Varghese]

In case incorrect NUMA configuration, the current commit shares
 1) either `source or destination numa is greater`
 2) instead of `actual NUMA` it is `acture NUMA`

Current changes helps to rectify the same by using `PRINT_ERR` instead
of printf.

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>

Changes:
 - inform incorrect numa
 - fix spelling from acture to actual
 - use PRINT_ERR instead of printf

---
---
 app/test-dma-perf/benchmark.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index 9b1f58c78c..b6d0dbe4c0 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
 	uint32_t nr_buf = cfg->nr_buf;
 
 	nr_sockets = rte_socket_count();
-	if (cfg->src_numa_node >= nr_sockets ||
-		cfg->dst_numa_node >= nr_sockets) {
-		printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
+
+	bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
+	bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);
+
+	if (isSrcNumaIncorrect || isDstNumaIncorrect) {
+		PRINT_ERR("Error: NUMA config exceeds the actual numa nodes for %s.\n",
+			(isSrcNumaIncorrect && isDstNumaIncorrect) ? "Source & Destination" :
+				(isSrcNumaIncorrect) ? "Source" : "Destination");
 		return -1;
 	}
 
-- 
2.39.3

Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

[fengchengwen]

Hi Vipin,

On 2024/3/11 14:00, Vipin Varghese wrote:
> In case incorrect NUMA configuration, the current commit shares
>  1) either `source or destination numa is greater`
>  2) instead of `actual NUMA` it is `acture NUMA`
> 
> Current changes helps to rectify the same by using `PRINT_ERR` instead
> of printf.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
> 
> Changes:
>  - inform incorrect numa
>  - fix spelling from acture to actual
>  - use PRINT_ERR instead of printf
> 
> ---
> ---
>  app/test-dma-perf/benchmark.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
> index 9b1f58c78c..b6d0dbe4c0 100644
> --- a/app/test-dma-perf/benchmark.c
> +++ b/app/test-dma-perf/benchmark.c
> @@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
>  	uint32_t nr_buf = cfg->nr_buf;
>  
>  	nr_sockets = rte_socket_count();
> -	if (cfg->src_numa_node >= nr_sockets ||
> -		cfg->dst_numa_node >= nr_sockets) {
> -		printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
> +
> +	bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
> +	bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);

The naming style needs to be adjusted, how about
bool is_src_numa_exceed, is_dst_numa_exceed;

And predefine the variable at the beginning of function, sort by length, some like:

	bool is_src_numa_exceed, is_dst_numa_exceed;
	unsigned int buf_size = cfg->buf_size.cur;
	uint32_t nr_buf = cfg->nr_buf;
	unsigned int nr_sockets;

	nr_sockets = rte_socket_count();
	is_src_numa_exceed =
	is_dst_numa_exceed =
	if (xxx)
		...

> +
> +	if (isSrcNumaIncorrect || isDstNumaIncorrect) {
> +		PRINT_ERR("Error: NUMA config exceeds the actual numa nodes for %s.\n",
> +			(isSrcNumaIncorrect && isDstNumaIncorrect) ? "Source & Destination" :
> +				(isSrcNumaIncorrect) ? "Source" : "Destination");

Please don't capitalize the first letter of "Source" and "Destination"

Thanks

>  		return -1;
>  	}
>  
> 

Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

<snipped>
>> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
>> index 9b1f58c78c..b6d0dbe4c0 100644
>> --- a/app/test-dma-perf/benchmark.c
>> +++ b/app/test-dma-perf/benchmark.c
>> @@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
>>        uint32_t nr_buf = cfg->nr_buf;
>>
>>        nr_sockets = rte_socket_count();
>> -     if (cfg->src_numa_node >= nr_sockets ||
>> -             cfg->dst_numa_node >= nr_sockets) {
>> -             printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
>> +
>> +     bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
>> +     bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);
> The naming style needs to be adjusted, how about
> bool is_src_numa_exceed, is_dst_numa_exceed;

Ok, the naming convention used by me is `CamelCase`. One suggested from 
your end is `snake_case`.

Does DPDK has a constrain it can not use CamelCase.

>
> And predefine the variable at the beginning of function, sort by length, some like:
>
>          bool is_src_numa_exceed, is_dst_numa_exceed;
>          unsigned int buf_size = cfg->buf_size.cur;
>          uint32_t nr_buf = cfg->nr_buf;
>          unsigned int nr_sockets;
>
>          nr_sockets = rte_socket_count();
>          is_src_numa_exceed =
>          is_dst_numa_exceed =
>          if (xxx)
>                  ...
>
>> +
>> +     if (isSrcNumaIncorrect || isDstNumaIncorrect) {
>> +             PRINT_ERR("Error: NUMA config exceeds the actual numa nodes for %s.\n",
>> +                     (isSrcNumaIncorrect && isDstNumaIncorrect) ? "Source & Destination" :
>> +                             (isSrcNumaIncorrect) ? "Source" : "Destination");
> Please don't capitalize the first letter of "Source" and "Destination"

Can you please explain why?


>
> Thanks
>
>>                return -1;
>>        }
>>
>>

[-- Attachment #2: Type: text/html, Size: 2980 bytes --]

RE: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

[Konstantin Ananyev]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

>Ok, the naming convention used by me is `CamelCase`. One suggested from your end is `snake_case`.

>Does DPDK has a constrain it can not use CamelCase.



Please refer to:

https://doc.dpdk.org/guides/contributing/coding_style.html

In particular:
1.5.4. Variable Declarations
In declarations, do not put any whitespace between asterisks and adjacent tokens, except for tokens that are identifiers related to types. (These identifiers are the names of basic types, type qualifiers, and typedef-names other than the one being declared.) Separate these identifiers from asterisks using a single space.
For example:
int *x;         /* no space after asterisk */
int * const x;  /* space after asterisk when using a type qualifier */
·         All externally-visible variables should have an rte_ prefix in the name to avoid namespace collisions.
·         Do not use uppercase letters - either in the form of ALL_UPPERCASE, or CamelCase - in variable names. Lower-case letters and underscores only.






From: Varghese, Vipin <vipin.varghese@amd.com>
Sent: Tuesday, March 12, 2024 3:48 AM
To: Fengchengwen <fengchengwen@huawei.com>; dev@dpdk.org
Cc: ferruh.yigit@amd.com; neerav.parikh@amd.com
Subject: Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

<snipped>

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c

index 9b1f58c78c..b6d0dbe4c0 100644

--- a/app/test-dma-perf/benchmark.c

+++ b/app/test-dma-perf/benchmark.c

@@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,

      uint32_t nr_buf = cfg->nr_buf;



      nr_sockets = rte_socket_count();

-     if (cfg->src_numa_node >= nr_sockets ||

-             cfg->dst_numa_node >= nr_sockets) {

-             printf("Error: Source or destination numa exceeds the acture numa nodes.\n");

+

+     bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);

+     bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);



The naming style needs to be adjusted, how about

bool is_src_numa_exceed, is_dst_numa_exceed;

Ok, the naming convention used by me is `CamelCase`. One suggested from your end is `snake_case`.

Does DPDK has a constrain it can not use CamelCase.





And predefine the variable at the beginning of function, sort by length, some like:



        bool is_src_numa_exceed, is_dst_numa_exceed;

        unsigned int buf_size = cfg->buf_size.cur;

        uint32_t nr_buf = cfg->nr_buf;

        unsigned int nr_sockets;



        nr_sockets = rte_socket_count();

        is_src_numa_exceed =

        is_dst_numa_exceed =

        if (xxx)

                ...



+

+     if (isSrcNumaIncorrect || isDstNumaIncorrect) {

+             PRINT_ERR("Error: NUMA config exceeds the actual numa nodes for %s.\n",

+                     (isSrcNumaIncorrect && isDstNumaIncorrect) ? "Source & Destination" :

+                             (isSrcNumaIncorrect) ? "Source" : "Destination");



Please don't capitalize the first letter of "Source" and "Destination"

Can you please explain why?







Thanks



              return -1;

      }





[-- Attachment #2: Type: text/html, Size: 14653 bytes --]

Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config

[Varghese, Vipin]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

Thank you Konstantin for the reply, Adding back the comments as it is 
not reflected the mail thread

<snipped>

>>> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
>>> index 9b1f58c78c..b6d0dbe4c0 100644
>>> --- a/app/test-dma-perf/benchmark.c
>>> +++ b/app/test-dma-perf/benchmark.c
>>> @@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
>>>        uint32_t nr_buf = cfg->nr_buf;
>>>
>>>        nr_sockets = rte_socket_count();
>>> -     if (cfg->src_numa_node >= nr_sockets ||
>>> -             cfg->dst_numa_node >= nr_sockets) {
>>> -             printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
>>> +
>>> +     bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
>>> +     bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);
>> The naming style needs to be adjusted, how about
>> bool is_src_numa_exceed, is_dst_numa_exceed;
>
> Ok, the naming convention used by me is `CamelCase`. One suggested 
> from your end is `snake_case`.
>
> Does DPDK has a constrain it can not use CamelCase.
>
[KA]

Please refer to:

https://doc.dpdk.org/guides/contributing/coding_style.html

In particular:
1.5.4. Variable Declarations
In declarations, do not put any whitespace between asterisks and adjacent tokens, except for tokens that are identifiers related to types. (These identifiers are the names of basic types, type qualifiers, and typedef-names other than the one being declared.) Separate these identifiers from asterisks using a single space.
For example:
int *x;         /* no space after asterisk */
int * const x;  /* space after asterisk when using a type qualifier */
·         All externally-visible variables should have an rte_ prefix in the name to avoid namespace collisions.
·         Do not use uppercase letters - either in the form of ALL_UPPERCASE, or CamelCase - in variable names. Lower-case letters and underscores only.

[VV] Thank you for the clarification, I mistook this is applicable only to `All externally-visible variables`and not for `static` functions.

> <snipped>

[-- Attachment #2: Type: text/html, Size: 3999 bytes --]

[PATCH v3] app/dma-perf: calrify incorrect NUMA config

[Vipin Varghese]

In case incorrect NUMA configuration, the current commit shares
 1) either `source or destination numa is greater`
 2) instead of `actual NUMA` it is `acture NUMA`
 3) uses `printf` instead of PRINT_ERR

current patch changes the above to
 1) identify if source or|and destination is incorrect
 2) fix wording to incorrect
 3) use PRINT_ERR macro

Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>
---

V3 changes:
 - use snake-case instead of camel case for static scope functions.
 - convert console words to lower case.

V2 changes:
 - inform incorrect numa
 - fix spelling from acture to actual
 - use PRINT_ERR instead of printf
---
 app/test-dma-perf/benchmark.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
index d167adc4d2..a437b715bd 100644
--- a/app/test-dma-perf/benchmark.c
+++ b/app/test-dma-perf/benchmark.c
@@ -442,11 +442,16 @@ setup_memory_env(struct test_configure *cfg,
 	unsigned int nr_sockets;
 	uint32_t nr_buf = cfg->nr_buf;
 	uint32_t i;
+	bool is_src_numa_incorrect, is_dst_numa_incorrect;
 
 	nr_sockets = rte_socket_count();
-	if (cfg->src_numa_node >= nr_sockets ||
-		cfg->dst_numa_node >= nr_sockets) {
-		printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
+	is_src_numa_incorrect = (cfg->src_numa_node >= nr_sockets);
+	is_dst_numa_incorrect = (cfg->dst_numa_node >= nr_sockets);
+
+	if (is_src_numa_incorrect || is_dst_numa_incorrect) {
+		PRINT_ERR("Error: Incorrect NUMA config for %s.\n",
+			(is_src_numa_incorrect && is_dst_numa_incorrect) ? "source & destination" :
+				(is_src_numa_incorrect) ? "source" : "destination");
 		return -1;
 	}
 
-- 
2.39.3

Re: [PATCH v3] app/dma-perf: calrify incorrect NUMA config

[Thomas Monjalon]

20/03/2024 02:40, Vipin Varghese:
> In case incorrect NUMA configuration, the current commit shares
>  1) either `source or destination numa is greater`
>  2) instead of `actual NUMA` it is `acture NUMA`
>  3) uses `printf` instead of PRINT_ERR
> 
> current patch changes the above to
>  1) identify if source or|and destination is incorrect
>  2) fix wording to incorrect
>  3) use PRINT_ERR macro

This is waiting for review.

RE: [EXTERNAL] [PATCH v3] app/dma-perf: calrify incorrect NUMA config

[Gowrishankar Muthukrishnan]

> Subject: [EXTERNAL] [PATCH v3] app/dma-perf: calrify incorrect NUMA config

Looks good to me, with minor correction in subject:  calrify  -> clarify.

Thanks,
Gowrishankar

Re: [PATCH v3] app/dma-perf: calrify incorrect NUMA config

[Thomas Monjalon]

20/03/2024 02:40, Vipin Varghese:
> In case incorrect NUMA configuration, the current commit shares
>  1) either `source or destination numa is greater`
>  2) instead of `actual NUMA` it is `acture NUMA`
>  3) uses `printf` instead of PRINT_ERR
> 
> current patch changes the above to
>  1) identify if source or|and destination is incorrect
>  2) fix wording to incorrect
>  3) use PRINT_ERR macro
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@amd.com>

Applied, thanks.