Hi Marcin,
Comments inline below.
> -----Original Message-----
> From: Danilewicz, MarcinX <marcinx.danilew...@intel.com>
> Sent: Thursday, May 12, 2022 2:11 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.si...@intel.com>;
> Dumitrescu, Cristian <cristian.dumitre...@intel.com>
> Cc: Ajmera, Megha <megha.ajm...@intel.com>
> Subject: [PATCH v2] sched: enable CMAN at runtime
>
> Added changes to enable CMAN (RED or PIE) at init
> from profile configuration file.
>
> By default CMAN code is enable but not in use, when
> there is no RED or PIE profile configured.
>
> Log: v2 change in rte_sched.h to avoid ABI breakage.
>
> Signed-off-by: Marcin Danilewicz <marcinx.danilew...@intel.com>
> ---
> config/rte_config.h | 3 -
> drivers/net/softnic/rte_eth_softnic_tm.c | 12 --
> examples/ip_pipeline/tmgr.c | 4 -
> examples/qos_sched/cfg_file.c | 14 +--
> examples/qos_sched/cfg_file.h | 2 -
> examples/qos_sched/init.c | 4 -
> examples/qos_sched/main.h | 2 -
> examples/qos_sched/profile.cfg | 130 ++++++++++-----------
> examples/qos_sched/profile_pie.cfg | 142 ++++++++++++++++++++++
> examples/qos_sched/profile_red.cfg | 143 +++++++++++++++++++++++
> lib/sched/rte_sched.c | 53 ++-------
> lib/sched/rte_sched.h | 2 +
> 12 files changed, 371 insertions(+), 140 deletions(-)
> create mode 100644 examples/qos_sched/profile_pie.cfg
> create mode 100644 examples/qos_sched/profile_red.cfg
>
<snip>
> diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
> index ec74bee939..db14934832 100644
> --- a/lib/sched/rte_sched.c
> +++ b/lib/sched/rte_sched.c
<snip>
> -#ifdef RTE_SCHED_CMAN
> static int
> rte_sched_red_config(struct rte_sched_port *port,
> struct rte_sched_subport *s,
> @@ -1161,9 +1156,11 @@ rte_sched_cman_config(struct rte_sched_port
> *port,
> else if (params->cman_params->cman_mode ==
> RTE_SCHED_CMAN_PIE)
> return rte_sched_pie_config(port, s, params, n_subports);
>
> + else if (params->cman_params->cman_mode ==
> RTE_SCHED_CMAN_NONE)
> + return 1;
> +
> return -EINVAL;
> }
> -#endif
Yes, you do need to remove the #ifdef ... #endif around this function, but no,
you don't need to change this function, as it is only called when
params->cman_params != NULL, i.e. CMAN is enabled. As mentioned before, there
is no need to add the CMAN_NONE to the enumeration, as the CMAN_NONE value is
equivalent to params->cman_params being set to NULL.
>
> int
> rte_sched_subport_config(struct rte_sched_port *port,
> @@ -1254,19 +1251,20 @@ rte_sched_subport_config(struct
> rte_sched_port *port,
> s->n_pipe_profiles = params->n_pipe_profiles;
> s->n_max_pipe_profiles = params->n_max_pipe_profiles;
>
> -#ifdef RTE_SCHED_CMAN
> + s->cman_enabled = false;
> +
> if (params->cman_params != NULL) {
> - s->cman_enabled = true;
> status = rte_sched_cman_config(port, s, params,
> n_subports);
> if (status) {
> - RTE_LOG(NOTICE, SCHED,
> - "%s: CMAN configuration fails\n",
> __func__);
> - return status;
> + if (status != 1) {
> + RTE_LOG(NOTICE, SCHED,
> + "%s: CMAN configuration
> fails\n", __func__);
> + return status;
> + }
> + } else {
> + s->cman_enabled = true;
> }
> - } else {
> - s->cman_enabled = false;
> }
> -#endif
>
Same comment here: yes, remove the #ifdef ... #endif, but no need to change
this code fragment, as it sets correctly the s->cman_enabled flag, which is
then the only flag used by the rest of the code. Again, no need to have a
CMAN_NONE in the enumeration, as the same is achieved by setting
params->cman_params to NULL.
> /* Scheduling loop detection */
> s->pipe_loop = RTE_SCHED_PIPE_INVALID;
> @@ -1825,14 +1823,10 @@
> rte_sched_port_update_queue_stats_on_drop(struct rte_sched_subport
> *subport,
>
> qe->stats.n_pkts_dropped += 1;
> qe->stats.n_bytes_dropped += pkt_len;
> -#ifdef RTE_SCHED_CMAN
> if (subport->cman_enabled)
> qe->stats.n_pkts_cman_dropped += n_pkts_cman_dropped;
> -#endif
> }
Please don't forget to remove the __rte_unused attribute for the
n_pkts_cman_dropped in the function parameters, right?
<snip>
Regards,
Cristian
