[PATCH v2] sched: Fix subport profile id not set correctly.

[PATCH v2] sched: Fix subport profile id not set correctly.

[Megha Ajmera]

In rte_sched_subport_config() API, subport_profile_id is not set correctly.

Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 lib/sched/rte_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c5fa9e4582..c91697131d 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
 
 		n_subports++;
 
-		subport_profile_id = 0;
-
 		/* Port */
 		port->subports[subport_id] = s;
 
-- 
2.25.1

RE: [PATCH v2] sched: Fix subport profile id not set correctly.

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Wednesday, October 5, 2022 6:23 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH v2] sched: Fix subport profile id not set correctly.
> 
> In rte_sched_subport_config() API, subport_profile_id is not set correctly.
> 
> Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
> Cc: cristian.dumitrescu@intel.com
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  lib/sched/rte_sched.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index c5fa9e4582..c91697131d 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
> @@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
> 
>  		n_subports++;
> 
> -		subport_profile_id = 0;
> -
>  		/* Port */
>  		port->subports[subport_id] = s;
> 
> --
> 2.25.1

Hi Megha,

I noticed several patches from you that all fix small things, can you please put all of them into a series as opposed to isolated patches? This is to avoid apply issues due to dependency order. No need for a cover letter in this case.

Can you please also pay attention to the details in the patch title and description:
-title needs to have a lower case letter after "sched: "
-title needs to start with a verb
-title needs to be short and clear
-description needs to make sense

Thanks,
Cristian

RE: [PATCH v2] sched: Fix subport profile id not set correctly.

[Ajmera, Megha]

> 
> Hi Megha,
> 
> I noticed several patches from you that all fix small things, can you please put all
> of them into a series as opposed to isolated patches? This is to avoid apply
> issues due to dependency order. No need for a cover letter in this case.
> 
> Can you please also pay attention to the details in the patch title and
> description:
> -title needs to have a lower case letter after "sched: "
> -title needs to start with a verb
> -title needs to be short and clear
> -description needs to make sense

Hi Cristian,

I have incorporated review comments in recent patch-set series. Can you please help review the same?

Regards,
Megha

[PATCH 1/3] sched: fix subport profile id not set correctly

[Megha Ajmera]

In rte_sched_subport_config() API, subport_profile_id is not set correctly.

Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 lib/sched/rte_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c5fa9e4582..c91697131d 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
 
 		n_subports++;
 
-		subport_profile_id = 0;
-
 		/* Port */
 		port->subports[subport_id] = s;
 
-- 
2.25.1

[PATCH 2/3] sched: removed unused subport field in hqos profile

[Megha Ajmera]

Removed unused subport field from profile.cfg
Correctly using subport profile id in subport config load.

