[PATCH] app/testpmd: fix lcore ID restriction

[PATCH] app/testpmd: fix lcore ID restriction

[Sivaprasad Tummala]

With modern CPUs, it is possible to have higher
CPU count thus we can have higher RTE_MAX_LCORES.
In testpmd application, the current config forwarding
cores option "--nb-cores" is hard limited to 255.

The patch fixes this constraint and also adjusts the lcore
data structure to 32-bit to align with rte lcore APIs.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
 app/test-pmd/parameters.c | 2 +-
 app/test-pmd/testpmd.h    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index c13f7564bf..6d2bf9b7c1 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1072,7 +1072,7 @@ launch_args_parse(int argc, char** argv)
 		case TESTPMD_OPT_NB_CORES_NUM:
 			n = atoi(optarg);
 			if (n > 0 && n <= nb_lcores)
-				nb_fwd_lcores = (uint8_t) n;
+				nb_fwd_lcores = (lcoreid_t) n;
 			else
 				rte_exit(EXIT_FAILURE,
 					"nb-cores should be > 0 and <= %d\n",
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 0afae7d771..9facd7f281 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -84,7 +84,7 @@ extern volatile uint8_t f_quit;
 /* Maximum number of pools supported per Rx queue */
 #define MAX_MEMPOOL 8
 
-typedef uint8_t  lcoreid_t;
+typedef uint32_t lcoreid_t;
 typedef uint16_t portid_t;
 typedef uint16_t queueid_t;
 typedef uint16_t streamid_t;
-- 
2.40.1

Re: [PATCH] app/testpmd: fix lcore ID restriction

[Stephen Hemminger]

On Mon, 15 Apr 2024 19:46:31 +0000
Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:

> +				nb_fwd_lcores = (lcoreid_t) n;

This should really be using strtoul() not atoi() because it offers
more error checking.

Re: [PATCH] app/testpmd: fix lcore ID restriction

[Stephen Hemminger]

On Mon, 15 Apr 2024 19:46:31 +0000
Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:

> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -84,7 +84,7 @@ extern volatile uint8_t f_quit;
>  /* Maximum number of pools supported per Rx queue */
>  #define MAX_MEMPOOL 8
>  
> -typedef uint8_t  lcoreid_t;
> +typedef uint32_t lcoreid_t;
>  typedef uint16_t portid_t;
>  typedef uint16_t queueid_t;
>  typedef uint16_t streamid_t;
> -- 

The other DPDK API's are using unsigned for lcore_id.
On the platforms that DPDK supports both are the same size.

[PATCH v2] app/testpmd: fix lcore ID restriction

[Sivaprasad Tummala]

With modern CPUs, it is possible to have higher
CPU count thus we can have higher RTE_MAX_LCORES.
In testpmd application, the current config forwarding
cores option "--nb-cores" is hard limited to 255.

The patch fixes this constraint and also adjusts the lcore
data structure to 32-bit to align with rte lcore APIs.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
v2:
* Fixed type mistmatch in comparison 
* Fixed incorrect format specifier
---
 app/test-pmd/config.c     | 6 +++---
 app/test-pmd/parameters.c | 4 ++--
 app/test-pmd/testpmd.h    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ba1007ace6..6b28c22c96 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4785,9 +4785,9 @@ fwd_stream_on_other_lcores(uint16_t domain_id, lcoreid_t src_lc,
 				continue;
 			printf("Shared Rx queue group %u queue %hu can't be scheduled on different cores:\n",
 			       share_group, share_rxq);
-			printf("  lcore %hhu Port %hu queue %hu\n",
+			printf("  lcore %u Port %hu queue %hu\n",
 			       src_lc, src_port, src_rxq);
-			printf("  lcore %hhu Port %hu queue %hu\n",
+			printf("  lcore %u Port %hu queue %hu\n",
 			       lc_id, fs->rx_port, fs->rx_queue);
 			printf("Please use --nb-cores=%hu to limit number of forwarding cores\n",
 			       nb_rxq);
@@ -5159,7 +5159,7 @@ icmp_echo_config_setup(void)
 	lcoreid_t lc_id;
 	uint16_t  sm_id;
 
-	if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
+	if ((lcoreid_t)(nb_txq * nb_fwd_ports) < nb_fwd_lcores)
 		cur_fwd_config.nb_fwd_lcores = (lcoreid_t)
 			(nb_txq * nb_fwd_ports);
 	else
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index c13f7564bf..22364e09ab 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1071,8 +1071,8 @@ launch_args_parse(int argc, char** argv)
 			break;
 		case TESTPMD_OPT_NB_CORES_NUM:
 			n = atoi(optarg);
-			if (n > 0 && n <= nb_lcores)
-				nb_fwd_lcores = (uint8_t) n;
+			if (n > 0 && (lcoreid_t)n <= nb_lcores)
+				nb_fwd_lcores = (lcoreid_t) n;
 			else
 				rte_exit(EXIT_FAILURE,
 					"nb-cores should be > 0 and <= %d\n",
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 0afae7d771..9facd7f281 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -84,7 +84,7 @@ extern volatile uint8_t f_quit;
 /* Maximum number of pools supported per Rx queue */
 #define MAX_MEMPOOL 8
 
-typedef uint8_t  lcoreid_t;
+typedef uint32_t lcoreid_t;
 typedef uint16_t portid_t;
 typedef uint16_t queueid_t;
 typedef uint16_t streamid_t;
-- 
2.34.1

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 4/16/2024 10:55 AM, Sivaprasad Tummala wrote:
> With modern CPUs, it is possible to have higher
> CPU count thus we can have higher RTE_MAX_LCORES.
> In testpmd application, the current config forwarding
> cores option "--nb-cores" is hard limited to 255.
> 
> The patch fixes this constraint and also adjusts the lcore
> data structure to 32-bit to align with rte lcore APIs.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>

Recheck-request: iol-unit-amd64-testing

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 4/16/2024 10:55 AM, Sivaprasad Tummala wrote:
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index ba1007ace6..6b28c22c96 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4785,9 +4785,9 @@ fwd_stream_on_other_lcores(uint16_t domain_id, lcoreid_t src_lc,
>  				continue;
>  			printf("Shared Rx queue group %u queue %hu can't be scheduled on different cores:\n",
>  			       share_group, share_rxq);
> -			printf("  lcore %hhu Port %hu queue %hu\n",
> +			printf("  lcore %u Port %hu queue %hu\n",
>  			       src_lc, src_port, src_rxq);
> -			printf("  lcore %hhu Port %hu queue %hu\n",
> +			printf("  lcore %u Port %hu queue %hu\n",
>  			       lc_id, fs->rx_port, fs->rx_queue);
>  			printf("Please use --nb-cores=%hu to limit number of forwarding cores\n",
>  			       nb_rxq);
> @@ -5159,7 +5159,7 @@ icmp_echo_config_setup(void)
>  	lcoreid_t lc_id;
>  	uint16_t  sm_id;
>  
> -	if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
> +	if ((lcoreid_t)(nb_txq * nb_fwd_ports) < nb_fwd_lcores)
>  		cur_fwd_config.nb_fwd_lcores = (lcoreid_t)
>  			(nb_txq * nb_fwd_ports);
>

Hi Sivaprasad,

Is this '(lcoreid_t)' cast required? Because of integer promotion I
think result will be correct without casting.

(And without integer promotion considered, casting needs to be done on
one of the variables, not to the result, because result may be already
cast down I think. Anyway this is not required for this case since
variables are u16.)

RE: [PATCH v2] app/testpmd: fix lcore ID restriction

[Tummala, Sivaprasad]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh <Ferruh.Yigit@amd.com>
> Sent: Friday, April 19, 2024 5:00 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>;
> david.marchand@redhat.com; aman.deep.singh@intel.com;
> yuying.zhang@intel.com
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2] app/testpmd: fix lcore ID restriction
>
> On 4/16/2024 10:55 AM, Sivaprasad Tummala wrote:
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > ba1007ace6..6b28c22c96 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -4785,9 +4785,9 @@ fwd_stream_on_other_lcores(uint16_t domain_id,
> lcoreid_t src_lc,
> >                             continue;
> >                     printf("Shared Rx queue group %u queue %hu can't be
> scheduled on different cores:\n",
> >                            share_group, share_rxq);
> > -                   printf("  lcore %hhu Port %hu queue %hu\n",
> > +                   printf("  lcore %u Port %hu queue %hu\n",
> >                            src_lc, src_port, src_rxq);
> > -                   printf("  lcore %hhu Port %hu queue %hu\n",
> > +                   printf("  lcore %u Port %hu queue %hu\n",
> >                            lc_id, fs->rx_port, fs->rx_queue);
> >                     printf("Please use --nb-cores=%hu to limit number of
> forwarding cores\n",
> >                            nb_rxq);
> > @@ -5159,7 +5159,7 @@ icmp_echo_config_setup(void)
> >     lcoreid_t lc_id;
> >     uint16_t  sm_id;
> >
> > -   if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
> > +   if ((lcoreid_t)(nb_txq * nb_fwd_ports) < nb_fwd_lcores)
> >             cur_fwd_config.nb_fwd_lcores = (lcoreid_t)
> >                     (nb_txq * nb_fwd_ports);
> >
>
> Hi Sivaprasad,
>
> Is this '(lcoreid_t)' cast required? Because of integer promotion I think result will
> be correct without casting.
>
> (And without integer promotion considered, casting needs to be done on one of
> the variables, not to the result, because result may be already cast down I think.
> Anyway this is not required for this case since variables are u16.)
Agreed., the change was added to address a compilation issue on Centos7. It seems
The compiler was reporting a false alarm and I will revert this change in the next patch.

[PATCH v3] app/testpmd: fix lcore ID restriction

[Sivaprasad Tummala]

With modern CPUs, it is possible to have higher
CPU count thus we can have higher RTE_MAX_LCORES.
In testpmd application, the current config forwarding
cores option "--nb-cores" is hard limited to 255.

The patch fixes this constraint and also adjusts the lcore
data structure to 32-bit to align with rte lcore APIs.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
v3:
* Removed unnecessary cast

v2:
* Fixed type mistmatch in comparison
* Fixed incorrect format specifier
---
 app/test-pmd/config.c     | 4 ++--
 app/test-pmd/parameters.c | 4 ++--
 app/test-pmd/testpmd.h    | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ba1007ace6..a0c8bb8fe1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4785,9 +4785,9 @@ fwd_stream_on_other_lcores(uint16_t domain_id, lcoreid_t src_lc,
 				continue;
 			printf("Shared Rx queue group %u queue %hu can't be scheduled on different cores:\n",
 			       share_group, share_rxq);
-			printf("  lcore %hhu Port %hu queue %hu\n",
+			printf("  lcore %u Port %hu queue %hu\n",
 			       src_lc, src_port, src_rxq);
-			printf("  lcore %hhu Port %hu queue %hu\n",
+			printf("  lcore %u Port %hu queue %hu\n",
 			       lc_id, fs->rx_port, fs->rx_queue);
 			printf("Please use --nb-cores=%hu to limit number of forwarding cores\n",
 			       nb_rxq);
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index c13f7564bf..22364e09ab 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -1071,8 +1071,8 @@ launch_args_parse(int argc, char** argv)
 			break;
 		case TESTPMD_OPT_NB_CORES_NUM:
 			n = atoi(optarg);
-			if (n > 0 && n <= nb_lcores)
-				nb_fwd_lcores = (uint8_t) n;
+			if (n > 0 && (lcoreid_t)n <= nb_lcores)
+				nb_fwd_lcores = (lcoreid_t) n;
 			else
 				rte_exit(EXIT_FAILURE,
 					"nb-cores should be > 0 and <= %d\n",
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 0afae7d771..9facd7f281 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -84,7 +84,7 @@ extern volatile uint8_t f_quit;
 /* Maximum number of pools supported per Rx queue */
 #define MAX_MEMPOOL 8
 
-typedef uint8_t  lcoreid_t;
+typedef uint32_t lcoreid_t;
 typedef uint16_t portid_t;
 typedef uint16_t queueid_t;
 typedef uint16_t streamid_t;
-- 
2.34.1

Re: [PATCH v3] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 6/6/2024 12:27 PM, Sivaprasad Tummala wrote:
> With modern CPUs, it is possible to have higher
> CPU count thus we can have higher RTE_MAX_LCORES.
> In testpmd application, the current config forwarding
> cores option "--nb-cores" is hard limited to 255.
> 
> The patch fixes this constraint and also adjusts the lcore
> data structure to 32-bit to align with rte lcore APIs.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v3] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 6/12/2024 12:37 AM, Ferruh Yigit wrote:
> On 6/6/2024 12:27 PM, Sivaprasad Tummala wrote:
>> With modern CPUs, it is possible to have higher
>> CPU count thus we can have higher RTE_MAX_LCORES.
>> In testpmd application, the current config forwarding
>> cores option "--nb-cores" is hard limited to 255.
>>
>> The patch fixes this constraint and also adjusts the lcore
>> data structure to 32-bit to align with rte lcore APIs.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> Applied to dpdk-next-net/main, thanks.
>

Raslan reported build error because of the casting I asked to remove :(
I will add it back in the next-net repo directly, meanwhile I will
comment on the patch for details.

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 4/19/2024 12:30 PM, Ferruh Yigit wrote:
> On 4/16/2024 10:55 AM, Sivaprasad Tummala wrote:
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index ba1007ace6..6b28c22c96 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -4785,9 +4785,9 @@ fwd_stream_on_other_lcores(uint16_t domain_id, lcoreid_t src_lc,
>>  				continue;
>>  			printf("Shared Rx queue group %u queue %hu can't be scheduled on different cores:\n",
>>  			       share_group, share_rxq);
>> -			printf("  lcore %hhu Port %hu queue %hu\n",
>> +			printf("  lcore %u Port %hu queue %hu\n",
>>  			       src_lc, src_port, src_rxq);
>> -			printf("  lcore %hhu Port %hu queue %hu\n",
>> +			printf("  lcore %u Port %hu queue %hu\n",
>>  			       lc_id, fs->rx_port, fs->rx_queue);
>>  			printf("Please use --nb-cores=%hu to limit number of forwarding cores\n",
>>  			       nb_rxq);
>> @@ -5159,7 +5159,7 @@ icmp_echo_config_setup(void)
>>  	lcoreid_t lc_id;
>>  	uint16_t  sm_id;
>>  
>> -	if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
>> +	if ((lcoreid_t)(nb_txq * nb_fwd_ports) < nb_fwd_lcores)
>>  		cur_fwd_config.nb_fwd_lcores = (lcoreid_t)
>>  			(nb_txq * nb_fwd_ports);
>>
> 
> Hi Sivaprasad,
> 
> Is this '(lcoreid_t)' cast required? Because of integer promotion I
> think result will be correct without casting.
> 
> (And without integer promotion considered, casting needs to be done on
> one of the variables, not to the result, because result may be already
> cast down I think. Anyway this is not required for this case since
> variables are u16.)
>

Why casing required (for record) is,
'nb_txq' -> uint16_t, promoted to 'int'
'nb_fwd_ports' -> uint16_t, promoted to 'int'
(nb_txq * nb_fwd_ports) -> result 'int'
nb_fwd_lcores  -> 'uint32_t'

comparison between 'int' & 'uint32_t' gives warning. After some compiler
version it is smart enough to not give a warning, but casting is
required for old compilers.

And back to my comment above, casting one of the parameter to
'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
instead. But logically casting looks odd, so keeping casting result.

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Stephen Hemminger]

On Thu, 13 Jun 2024 17:51:14 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> > Hi Sivaprasad,
> > 
> > Is this '(lcoreid_t)' cast required? Because of integer promotion I
> > think result will be correct without casting.
> > 
> > (And without integer promotion considered, casting needs to be done on
> > one of the variables, not to the result, because result may be already
> > cast down I think. Anyway this is not required for this case since
> > variables are u16.)
> >  
> 
> Why casing required (for record) is,
> 'nb_txq' -> uint16_t, promoted to 'int'
> 'nb_fwd_ports' -> uint16_t, promoted to 'int'
> (nb_txq * nb_fwd_ports) -> result 'int'
> nb_fwd_lcores  -> 'uint32_t'
> 
> comparison between 'int' & 'uint32_t' gives warning. After some compiler
> version it is smart enough to not give a warning, but casting is
> required for old compilers.
> 
> And back to my comment above, casting one of the parameter to
> 'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
> instead. But logically casting looks odd, so keeping casting result.

Where is the integer promotion happening?
Also would be better to use strtoul() instead of atoi() when parsing the args.
That gives better error handling and handles someone passing negative number correctly.

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 6/13/2024 8:13 PM, Stephen Hemminger wrote:
> On Thu, 13 Jun 2024 17:51:14 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>>> Hi Sivaprasad,
>>>
>>> Is this '(lcoreid_t)' cast required? Because of integer promotion I
>>> think result will be correct without casting.
>>>
>>> (And without integer promotion considered, casting needs to be done on
>>> one of the variables, not to the result, because result may be already
>>> cast down I think. Anyway this is not required for this case since
>>> variables are u16.)
>>>  
>>
>> Why casing required (for record) is,
>> 'nb_txq' -> uint16_t, promoted to 'int'
>> 'nb_fwd_ports' -> uint16_t, promoted to 'int'
>> (nb_txq * nb_fwd_ports) -> result 'int'
>> nb_fwd_lcores  -> 'uint32_t'
>>
>> comparison between 'int' & 'uint32_t' gives warning. After some compiler
>> version it is smart enough to not give a warning, but casting is
>> required for old compilers.
>>
>> And back to my comment above, casting one of the parameter to
>> 'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
>> instead. But logically casting looks odd, so keeping casting result.
> 
> Where is the integer promotion happening?
>

Raslan reported following compile error, this version of the patch has
the cast, but merged version, v3, doesn't.


```
../../root/dpdk/app/test-pmd/config.c: In function 'icmp_echo_config_setup':
../../root/dpdk/app/test-pmd/config.c:5159:30: error: comparison between
signed and unsigned integer expressions [-Werror=sign-compare]
  if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
                              ^
```

> Also would be better to use strtoul() instead of atoi() when parsing the args.
> That gives better error handling and handles someone passing negative number correctly.

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Stephen Hemminger]

On Fri, 14 Jun 2024 10:01:02 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 6/13/2024 8:13 PM, Stephen Hemminger wrote:
> > On Thu, 13 Jun 2024 17:51:14 +0100
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >   
> >>> Hi Sivaprasad,
> >>>
> >>> Is this '(lcoreid_t)' cast required? Because of integer promotion I
> >>> think result will be correct without casting.
> >>>
> >>> (And without integer promotion considered, casting needs to be done on
> >>> one of the variables, not to the result, because result may be already
> >>> cast down I think. Anyway this is not required for this case since
> >>> variables are u16.)
> >>>    
> >>
> >> Why casing required (for record) is,
> >> 'nb_txq' -> uint16_t, promoted to 'int'
> >> 'nb_fwd_ports' -> uint16_t, promoted to 'int'
> >> (nb_txq * nb_fwd_ports) -> result 'int'
> >> nb_fwd_lcores  -> 'uint32_t'
> >>
> >> comparison between 'int' & 'uint32_t' gives warning. After some compiler
> >> version it is smart enough to not give a warning, but casting is
> >> required for old compilers.
> >>
> >> And back to my comment above, casting one of the parameter to
> >> 'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
> >> instead. But logically casting looks odd, so keeping casting result.  
> > 
> > Where is the integer promotion happening?
> >  
> 
> Raslan reported following compile error, this version of the patch has
> the cast, but merged version, v3, doesn't.
> 
> 
> ```
> ../../root/dpdk/app/test-pmd/config.c: In function 'icmp_echo_config_setup':
> ../../root/dpdk/app/test-pmd/config.c:5159:30: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
>   if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)

That does look like a compiler bug. uint16 multiplied by uint16 should be uint16.
Not sure why DPDK keeps using small types like uint16 so much, doesn't have any
real benefit since all this is in registers.                             ^

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Ferruh Yigit]

On 6/14/2024 4:27 PM, Stephen Hemminger wrote:
> On Fri, 14 Jun 2024 10:01:02 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 6/13/2024 8:13 PM, Stephen Hemminger wrote:
>>> On Thu, 13 Jun 2024 17:51:14 +0100
>>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>   
>>>>> Hi Sivaprasad,
>>>>>
>>>>> Is this '(lcoreid_t)' cast required? Because of integer promotion I
>>>>> think result will be correct without casting.
>>>>>
>>>>> (And without integer promotion considered, casting needs to be done on
>>>>> one of the variables, not to the result, because result may be already
>>>>> cast down I think. Anyway this is not required for this case since
>>>>> variables are u16.)
>>>>>    
>>>>
>>>> Why casing required (for record) is,
>>>> 'nb_txq' -> uint16_t, promoted to 'int'
>>>> 'nb_fwd_ports' -> uint16_t, promoted to 'int'
>>>> (nb_txq * nb_fwd_ports) -> result 'int'
>>>> nb_fwd_lcores  -> 'uint32_t'
>>>>
>>>> comparison between 'int' & 'uint32_t' gives warning. After some compiler
>>>> version it is smart enough to not give a warning, but casting is
>>>> required for old compilers.
>>>>
>>>> And back to my comment above, casting one of the parameter to
>>>> 'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
>>>> instead. But logically casting looks odd, so keeping casting result.  
>>>
>>> Where is the integer promotion happening?
>>>  
>>
>> Raslan reported following compile error, this version of the patch has
>> the cast, but merged version, v3, doesn't.
>>
>>
>> ```
>> ../../root/dpdk/app/test-pmd/config.c: In function 'icmp_echo_config_setup':
>> ../../root/dpdk/app/test-pmd/config.c:5159:30: error: comparison between
>> signed and unsigned integer expressions [-Werror=sign-compare]
>>   if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
> 
> That does look like a compiler bug. uint16 multiplied by uint16 should be uint16.
>

Probably, this warning goes away with newer version of compiler.

> Not sure why DPDK keeps using small types like uint16 so much, doesn't have any
> real benefit since all this is in registers.                             ^

RE: [PATCH v2] app/testpmd: fix lcore ID restriction

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 14 June 2024 17.27
> 
> On Fri, 14 Jun 2024 10:01:02 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
> > On 6/13/2024 8:13 PM, Stephen Hemminger wrote:
> > > On Thu, 13 Jun 2024 17:51:14 +0100
> > > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > >
> > >>> Hi Sivaprasad,
> > >>>
> > >>> Is this '(lcoreid_t)' cast required? Because of integer promotion
> I
> > >>> think result will be correct without casting.
> > >>>
> > >>> (And without integer promotion considered, casting needs to be
> done on
> > >>> one of the variables, not to the result, because result may be
> already
> > >>> cast down I think. Anyway this is not required for this case since
> > >>> variables are u16.)
> > >>>
> > >>
> > >> Why casing required (for record) is,
> > >> 'nb_txq' -> uint16_t, promoted to 'int'
> > >> 'nb_fwd_ports' -> uint16_t, promoted to 'int'
> > >> (nb_txq * nb_fwd_ports) -> result 'int'
> > >> nb_fwd_lcores  -> 'uint32_t'
> > >>
> > >> comparison between 'int' & 'uint32_t' gives warning. After some
> compiler
> > >> version it is smart enough to not give a warning, but casting is
> > >> required for old compilers.
> > >>
> > >> And back to my comment above, casting one of the parameter to
> > >> 'lcoreid_t' also works, as it forcing promotion to 'unsigned int'
> > >> instead. But logically casting looks odd, so keeping casting
> result.
> > >
> > > Where is the integer promotion happening?
> > >
> >
> > Raslan reported following compile error, this version of the patch has
> > the cast, but merged version, v3, doesn't.
> >
> >
> > ```
> > ../../root/dpdk/app/test-pmd/config.c: In function
> 'icmp_echo_config_setup':
> > ../../root/dpdk/app/test-pmd/config.c:5159:30: error: comparison
> between
> > signed and unsigned integer expressions [-Werror=sign-compare]
> >   if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)
> 
> That does look like a compiler bug. uint16 multiplied by uint16 should
> be uint16.

Not, C doesn't promote types like that. Arithmetic operations always promote smaller types to int or unsigned int. And since uint16 fits in int, uint16 is promoted to int (not unsigned int).

> Not sure why DPDK keeps using small types like uint16 so much, doesn't
> have any
> real benefit since all this is in registers.

Re: [PATCH v2] app/testpmd: fix lcore ID restriction

[Stephen Hemminger]

On Fri, 14 Jun 2024 19:50:29 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > > ```
> > > ../../root/dpdk/app/test-pmd/config.c: In function  
> > 'icmp_echo_config_setup':  
> > > ../../root/dpdk/app/test-pmd/config.c:5159:30: error: comparison  
> > between  
> > > signed and unsigned integer expressions [-Werror=sign-compare]
> > >   if ((nb_txq * nb_fwd_ports) < nb_fwd_lcores)  
> > 
> > That does look like a compiler bug. uint16 multiplied by uint16 should
> > be uint16.  
> 
> Not, C doesn't promote types like that. Arithmetic operations always promote smaller types to int or unsigned int. And since uint16 fits in int, uint16 is promoted to int (not unsigned int).


You are right. Long winded explanation here:
https://stackoverflow.com/questions/46073295/implicit-type-promotion-rules