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

["Singh, Jasvinder" <jasvinder.singh@intel.com>]

> -----Original Message-----
> From: Danilewicz, MarcinX <marcinx.danilewicz@intel.com>
> Sent: Tuesday, July 5, 2022 6:06 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 v8] 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.
> 
> Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
> ---
> Log: v2 change in rte_sched.h to avoid ABI breakage.
>      v3 changes from comments
>      v4 rebase to 22.07-rc1
>      v5 rebase to main latest
>      v6 commit message fixed
>      v7 changes from comments
>      v8 with changes from comments


You need to explicitly mention the changes done in each version to help reviewer easily locate the changes.



>  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            |  47 +-------
>  examples/qos_sched/cfg_file.h            |   5 -
>  examples/qos_sched/init.c                |  76 +-----------
>  examples/qos_sched/main.h                |   2 -
>  examples/qos_sched/profile.cfg           | 135 +--------------------
>  examples/qos_sched/profile_pie.cfg       | 142 ++++++++++++++++++++++
>  examples/qos_sched/profile_red.cfg       | 143 +++++++++++++++++++++++
>  lib/sched/rte_sched.c                    |  47 +-------
>  11 files changed, 296 insertions(+), 320 deletions(-)  create mode 100644
> examples/qos_sched/profile_pie.cfg
>  create mode 100644 examples/qos_sched/profile_red.cfg
> 
> diff --git a/config/rte_config.h b/config/rte_config.h index
> 46549cb062..ae56a86394 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -88,9 +88,6 @@
>  /* rte_power defines */
>  #define RTE_MAX_LCORE_FREQS 64
> 
> -/* rte_sched defines */
> -// RTE_SCHED_CMAN is not set
> -
>  /* rte_graph defines */
>  #define RTE_GRAPH_BURST_SIZE 256
>  #define RTE_LIBRTE_GRAPH_STATS 1
> diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c
> b/drivers/net/softnic/rte_eth_softnic_tm.c
> index 6a7766ba1c..3e4bed81e9 100644
> --- a/drivers/net/softnic/rte_eth_softnic_tm.c
> +++ b/drivers/net/softnic/rte_eth_softnic_tm.c
> @@ -420,11 +420,7 @@ pmd_tm_node_type_get(struct rte_eth_dev *dev,
>  	return 0;
>  }
> 
> -#ifdef RTE_SCHED_CMAN
> -#define WRED_SUPPORTED						1
> -#else
>  #define WRED_SUPPORTED						0
> -#endif
> 
>  #define STATS_MASK_DEFAULT					\
>  	(RTE_TM_STATS_N_PKTS |					\
> @@ -2300,8 +2296,6 @@ tm_tc_wred_profile_get(struct rte_eth_dev *dev,
> uint32_t tc_id)
>  	return NULL;
>  }
> 
> -#ifdef RTE_SCHED_CMAN
> -
>  static void
>  wred_profiles_set(struct rte_eth_dev *dev, uint32_t subport_id)  { @@ -
> 2325,12 +2319,6 @@ wred_profiles_set(struct rte_eth_dev *dev, uint32_t
> subport_id)
>  		}
>  }
> 
> -#else
> -
> -#define wred_profiles_set(dev, subport_id)
> -
> -#endif
> -
>  static struct tm_shared_shaper *
>  tm_tc_shared_shaper_get(struct rte_eth_dev *dev, struct tm_node
> *tc_node)  { diff --git a/examples/ip_pipeline/tmgr.c
> b/examples/ip_pipeline/tmgr.c index b138e885cf..e68e9961be 100644
> --- a/examples/ip_pipeline/tmgr.c
> +++ b/examples/ip_pipeline/tmgr.c
> @@ -17,7 +17,6 @@ static uint32_t n_subport_profiles;  static struct
> rte_sched_pipe_params
>  	pipe_profile[TMGR_PIPE_PROFILE_MAX];
> 
> -#ifdef RTE_SCHED_CMAN
>  static struct rte_sched_cman_params cman_params = {
>  	.red_params = {
>  		/* Traffic Class 0 Colors Green / Yellow / Red */ @@ -86,7
> +85,6 @@ static struct rte_sched_cman_params cman_params = {
>  		[12][2] = {.min_th = 32, .max_th = 64, .maxp_inv = 10,
> .wq_log2 = 9},
>  		},
>  };
> -#endif /* RTE_SCHED_CMAN */
> 
>  static uint32_t n_pipe_profiles;
> 
> @@ -96,9 +94,7 @@ static const struct rte_sched_subport_params
> subport_params_default = {
>  	.pipe_profiles = pipe_profile,
>  	.n_pipe_profiles = 0, /* filled at run time */
>  	.n_max_pipe_profiles = RTE_DIM(pipe_profile), -#ifdef
> RTE_SCHED_CMAN
>  	.cman_params = &cman_params,
> -#endif /* RTE_SCHED_CMAN */
>  };


Similar to what is discussed for qos_sched sample app, set cman_params to NULL and  remove default parameters for "static struct rte_sched_cman_params cman_params" above. 


>  static struct tmgr_port_list tmgr_port_list; diff --git
> a/examples/qos_sched/cfg_file.c b/examples/qos_sched/cfg_file.c index
> 450482f07d..7f4114bd56 100644
> --- a/examples/qos_sched/cfg_file.c
> +++ b/examples/qos_sched/cfg_file.c
> @@ -23,6 +23,8 @@
>  uint32_t active_queues[RTE_SCHED_QUEUES_PER_PIPE];
>  uint32_t n_active_queues;
> 
> +struct rte_sched_cman_params cman_params;
> +
>  int
>  cfg_load_port(struct rte_cfgfile *cfg, struct rte_sched_port_params
> *port_params)  { @@ -229,40 +231,6 @@ cfg_load_subport_profile(struct
> rte_cfgfile *cfg,
>  	return 0;
>  }
> 
> -#ifdef RTE_SCHED_CMAN
> -void set_subport_cman_params(struct rte_sched_subport_params
> *subport_p,
> -					struct rte_sched_cman_params
> cman_p)
> -{
> -	int j, k;
> -	subport_p->cman_params->cman_mode = cman_p.cman_mode;
> -
> -	for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++) {
> -		if (subport_p->cman_params->cman_mode ==
> -					RTE_SCHED_CMAN_RED) {
> -			for (k = 0; k < RTE_COLORS; k++) {
> -				subport_p->cman_params-
> >red_params[j][k].min_th =
> -					cman_p.red_params[j][k].min_th;
> -				subport_p->cman_params-
> >red_params[j][k].max_th =
> -					cman_p.red_params[j][k].max_th;
> -				subport_p->cman_params-
> >red_params[j][k].maxp_inv =
> -					cman_p.red_params[j][k].maxp_inv;
> -				subport_p->cman_params-
> >red_params[j][k].wq_log2 =
> -					cman_p.red_params[j][k].wq_log2;
> -			}
> -		} else {
> -			subport_p->cman_params-
> >pie_params[j].qdelay_ref =
> -				cman_p.pie_params[j].qdelay_ref;
> -			subport_p->cman_params-
> >pie_params[j].dp_update_interval =
> -				cman_p.pie_params[j].dp_update_interval;
> -			subport_p->cman_params-
> >pie_params[j].max_burst =
> -				cman_p.pie_params[j].max_burst;
> -			subport_p->cman_params->pie_params[j].tailq_th =
> -				cman_p.pie_params[j].tailq_th;
> -		}
> -	}
> -}
> -#endif
> -
>  int
>  cfg_load_subport(struct rte_cfgfile *cfg, struct rte_sched_subport_params
> *subport_params)  { @@ -276,11 +244,7 @@ cfg_load_subport(struct
> rte_cfgfile *cfg, struct rte_sched_subport_params *subpo
>  	memset(active_queues, 0, sizeof(active_queues));
>  	n_active_queues = 0;
> 
> -#ifdef RTE_SCHED_CMAN
> -	struct rte_sched_cman_params cman_params = {
> -		.cman_mode = RTE_SCHED_CMAN_RED,
> -		.red_params = { },
> -	};
> +	subport_params->cman_params = NULL;

No need to set subport_params->cman_params  again to null as it is already set to NULL in init.c.


 
>  	if (rte_cfgfile_has_section(cfg, "red")) {
>  		cman_params.cman_mode = RTE_SCHED_CMAN_RED; @@ -
> 387,7 +351,6 @@ cfg_load_subport(struct rte_cfgfile *cfg, struct
> rte_sched_subport_params *subpo
> 
>  		}
>  	}
> -#endif /* RTE_SCHED_CMAN */
> 
>  	for (i = 0; i < MAX_SCHED_SUBPORTS; i++) {
>  		char sec_name[CFG_NAME_LEN];
> @@ -465,9 +428,7 @@ cfg_load_subport(struct rte_cfgfile *cfg, struct
> rte_sched_subport_params *subpo
>  					}
>  				}
>  			}
> -#ifdef RTE_SCHED_CMAN
> -			set_subport_cman_params(subport_params+i,
> cman_params);
> -#endif
> +			subport_params[i].cman_params = &cman_params;

Since cman_params_is global variable, memory is allocated regardless of whether cman mechanism is enabled or disabled. So subport_params->cman_params will never point to NULL even when red/pie is disabled.  Define local flag "cman_enabled" and set his flag if red or pie is enabled and check this flag to set subport_params[i].cman_params to cman_params.