Fixes: 802d214dc880 ("examples/qos_sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/cfg_file.c  | 2 +-
 examples/qos_sched/profile.cfg | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index 3d5d75fcf0..ca871d3287 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -157,7 +157,7 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 	profiles = rte_cfgfile_num_sections(cfg, "subport profile",
 					   sizeof("subport profile") - 1);
-	subport_params[0].n_pipe_profiles = profiles;
+	port_params.n_subport_profiles = profiles;
 
 	for (i = 0; i < profiles; i++) {
 		char sec_name[32];
diff --git a/examples/qos_sched/profile.cfg b/examples/qos_sched/profile.cfg
index c9ec187c93..e8de101b6c 100644
--- a/examples/qos_sched/profile.cfg
+++ b/examples/qos_sched/profile.cfg
@@ -26,8 +26,6 @@ number of subports per port = 1
 number of pipes per subport = 4096
 queue sizes = 64 64 64 64 64 64 64 64 64 64 64 64 64
 
-subport 0-8 = 0                ; These subports are configured with subport profile 0
-
 [subport profile 0]
 tb rate = 1250000000           ; Bytes per second
 tb size = 1000000              ; Bytes
-- 
2.25.1

RE: [PATCH 2/3] sched: removed unused subport field in hqos profile

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Thursday, October 6, 2022 8:01 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH 2/3] sched: removed unused subport field in hqos profile
> 
> Removed unused subport field from profile.cfg
> Correctly using subport profile id in subport config load.
> 
> Fixes: 802d214dc880 ("examples/qos_sched: update subport rate
> dynamically")
> Cc: cristian.dumitrescu@intel.com
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/cfg_file.c  | 2 +-
>  examples/qos_sched/profile.cfg | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
> index 3d5d75fcf0..ca871d3287 100644
> --- a/examples/qos_sched/cfg_file.c
> +++ b/examples/qos_sched/cfg_file.c
> @@ -157,7 +157,7 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
> 
>  	profiles = rte_cfgfile_num_sections(cfg, "subport profile",
>  					   sizeof("subport profile") - 1);
> -	subport_params[0].n_pipe_profiles = profiles;
> +	port_params.n_subport_profiles = profiles;
> 
>  	for (i = 0; i < profiles; i++) {
>  		char sec_name[32];
> diff --git a/examples/qos_sched/profile.cfg
> b/examples/qos_sched/profile.cfg
> index c9ec187c93..e8de101b6c 100644
> --- a/examples/qos_sched/profile.cfg
> +++ b/examples/qos_sched/profile.cfg
> @@ -26,8 +26,6 @@ number of subports per port = 1
>  number of pipes per subport = 4096
>  queue sizes = 64 64 64 64 64 64 64 64 64 64 64 64 64
> 
> -subport 0-8 = 0                ; These subports are configured with subport profile
> 0
> -
>  [subport profile 0]
>  tb rate = 1250000000           ; Bytes per second
>  tb size = 1000000              ; Bytes
> --
> 2.25.1

Hi Megha,

Please fix:
-the title: "sched: fix number of subport profiles".
-add Cc: stable@dpdk.org, not Cc to me; you need to add me in the "To:" line of the email

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Regards,
Cristian

[PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Megha Ajmera]

Config load functions updated to support 100G rates
for subport and pipes.

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/cfg_file.c | 64 +++++++++++++++++------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index ca871d3287..ca60d616a1 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -64,67 +64,67 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
 		if (entry)
-			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
+			pipe_params[j].tb_rate = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb size");
 		if (entry)
-			pipe_params[j].tb_size = (uint64_t)atoi(entry);
+			pipe_params[j].tb_size = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc period");
 		if (entry)
-			pipe_params[j].tc_period = (uint64_t)atoi(entry);
+			pipe_params[j].tc_period = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 0 rate");
 		if (entry)
-			pipe_params[j].tc_rate[0] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[0] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 1 rate");
 		if (entry)
-			pipe_params[j].tc_rate[1] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[1] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 2 rate");
 		if (entry)
-			pipe_params[j].tc_rate[2] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[2] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 3 rate");
 		if (entry)
-			pipe_params[j].tc_rate[3] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[3] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 4 rate");
 		if (entry)
-			pipe_params[j].tc_rate[4] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[4] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 5 rate");
 		if (entry)
-			pipe_params[j].tc_rate[5] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[5] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 6 rate");
 		if (entry)
-			pipe_params[j].tc_rate[6] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[6] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 7 rate");
 		if (entry)
-			pipe_params[j].tc_rate[7] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[7] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 8 rate");
 		if (entry)
-			pipe_params[j].tc_rate[8] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[8] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 9 rate");
 		if (entry)
-			pipe_params[j].tc_rate[9] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[9] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 10 rate");
 		if (entry)
-			pipe_params[j].tc_rate[10] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[10] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 11 rate");
 		if (entry)
-			pipe_params[j].tc_rate[11] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[11] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 rate");
 		if (entry)
-			pipe_params[j].tc_rate[12] = (uint64_t)atoi(entry);
+			pipe_params[j].tc_rate[12] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 oversubscription weight");
 		if (entry)
@@ -165,67 +165,67 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb rate");
 		if (entry)
-			subport_profile[i].tb_rate = (uint64_t)atoi(entry);
+			subport_profile[i].tb_rate = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb size");
 		if (entry)
-			subport_profile[i].tb_size = (uint64_t)atoi(entry);
+			subport_profile[i].tb_size = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc period");
 		if (entry)
-			subport_profile[i].tc_period = (uint64_t)atoi(entry);
+			subport_profile[i].tc_period = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 0 rate");
 		if (entry)
-			subport_profile[i].tc_rate[0] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[0] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 1 rate");
 		if (entry)
-			subport_profile[i].tc_rate[1] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[1] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 2 rate");
 		if (entry)
-			subport_profile[i].tc_rate[2] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[2] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 3 rate");
 		if (entry)
-			subport_profile[i].tc_rate[3] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[3] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 4 rate");
 		if (entry)
-			subport_profile[i].tc_rate[4] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[4] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 5 rate");
 		if (entry)
-			subport_profile[i].tc_rate[5] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[5] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 6 rate");
 		if (entry)
-			subport_profile[i].tc_rate[6] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[6] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 7 rate");
 		if (entry)
-			subport_profile[i].tc_rate[7] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[7] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 8 rate");
 		if (entry)
-			subport_profile[i].tc_rate[8] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[8] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 9 rate");
 		if (entry)
-			subport_profile[i].tc_rate[9] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[9] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 10 rate");
 		if (entry)
-			subport_profile[i].tc_rate[10] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[10] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 11 rate");
 		if (entry)
-			subport_profile[i].tc_rate[11] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[11] = atol(entry);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
 		if (entry)
-			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
+			subport_profile[i].tc_rate[12] = atol(entry);
 	}
 
 	return 0;
-- 
2.25.1

RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Thursday, October 6, 2022 8:01 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config
> 
> Config load functions updated to support 100G rates
> for subport and pipes.
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/cfg_file.c | 64 +++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
> index ca871d3287..ca60d616a1 100644
> --- a/examples/qos_sched/cfg_file.c
> +++ b/examples/qos_sched/cfg_file.c
> @@ -64,67 +64,67 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct
> rte_sched_pipe_params *pipe_params
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
>  		if (entry)
> -			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
> +			pipe_params[j].tb_rate = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb size");
>  		if (entry)
> -			pipe_params[j].tb_size = (uint64_t)atoi(entry);
> +			pipe_params[j].tb_size = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc period");
>  		if (entry)
> -			pipe_params[j].tc_period = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_period = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 0 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[0] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[0] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 1 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[1] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[1] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 2 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[2] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[2] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 3 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[3] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[3] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 4 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[4] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[4] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 5 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[5] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[5] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 6 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[6] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[6] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 7 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[7] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[7] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 8 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[8] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[8] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 9 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[9] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[9] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 10 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[10] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[10] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 11 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[11] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[11] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 rate");
>  		if (entry)
> -			pipe_params[j].tc_rate[12] = (uint64_t)atoi(entry);
> +			pipe_params[j].tc_rate[12] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12
> oversubscription weight");
>  		if (entry)
> @@ -165,67 +165,67 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb rate");
>  		if (entry)
> -			subport_profile[i].tb_rate = (uint64_t)atoi(entry);
> +			subport_profile[i].tb_rate = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb size");
>  		if (entry)
> -			subport_profile[i].tb_size = (uint64_t)atoi(entry);
> +			subport_profile[i].tb_size = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc period");
>  		if (entry)
> -			subport_profile[i].tc_period = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_period = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 0 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[0] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[0] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 1 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[1] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[1] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 2 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[2] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[2] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 3 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[3] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[3] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 4 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[4] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[4] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 5 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[5] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[5] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 6 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[6] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[6] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 7 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[7] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[7] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 8 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[8] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[8] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 9 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[9] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[9] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 10 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[10] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[10] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 11 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[11] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[11] = atol(entry);
> 
>  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
>  		if (entry)
> -			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
> +			subport_profile[i].tc_rate[12] = atol(entry);
>  	}
> 
>  	return 0;
> --
> 2.25.1

Hi Megha,

Maybe you can explain how removing this typecast can provide support for 100+G rates?

The atoi() function returns a 32-bit value, while the subport and pipe rates are 64-bit values; this typecast can at most remove a compiler warning ...

Regards,
Cristian

RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Ajmera, Megha]

