diff mbox series

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

Message ID 20221020094747.605087-3-megha.ajmera@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v4,1/3] sched: fix subport profile ID |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-x86_64-unit-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Ajmera, Megha Oct. 20, 2022, 9:47 a.m. UTC 
  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(-)

Comments

Cristian Dumitrescu Oct. 24, 2022, 6:57 p.m. UTC | #1 
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
diff mbox series

Patch

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;