diff mbox series

[v3,5/6] examples/qos_sched: fix lcore ID restriction

Message ID 20231220064502.2830-6-sivaprasad.tummala@amd.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series fix lcore ID restriction |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Sivaprasad Tummala Dec. 20, 2023, 6:45 a.m. UTC 
  Currently the config option allows lcore IDs up to 255,
irrespective of RTE_MAX_LCORES and needs to be fixed.

The patch allows config options based on DPDK config.

Fixes: de3cfa2c9823 ("sched: initial import")
Cc: stable@dpdk.org

Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
 examples/qos_sched/args.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Stephen Hemminger Dec. 20, 2023, 4:31 p.m. UTC | #1 
On Wed, 20 Dec 2023 07:45:00 +0100
Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:

> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> index e97273152a..22fe76eeb5 100644
> --- a/examples/qos_sched/args.c
> +++ b/examples/qos_sched/args.c
> @@ -182,10 +182,10 @@ app_parse_flow_conf(const char *conf_str)
>  
>  	pconf->rx_port = vals[0];
>  	pconf->tx_port = vals[1];
> -	pconf->rx_core = (uint8_t)vals[2];
> -	pconf->wt_core = (uint8_t)vals[3];
> +	pconf->rx_core = (uint16_t)vals[2];
> +	pconf->wt_core = (uint16_t)vals[3];
>  	if (ret == 5)
> -		pconf->tx_core = (uint8_t)vals[4];
> +		pconf->tx_core = (uint16_t)vals[4];
>  	else
>  		pconf->tx_core = pconf->wt_core;
>  
> -- 

Not sure why cast is even needed, assigning uint32_t to uint16_t
is not going to generate a warning with current compiler settings.
Ferruh Yigit Jan. 9, 2024, 3:16 p.m. UTC | #2 
On 12/20/2023 4:31 PM, Stephen Hemminger wrote:
> On Wed, 20 Dec 2023 07:45:00 +0100
> Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:
> 
>> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
>> index e97273152a..22fe76eeb5 100644
>> --- a/examples/qos_sched/args.c
>> +++ b/examples/qos_sched/args.c
>> @@ -182,10 +182,10 @@ app_parse_flow_conf(const char *conf_str)
>>  
>>  	pconf->rx_port = vals[0];
>>  	pconf->tx_port = vals[1];
>> -	pconf->rx_core = (uint8_t)vals[2];
>> -	pconf->wt_core = (uint8_t)vals[3];
>> +	pconf->rx_core = (uint16_t)vals[2];
>> +	pconf->wt_core = (uint16_t)vals[3];
>>  	if (ret == 5)
>> -		pconf->tx_core = (uint8_t)vals[4];
>> +		pconf->tx_core = (uint16_t)vals[4];
>>  	else
>>  		pconf->tx_core = pconf->wt_core;
>>  
>> -- 
> 
> Not sure why cast is even needed, assigning uint32_t to uint16_t
> is not going to generate a warning with current compiler settings.
>

I was assuming compiler will complain when assigning uint32_t to
uint16_t, but it seems '-Wconversion' compiler flag is required for this
warning.
Enabling this flag for DPDK build causes lots of warnings, I wonder if
we should add a new buildtype in meson that enables this flag.


And except from compiler warning, I think it is good to keep explicit
cast where assignment can cause change of value. This at worst can work
as documentation that assignment between different types done intentionally.
Sivaprasad Tummala Jan. 16, 2024, 12:33 p.m. UTC | #3 
[AMD Official Use Only - General]

Hi Stephen,

> -----Original Message-----
> From: Yigit, Ferruh <Ferruh.Yigit@amd.com>
> Sent: Tuesday, January 9, 2024 8:47 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Tummala, Sivaprasad
> <Sivaprasad.Tummala@amd.com>
> Cc: david.hunt@intel.com; anatoly.burakov@intel.com; jerinj@marvell.com;
> radu.nicolau@intel.com; gakhil@marvell.com; cristian.dumitrescu@intel.com;
> konstantin.ananyev@huawei.com; dev@dpdk.org; stable@dpdk.org; Bruce
> Richardson <bruce.richardson@intel.com>
> Subject: Re: [PATCH v3 5/6] examples/qos_sched: fix lcore ID restriction
>
> On 12/20/2023 4:31 PM, Stephen Hemminger wrote:
> > On Wed, 20 Dec 2023 07:45:00 +0100
> > Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:
> >
> >> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> >> index e97273152a..22fe76eeb5 100644
> >> --- a/examples/qos_sched/args.c
> >> +++ b/examples/qos_sched/args.c
> >> @@ -182,10 +182,10 @@ app_parse_flow_conf(const char *conf_str)
> >>
> >>    pconf->rx_port = vals[0];
> >>    pconf->tx_port = vals[1];
> >> -  pconf->rx_core = (uint8_t)vals[2];
> >> -  pconf->wt_core = (uint8_t)vals[3];
> >> +  pconf->rx_core = (uint16_t)vals[2];
> >> +  pconf->wt_core = (uint16_t)vals[3];
> >>    if (ret == 5)
> >> -          pconf->tx_core = (uint8_t)vals[4];
> >> +          pconf->tx_core = (uint16_t)vals[4];
> >>    else
> >>            pconf->tx_core = pconf->wt_core;
> >>
> >> --
> >
> > Not sure why cast is even needed, assigning uint32_t to uint16_t is
> > not going to generate a warning with current compiler settings.
> >
>
> I was assuming compiler will complain when assigning uint32_t to uint16_t, but it
> seems '-Wconversion' compiler flag is required for this warning.
> Enabling this flag for DPDK build causes lots of warnings, I wonder if we should add
> a new buildtype in meson that enables this flag.
>
>
> And except from compiler warning, I think it is good to keep explicit cast where
> assignment can cause change of value. This at worst can work as documentation
> that assignment between different types done intentionally.

I would prefer to keep the explicit conversion for consistency.
Please let me know if you think otherwise.
Stephen Hemminger Jan. 16, 2024, 4:28 p.m. UTC | #4 
On Tue, 16 Jan 2024 12:33:48 +0000
"Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com> wrote:

> > > Not sure why cast is even needed, assigning uint32_t to uint16_t is
> > > not going to generate a warning with current compiler settings.
> > >  
> >
> > I was assuming compiler will complain when assigning uint32_t to uint16_t, but it
> > seems '-Wconversion' compiler flag is required for this warning.
> > Enabling this flag for DPDK build causes lots of warnings, I wonder if we should add
> > a new buildtype in meson that enables this flag.
> >
> >
> > And except from compiler warning, I think it is good to keep explicit cast where
> > assignment can cause change of value. This at worst can work as documentation
> > that assignment between different types done intentionally.  
> 
> I would prefer to keep the explicit conversion for consistency.
> Please let me know if you think otherwise.

Keep it, but casts are often a source of bug.
They defeat type checking, and often not needed.
diff mbox series

Patch

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index e97273152a..22fe76eeb5 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -182,10 +182,10 @@  app_parse_flow_conf(const char *conf_str)
 
 	pconf->rx_port = vals[0];
 	pconf->tx_port = vals[1];
-	pconf->rx_core = (uint8_t)vals[2];
-	pconf->wt_core = (uint8_t)vals[3];
+	pconf->rx_core = (uint16_t)vals[2];
+	pconf->wt_core = (uint16_t)vals[3];
 	if (ret == 5)
-		pconf->tx_core = (uint8_t)vals[4];
+		pconf->tx_core = (uint16_t)vals[4];
 	else
 		pconf->tx_core = pconf->wt_core;