> >  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
> >  		if (entry)
> > -			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
> > +			subport_profile[i].tc_rate[12] = atol(entry);
> >  	}
> >
> >  	return 0;
> > --
> > 2.25.1
> 
> Hi Megha,
> 
> Maybe you can explain how removing this typecast can provide support for
> 100+G rates?
> 
> The atoi() function returns a 32-bit value, while the subport and pipe rates are
> 64-bit values; this typecast can at most remove a compiler warning ...

Hi Cristian,

We have now changed 'atoi' to 'atol' which will return 64-bit value so it will take care of 100G+ port speeds. However, I noticed that 'atol' will return signed-64-bit so typecast may still be needed to assign it to unsigned-64-bit variable. Will send updated patch today.

Regards,
Megha

RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Tuesday, October 18, 2022 6:40 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: stable@dpdk.org
> Subject: RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe
> config
> 
> > >  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
> > >  		if (entry)
> > > -			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
> > > +			subport_profile[i].tc_rate[12] = atol(entry);
> > >  	}
> > >
> > >  	return 0;
> > > --
> > > 2.25.1
> >
> > Hi Megha,
> >
> > Maybe you can explain how removing this typecast can provide support for
> > 100+G rates?
> >
> > The atoi() function returns a 32-bit value, while the subport and pipe rates
> are
> > 64-bit values; this typecast can at most remove a compiler warning ...
> 
> Hi Cristian,
> 
> We have now changed 'atoi' to 'atol' which will return 64-bit value so it will
> take care of 100G+ port speeds. However, I noticed that 'atol' will return
> signed-64-bit so typecast may still be needed to assign it to unsigned-64-bit
> variable. Will send updated patch today.
> 
> Regards,
> Megha

You need to use 'atoll', not 'atol'.  And yes, typecast is still required.

Re: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Stephen Hemminger]

On Tue, 18 Oct 2022 13:12:10 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> > -----Original Message-----
> > From: Ajmera, Megha <megha.ajmera@intel.com>
> > Sent: Tuesday, October 18, 2022 6:40 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> > Singh, Jasvinder <jasvinder.singh@intel.com>
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe
> > config
> >   
> > > >  		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
> > > >  		if (entry)
> > > > -			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
> > > > +			subport_profile[i].tc_rate[12] = atol(entry);
> > > >  	}
> > > >
> > > >  	return 0;
> > > > --
> > > > 2.25.1  
> > >
> > > Hi Megha,
> > >
> > > Maybe you can explain how removing this typecast can provide support for
> > > 100+G rates?
> > >
> > > The atoi() function returns a 32-bit value, while the subport and pipe rates  
> > are  
> > > 64-bit values; this typecast can at most remove a compiler warning ...  
> > 
> > Hi Cristian,
> > 
> > We have now changed 'atoi' to 'atol' which will return 64-bit value so it will
> > take care of 100G+ port speeds. However, I noticed that 'atol' will return
> > signed-64-bit so typecast may still be needed to assign it to unsigned-64-bit
> > variable. Will send updated patch today.
> > 
> > Regards,
> > Megha  
> 
> You need to use 'atoll', not 'atol'.  And yes, typecast is still required.

The code should be using strtoull() and checking for errors.

[PATCH v4 1/3] sched: fix subport profile ID

[Megha Ajmera]

In rte_sched_subport_config() API, subport_profile_id is not set correctly.

Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked by: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
---
 lib/sched/rte_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c5fa9e4582..c91697131d 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
 
 		n_subports++;
 
-		subport_profile_id = 0;
-
 		/* Port */
 		port->subports[subport_id] = s;
 
-- 
2.25.1

[PATCH v4 2/3] sched: fix number of subport profiles

[Megha Ajmera]

Removed unused subport field from profile.cfg
Correctly using subport profile id in subport config load.

Fixes: 802d214dc880 ("examples/qos_sched: update subport rate dynamically")
Cc: stable@dpdk.org

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 examples/qos_sched/cfg_file.c  | 2 +-
 examples/qos_sched/profile.cfg | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index 3d5d75fcf0..ca871d3287 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -157,7 +157,7 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 	profiles = rte_cfgfile_num_sections(cfg, "subport profile",
 					   sizeof("subport profile") - 1);
