RE: [PATCH v2] sched: enable CMAN at runtime

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Marcin,

Comments inline below.

> -----Original Message-----
> From: Danilewicz, MarcinX <marcinx.danilewicz@intel.com>
> Sent: Thursday, May 12, 2022 2:11 PM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Ajmera, Megha <megha.ajmera@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.danilewicz@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