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

  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(-)
  

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;
>  	}
>  
>
  

<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;
>>        }
>>
>>
  

>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;

      }
  

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>