-	subport_params[0].n_pipe_profiles = profiles;
+	port_params.n_subport_profiles = profiles;
 
 	for (i = 0; i < profiles; i++) {
 		char sec_name[32];
diff --git a/examples/qos_sched/profile.cfg b/examples/qos_sched/profile.cfg
index c9ec187c93..e8de101b6c 100644
--- a/examples/qos_sched/profile.cfg
+++ b/examples/qos_sched/profile.cfg
@@ -26,8 +26,6 @@ number of subports per port = 1
 number of pipes per subport = 4096
 queue sizes = 64 64 64 64 64 64 64 64 64 64 64 64 64
 
-subport 0-8 = 0                ; These subports are configured with subport profile 0
-
 [subport profile 0]
 tb rate = 1250000000           ; Bytes per second
 tb size = 1000000              ; Bytes
-- 
2.25.1

[PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe config

[Megha Ajmera]

Config load functions updated to support 100G rates
for subport and pipes.

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/cfg_file.c | 294 ++++++++++++++++++++++++++--------
 1 file changed, 230 insertions(+), 64 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index ca871d3287..9fcdf5eeb6 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -60,71 +60,154 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params
 
 	for (j = 0; j < profiles; j++) {
 		char pipe_name[32];
+		/* Convert to decimal */
+		int base = 10;
+
 		snprintf(pipe_name, sizeof(pipe_name), "pipe profile %d", j);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
-		if (entry)
-			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tb_rate = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb size");
-		if (entry)
-			pipe_params[j].tb_size = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tb_size = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc period");
-		if (entry)
-			pipe_params[j].tc_period = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_period = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 0 rate");
-		if (entry)
-			pipe_params[j].tc_rate[0] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[0] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 1 rate");
-		if (entry)
-			pipe_params[j].tc_rate[1] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[1] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 2 rate");
-		if (entry)
-			pipe_params[j].tc_rate[2] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[2] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 3 rate");
-		if (entry)
-			pipe_params[j].tc_rate[3] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[3] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 4 rate");
-		if (entry)
-			pipe_params[j].tc_rate[4] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[4] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 5 rate");
-		if (entry)
-			pipe_params[j].tc_rate[5] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[5] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 6 rate");
-		if (entry)
-			pipe_params[j].tc_rate[6] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[6] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 7 rate");
-		if (entry)
-			pipe_params[j].tc_rate[7] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[7] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 8 rate");
-		if (entry)
-			pipe_params[j].tc_rate[8] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[8] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 9 rate");
-		if (entry)
-			pipe_params[j].tc_rate[9] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[9] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 10 rate");
-		if (entry)
-			pipe_params[j].tc_rate[10] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[10] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 11 rate");
-		if (entry)
-			pipe_params[j].tc_rate[11] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[11] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 rate");
-		if (entry)
-			pipe_params[j].tc_rate[12] = (uint64_t)atoi(entry);
+		if (entry) {
+			pipe_params[j].tc_rate[12] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 oversubscription weight");
 		if (entry)
@@ -161,71 +244,154 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 	for (i = 0; i < profiles; i++) {
 		char sec_name[32];
+		/* convert to decimal */
+		int base = 10;
+
 		snprintf(sec_name, sizeof(sec_name), "subport profile %d", i);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb rate");
-		if (entry)
-			subport_profile[i].tb_rate = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tb_rate = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb size");
-		if (entry)
-			subport_profile[i].tb_size = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tb_size = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc period");
-		if (entry)
-			subport_profile[i].tc_period = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_period = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 0 rate");
-		if (entry)
-			subport_profile[i].tc_rate[0] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[0] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 1 rate");
-		if (entry)
-			subport_profile[i].tc_rate[1] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[1] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 2 rate");
-		if (entry)
-			subport_profile[i].tc_rate[2] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[2] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 3 rate");
-		if (entry)
-			subport_profile[i].tc_rate[3] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[3] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 4 rate");
-		if (entry)
-			subport_profile[i].tc_rate[4] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[4] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 5 rate");
-		if (entry)
-			subport_profile[i].tc_rate[5] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[5] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 6 rate");
-		if (entry)
-			subport_profile[i].tc_rate[6] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[6] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 7 rate");
-		if (entry)
-			subport_profile[i].tc_rate[7] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[7] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 8 rate");
-		if (entry)
-			subport_profile[i].tc_rate[8] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[8] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 9 rate");
-		if (entry)
-			subport_profile[i].tc_rate[9] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[9] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 10 rate");
-		if (entry)
-			subport_profile[i].tc_rate[10] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[10] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 11 rate");
-		if (entry)
-			subport_profile[i].tc_rate[11] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[11] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
-		if (entry)
-			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
+		if (entry) {
+			subport_profile[i].tc_rate[12] = (uint64_t)strtoull(entry, NULL, base);
+			if (errno == EINVAL || errno == ERANGE) {
+				/* cannot convert string to unsigned long long */
+				return -1;
+			}
+		}
 	}
 
 	return 0;
-- 
2.25.1

RE: [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe config

[Dumitrescu, Cristian]

Hi Megha,

Comments inline below.

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Thursday, October 20, 2022 10:48 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> stephen@networkplumber.org
> Cc: stable@dpdk.org
> Subject: [PATCH v4 3/3] sched: support for 100G+ rates in subport/pipe
> config
> 
> Config load functions updated to support 100G rates
> for subport and pipes.
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/cfg_file.c | 294 ++++++++++++++++++++++++++------
> --
>  1 file changed, 230 insertions(+), 64 deletions(-)
> 
> diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
> index ca871d3287..9fcdf5eeb6 100644
> --- a/examples/qos_sched/cfg_file.c
> +++ b/examples/qos_sched/cfg_file.c
> @@ -60,71 +60,154 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct
> rte_sched_pipe_params *pipe_params
> 
>  	for (j = 0; j < profiles; j++) {
>  		char pipe_name[32];
> +		/* Convert to decimal */
> +		int base = 10;

We should set base to 0 to allow multiple basis (8, 10, 16), I see no reason to limit to base 10, let's take the benefits out of strtoull. I suggest we use the value 0 directly in the function, no need to have a local variable just for this.
> +
>  		snprintf(pipe_name, sizeof(pipe_name), "pipe profile %d",
> j);
> 
>  		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
> -		if (entry)
> -			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
> +		if (entry) {
> +			pipe_params[j].tb_rate = (uint64_t)strtoull(entry,
> NULL, base);
> +			if (errno == EINVAL || errno == ERANGE) {
> +				/* cannot convert string to unsigned long
> long */
> +				return -1;
> +			}
> +		}

Some comments for this recurring pattern:

1. Why are you still doing conversion to uint64_t, as strtoull already returns a 64-bit unsigned integer?
2. If you are checking errno after the call, you should first set errno to 0 before the call, right? See man strtoull, please.
3. The errno check does not fully handle the error cases, you should also use the second argument (as opposed to setting it to NULL) to check that after the call the value referenced by the second argument is 0. This ensures that there are no illegal characters after the digits, such as in the case of "1234qwerty".
4. The comment just before the return does not bring any value, as it is obvious that an error took place if you're returning an error.
5. I suggest you wrap the strtoull() usage into local function, such as parse_u64() or similar.


<snip>

Regards,
Cristian

RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config

[Ajmera, Megha]

> >
> > You need to use 'atoll', not 'atol'.  And yes, typecast is still required.
> 
> The code should be using strtoull() and checking for errors.

Thanks Cristian and Stephen. I have sent updated patch-series v4 for review and commit.

RE: [PATCH 1/3] sched: fix subport profile id not set correctly

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Thursday, October 6, 2022 8:01 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH 1/3] sched: fix subport profile id not set correctly
> 
> In rte_sched_subport_config() API, subport_profile_id is not set correctly.
> 
> Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
> Cc: cristian.dumitrescu@intel.com
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  lib/sched/rte_sched.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index c5fa9e4582..c91697131d 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
> @@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
> 
>  		n_subports++;
> 
> -		subport_profile_id = 0;
> -
>  		/* Port */
>  		port->subports[subport_id] = s;
> 
> --
> 2.25.1

Hi Megha,

Please fix the title to: "sched: fix subport profile ID".

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Regards,
Cristian

[PATCH v5 1/3] sched: fix subport profile ID

[Megha Ajmera]

In rte_sched_subport_config() API, subport_profile_id is not set correctly.

Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
---
 lib/sched/rte_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c5fa9e4582..c91697131d 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
 
 		n_subports++;
 
-		subport_profile_id = 0;
-
 		/* Port */
 		port->subports[subport_id] = s;
 
-- 
2.25.1

[PATCH v5 2/3] sched: fix number of subport profiles

[Megha Ajmera]

Removed unused subport field from profile.cfg
Correctly using subport profile id in subport config load.

Fixes: 802d214dc880 ("examples/qos_sched: update subport rate dynamically")
Cc: stable@dpdk.org

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 examples/qos_sched/cfg_file.c  | 2 +-
 examples/qos_sched/profile.cfg | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index 3d5d75fcf0..ca871d3287 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -157,7 +157,7 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 	profiles = rte_cfgfile_num_sections(cfg, "subport profile",
 					   sizeof("subport profile") - 1);
-	subport_params[0].n_pipe_profiles = profiles;
+	port_params.n_subport_profiles = profiles;
 
 	for (i = 0; i < profiles; i++) {
 		char sec_name[32];
diff --git a/examples/qos_sched/profile.cfg b/examples/qos_sched/profile.cfg
index c9ec187c93..e8de101b6c 100644
--- a/examples/qos_sched/profile.cfg
+++ b/examples/qos_sched/profile.cfg
@@ -26,8 +26,6 @@ number of subports per port = 1
 number of pipes per subport = 4096
 queue sizes = 64 64 64 64 64 64 64 64 64 64 64 64 64
 
-subport 0-8 = 0                ; These subports are configured with subport profile 0
-
 [subport profile 0]
 tb rate = 1250000000           ; Bytes per second
 tb size = 1000000              ; Bytes
-- 
2.25.1

[PATCH v5 3/3] sched: support for 100G+ rates in subport/pipe config

[Megha Ajmera]

Config load functions updated to support 100G rates
for subport and pipes.
Added new parse function to convert string to unsigned
long long.
Added error checks.

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/cfg_file.c | 180 +++++++++++++++++++++-------------
 examples/qos_sched/cfg_file.h |   2 +
 examples/qos_sched/init.c     |  16 ++-
 3 files changed, 128 insertions(+), 70 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index ca871d3287..d203621fa4 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -25,6 +25,21 @@ uint32_t n_active_queues;
 
 struct rte_sched_cman_params cman_params;
 
+int parse_u64(const char *entry, uint64_t *val)
+{
+	char *endptr;
+	if(!entry || !val)
+		return -EINVAL;
+
+	errno = 0;
+
+	*val = strtoull(entry, &endptr, 0);
+	if (errno == EINVAL || errno == ERANGE || *endptr != '\0') {
+		return -EINVAL;
+	}
+	return 0;
+}
+
 int
 cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port_params)
 {
@@ -47,7 +62,7 @@ cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port_params
 int
 cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params)
 {
-	int i, j;
+	int i, j, ret = 0;
 	char *next;
 	const char *entry;
 	int profiles;
@@ -63,68 +78,84 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params
 		snprintf(pipe_name, sizeof(pipe_name), "pipe profile %d", j);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
-		if (entry)
-			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tb_rate);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb size");
-		if (entry)
-			pipe_params[j].tb_size = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tb_size);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc period");
-		if (entry)
-			pipe_params[j].tc_period = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_period);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 0 rate");
-		if (entry)
-			pipe_params[j].tc_rate[0] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[0]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 1 rate");
-		if (entry)
-			pipe_params[j].tc_rate[1] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[1]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 2 rate");
-		if (entry)
-			pipe_params[j].tc_rate[2] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[2]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 3 rate");
-		if (entry)
-			pipe_params[j].tc_rate[3] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[3]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 4 rate");
-		if (entry)
-			pipe_params[j].tc_rate[4] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[4]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 5 rate");
-		if (entry)
-			pipe_params[j].tc_rate[5] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[5]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 6 rate");
-		if (entry)
-			pipe_params[j].tc_rate[6] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[6]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 7 rate");
-		if (entry)
-			pipe_params[j].tc_rate[7] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[7]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 8 rate");
-		if (entry)
-			pipe_params[j].tc_rate[8] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[8]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 9 rate");
-		if (entry)
-			pipe_params[j].tc_rate[9] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[9]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 10 rate");
-		if (entry)
-			pipe_params[j].tc_rate[10] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[10]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 11 rate");
-		if (entry)
-			pipe_params[j].tc_rate[11] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[11]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 rate");
-		if (entry)
-			pipe_params[j].tc_rate[12] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[12]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 oversubscription weight");
 		if (entry)
@@ -148,7 +179,7 @@ int
 cfg_load_subport_profile(struct rte_cfgfile *cfg,
 	struct rte_sched_subport_profile_params *subport_profile)
 {
-	int i;
+	int i, ret = 0;
 	const char *entry;
 	int profiles;
 
@@ -164,68 +195,85 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 		snprintf(sec_name, sizeof(sec_name), "subport profile %d", i);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb rate");
-		if (entry)
-			subport_profile[i].tb_rate = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tb_rate);
+		if (ret) {
+			return ret;
+		}
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb size");
-		if (entry)
-			subport_profile[i].tb_size = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tb_size);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc period");
-		if (entry)
-			subport_profile[i].tc_period = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_period);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 0 rate");
-		if (entry)
-			subport_profile[i].tc_rate[0] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[0]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 1 rate");
-		if (entry)
-			subport_profile[i].tc_rate[1] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[1]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 2 rate");
-		if (entry)
-			subport_profile[i].tc_rate[2] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[2]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 3 rate");
-		if (entry)
-			subport_profile[i].tc_rate[3] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[3]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 4 rate");
-		if (entry)
-			subport_profile[i].tc_rate[4] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[4]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 5 rate");
-		if (entry)
-			subport_profile[i].tc_rate[5] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[5]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 6 rate");
-		if (entry)
-			subport_profile[i].tc_rate[6] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[6]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 7 rate");
-		if (entry)
-			subport_profile[i].tc_rate[7] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[7]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 8 rate");
-		if (entry)
-			subport_profile[i].tc_rate[8] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[8]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 9 rate");
-		if (entry)
-			subport_profile[i].tc_rate[9] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[9]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 10 rate");
-		if (entry)
-			subport_profile[i].tc_rate[10] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[10]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 11 rate");
-		if (entry)
-			subport_profile[i].tc_rate[11] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[11]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
-		if (entry)
-			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[12]);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
diff --git a/examples/qos_sched/cfg_file.h b/examples/qos_sched/cfg_file.h
index 0dc458aa71..71d280718f 100644
--- a/examples/qos_sched/cfg_file.h
+++ b/examples/qos_sched/cfg_file.h
@@ -8,6 +8,8 @@
 #include <rte_sched.h>
 #include <rte_cfgfile.h>
 
+int parse_u64(const char *entry, uint64_t *val);
+
 int cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port);
 
 int cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe);
diff --git a/examples/qos_sched/init.c b/examples/qos_sched/init.c
index d897fd378b..e3b0930dd0 100644
--- a/examples/qos_sched/init.c
+++ b/examples/qos_sched/init.c
@@ -286,10 +286,18 @@ app_load_cfg_profile(const char *profile)
 	if (file == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot load configuration profile %s\n", profile);
 
-	cfg_load_port(file, &port_params);
-	cfg_load_subport(file, subport_params);
-	cfg_load_subport_profile(file, subport_profile);
-	cfg_load_pipe(file, pipe_profiles);
+	if (cfg_load_port(file, &port_params)) {
+		rte_exit(EXIT_FAILURE, "Invalid port configuration\n");
+	}
+	if (cfg_load_subport(file, subport_params)) {
+		rte_exit (EXIT_FAILURE, "Invalid subport configuration\n");
+	}
+	if (cfg_load_subport_profile(file, subport_profile)) {
+		rte_exit(EXIT_FAILURE, "Invalid subport profile configuration\n");
+	}
+	if (cfg_load_pipe(file, pipe_profiles)) {
+		rte_exit(EXIT_FAILURE, "Invalid pipe profile configuration\n");
+	}
 
 	rte_cfgfile_close(file);
 
-- 
2.25.1

[PATCH v6 1/3] sched: fix subport profile ID

[Megha Ajmera]

In rte_sched_subport_config() API, subport_profile_id is not set correctly.

Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
Cc: cristian.dumitrescu@intel.com

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
---
 lib/sched/rte_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index c5fa9e4582..c91697131d 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -1257,8 +1257,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
 
 		n_subports++;
 
-		subport_profile_id = 0;
-
 		/* Port */
 		port->subports[subport_id] = s;
 
-- 
2.25.1

[PATCH v6 2/3] sched: fix number of subport profiles

[Megha Ajmera]

Removed unused subport field from profile.cfg
Correctly using subport profile id in subport config load.

Fixes: 802d214dc880 ("examples/qos_sched: update subport rate dynamically")
Cc: stable@dpdk.org

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 examples/qos_sched/cfg_file.c  | 2 +-
 examples/qos_sched/profile.cfg | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index 3d5d75fcf0..ca871d3287 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -157,7 +157,7 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 
 	profiles = rte_cfgfile_num_sections(cfg, "subport profile",
 					   sizeof("subport profile") - 1);
-	subport_params[0].n_pipe_profiles = profiles;
+	port_params.n_subport_profiles = profiles;
 
 	for (i = 0; i < profiles; i++) {
 		char sec_name[32];
diff --git a/examples/qos_sched/profile.cfg b/examples/qos_sched/profile.cfg
index c9ec187c93..e8de101b6c 100644
--- a/examples/qos_sched/profile.cfg
+++ b/examples/qos_sched/profile.cfg
@@ -26,8 +26,6 @@ number of subports per port = 1
 number of pipes per subport = 4096
 queue sizes = 64 64 64 64 64 64 64 64 64 64 64 64 64
 
-subport 0-8 = 0                ; These subports are configured with subport profile 0
-
 [subport profile 0]
 tb rate = 1250000000           ; Bytes per second
 tb size = 1000000              ; Bytes
-- 
2.25.1

Re: [PATCH v6 2/3] sched: fix number of subport profiles

[David Marchand]

On Fri, Oct 28, 2022 at 11:59 AM Megha Ajmera <megha.ajmera@intel.com> wrote:
>

Wrong prefix in the title, it should be examples/qos_sched:.


> Removed unused subport field from profile.cfg
> Correctly using subport profile id in subport config load.

This reads odd... but I'll keep untouched.


>
> Fixes: 802d214dc880 ("examples/qos_sched: update subport rate dynamically")
> Cc: stable@dpdk.org
>
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>


-- 
David Marchand

[PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe config

[Megha Ajmera]

- Config load functions updated to support 100G rates
for subport and pipes.
- Added new parse function to convert string to unsigned
long long.
- Added error checks.
- Fixed format warnings.

Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
---
 examples/qos_sched/cfg_file.c | 179 +++++++++++++++++++++-------------
 examples/qos_sched/cfg_file.h |   2 +
 examples/qos_sched/init.c     |  23 ++++-
 3 files changed, 133 insertions(+), 71 deletions(-)

diff --git a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c
index ca871d3287..c75cf9db2e 100644
--- a/examples/qos_sched/cfg_file.c
+++ b/examples/qos_sched/cfg_file.c
@@ -25,6 +25,21 @@ uint32_t n_active_queues;
 
 struct rte_sched_cman_params cman_params;
 
+int parse_u64(const char *entry, uint64_t *val)
+{
+	char *endptr;
+	if (!entry || !val)
+		return -EINVAL;
+
+	errno = 0;
+
+	*val = strtoull(entry, &endptr, 0);
+	if (errno == EINVAL || errno == ERANGE || *endptr != '\0')
+		return -EINVAL;
+
+	return 0;
+}
+
 int
 cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port_params)
 {
@@ -47,7 +62,7 @@ cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port_params
 int
 cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params)
 {
-	int i, j;
+	int i, j, ret = 0;
 	char *next;
 	const char *entry;
 	int profiles;
@@ -63,68 +78,84 @@ cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe_params
 		snprintf(pipe_name, sizeof(pipe_name), "pipe profile %d", j);
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb rate");
-		if (entry)
-			pipe_params[j].tb_rate = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tb_rate);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tb size");
-		if (entry)
-			pipe_params[j].tb_size = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tb_size);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc period");
-		if (entry)
-			pipe_params[j].tc_period = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_period);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 0 rate");
-		if (entry)
-			pipe_params[j].tc_rate[0] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[0]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 1 rate");
-		if (entry)
-			pipe_params[j].tc_rate[1] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[1]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 2 rate");
-		if (entry)
-			pipe_params[j].tc_rate[2] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[2]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 3 rate");
-		if (entry)
-			pipe_params[j].tc_rate[3] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[3]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 4 rate");
-		if (entry)
-			pipe_params[j].tc_rate[4] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[4]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 5 rate");
-		if (entry)
-			pipe_params[j].tc_rate[5] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[5]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 6 rate");
-		if (entry)
-			pipe_params[j].tc_rate[6] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[6]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 7 rate");
-		if (entry)
-			pipe_params[j].tc_rate[7] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[7]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 8 rate");
-		if (entry)
-			pipe_params[j].tc_rate[8] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[8]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 9 rate");
-		if (entry)
-			pipe_params[j].tc_rate[9] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[9]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 10 rate");
-		if (entry)
-			pipe_params[j].tc_rate[10] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[10]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 11 rate");
-		if (entry)
-			pipe_params[j].tc_rate[11] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[11]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 rate");
-		if (entry)
-			pipe_params[j].tc_rate[12] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &pipe_params[j].tc_rate[12]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, pipe_name, "tc 12 oversubscription weight");
 		if (entry)
@@ -148,7 +179,7 @@ int
 cfg_load_subport_profile(struct rte_cfgfile *cfg,
 	struct rte_sched_subport_profile_params *subport_profile)
 {
-	int i;
+	int i, ret = 0;
 	const char *entry;
 	int profiles;
 
@@ -164,68 +195,84 @@ cfg_load_subport_profile(struct rte_cfgfile *cfg,
 		snprintf(sec_name, sizeof(sec_name), "subport profile %d", i);
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb rate");
-		if (entry)
-			subport_profile[i].tb_rate = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tb_rate);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tb size");
-		if (entry)
-			subport_profile[i].tb_size = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tb_size);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc period");
-		if (entry)
-			subport_profile[i].tc_period = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_period);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 0 rate");
-		if (entry)
-			subport_profile[i].tc_rate[0] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[0]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 1 rate");
-		if (entry)
-			subport_profile[i].tc_rate[1] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[1]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 2 rate");
-		if (entry)
-			subport_profile[i].tc_rate[2] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[2]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 3 rate");
-		if (entry)
-			subport_profile[i].tc_rate[3] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[3]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 4 rate");
-		if (entry)
-			subport_profile[i].tc_rate[4] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[4]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 5 rate");
-		if (entry)
-			subport_profile[i].tc_rate[5] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[5]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 6 rate");
-		if (entry)
-			subport_profile[i].tc_rate[6] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[6]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 7 rate");
-		if (entry)
-			subport_profile[i].tc_rate[7] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[7]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 8 rate");
-		if (entry)
-			subport_profile[i].tc_rate[8] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[8]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 9 rate");
-		if (entry)
-			subport_profile[i].tc_rate[9] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[9]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 10 rate");
-		if (entry)
-			subport_profile[i].tc_rate[10] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[10]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 11 rate");
-		if (entry)
-			subport_profile[i].tc_rate[11] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[11]);
+		if (ret)
+			return ret;
 
 		entry = rte_cfgfile_get_entry(cfg, sec_name, "tc 12 rate");
-		if (entry)
-			subport_profile[i].tc_rate[12] = (uint64_t)atoi(entry);
+		ret = parse_u64(entry, &subport_profile[i].tc_rate[12]);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
diff --git a/examples/qos_sched/cfg_file.h b/examples/qos_sched/cfg_file.h
index 0dc458aa71..71d280718f 100644
--- a/examples/qos_sched/cfg_file.h
+++ b/examples/qos_sched/cfg_file.h
@@ -8,6 +8,8 @@
 #include <rte_sched.h>
 #include <rte_cfgfile.h>
 
+int parse_u64(const char *entry, uint64_t *val);
+
 int cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params *port);
 
 int cfg_load_pipe(struct rte_cfgfile *cfg, struct rte_sched_pipe_params *pipe);
diff --git a/examples/qos_sched/init.c b/examples/qos_sched/init.c
index d897fd378b..0709aec10c 100644
--- a/examples/qos_sched/init.c
+++ b/examples/qos_sched/init.c
@@ -280,20 +280,33 @@ app_init_sched_port(uint32_t portid, uint32_t socketid)
 static int
 app_load_cfg_profile(const char *profile)
 {
+	int ret  = 0;
 	if (profile == NULL)
 		return 0;
 	struct rte_cfgfile *file = rte_cfgfile_load(profile, 0);
 	if (file == NULL)
 		rte_exit(EXIT_FAILURE, "Cannot load configuration profile %s\n", profile);
 
-	cfg_load_port(file, &port_params);
-	cfg_load_subport(file, subport_params);
-	cfg_load_subport_profile(file, subport_profile);
-	cfg_load_pipe(file, pipe_profiles);
+	ret = cfg_load_port(file, &port_params);
+	if (ret)
+		goto _app_load_cfg_profile_error_return;
 
+	ret = cfg_load_subport(file, subport_params);
+	if (ret)
+		goto _app_load_cfg_profile_error_return;
+
+	ret = cfg_load_subport_profile(file, subport_profile);
+	if (ret)
+		goto _app_load_cfg_profile_error_return;
+
+	ret = cfg_load_pipe(file, pipe_profiles);
+	if (ret)
+		goto _app_load_cfg_profile_error_return;
+
+_app_load_cfg_profile_error_return:
 	rte_cfgfile_close(file);
 
-	return 0;
+	return ret;
 }
 
 int app_init(void)
-- 
2.25.1

RE: [PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe config

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Ajmera, Megha <megha.ajmera@intel.com>
> Sent: Friday, October 28, 2022 10:56 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> stephen@networkplumber.org
> Cc: stable@dpdk.org
> Subject: [PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe
> config
> 
> - Config load functions updated to support 100G rates
> for subport and pipes.
> - Added new parse function to convert string to unsigned
> long long.
> - Added error checks.
> - Fixed format warnings.
> 
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> ---
>  examples/qos_sched/cfg_file.c | 179 +++++++++++++++++++++-------------
>  examples/qos_sched/cfg_file.h |   2 +
>  examples/qos_sched/init.c     |  23 ++++-
>  3 files changed, 133 insertions(+), 71 deletions(-)
> 

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Re: [PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe config

[David Marchand]

On Fri, Oct 28, 2022 at 12:57 PM Dumitrescu, Cristian
<cristian.dumitrescu@intel.com> wrote:
> > Subject: [PATCH v6 3/3] sched: support for 100G+ rates in subport/pipe
> > config

As mentionned in 2/3, the title prefix is incorrect.

> >
> > - Config load functions updated to support 100G rates
> > for subport and pipes.
> > - Added new parse function to convert string to unsigned
> > long long.
> > - Added error checks.
> > - Fixed format warnings.

I reformated this a bit as I don't understand why this patch deserves a list.


> >
> > Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> > ---
> >  examples/qos_sched/cfg_file.c | 179 +++++++++++++++++++++-------------
> >  examples/qos_sched/cfg_file.h |   2 +
> >  examples/qos_sched/init.c     |  23 ++++-
> >  3 files changed, 133 insertions(+), 71 deletions(-)
> >
>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
>

Series applied with fixes.


-- 
David Marchand

Re: [PATCH v6 1/3] sched: fix subport profile ID

[David Marchand]

On Fri, Oct 28, 2022 at 11:58 AM Megha Ajmera <megha.ajmera@intel.com> wrote:
>
> In rte_sched_subport_config() API, subport_profile_id is not set correctly.
>
> Fixes: ac6fcb841b0f ("sched: update subport rate dynamically")
> Cc: cristian.dumitrescu@intel.com

Cc: stable@dpdk.org

>
> Signed-off-by: Megha Ajmera <megha.ajmera@intel.com>
> Acked-by: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>

It should be Cristian Dumitrescu.

And there is no "Dumitrescu@dpdk.org".
I removed it before replying.


-- 
David Marchand