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
RE: [PATCH 3/3] sched: support for 100G+ rates in subport/pipe config
> > > > 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] sched: fix for tc_ov_enable flag position in subport structure.
> > > > 10/01/2023 12:27, David Marchand: > > > On Mon, Jan 9, 2023 at 3:59 PM Megha Ajmera > > wrote: > > > > > > > > Current position of "tv_ov_enable" variable in > > > > > > tc_ov_enabled* > > > > > > > > > > rte_sched_subport structure makes the "memory" variable unused. > > > > > > I did not enter the beast... but my understanding is that some > > > object internal to rte_sched_subport currently shares location with > > > this tc_ov_enabled field. > > > So please find a better title and describe the impact. > > > > > > > > > > > > This is sta...@dpdk.org material, isn't it? > > > > > > Yes. Applicable to stable branch as well. > > Megha, > > David asked for a better description of the fix and CC stable about a month > ago, > did you miss his email? These should be quick to handle, are you able to send > V2? Hi Cristian, I missed earlier email on this thread. Sent v2 version of patch with updated title and description. Please have a look and review.
RE: [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
> ---
> 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
> ---
> 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 */
> };
>
> 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
RE: [PATCH v3] sched: enable/disable TC OV at runtime
Hi Cristian, Marcin,
> > -Original Message-
> > From: Danilewicz, MarcinX
> > Sent: Wednesday, April 27, 2022 10:24 AM
> > To: dev@dpdk.org; Singh, Jasvinder ;
> > Dumitrescu, Cristian
> > Cc: Ajmera, Megha
> > Subject: [PATCH v3] sched: enable/disable TC OV at runtime
>
> We are not trying to enable/disable the traffic class oversubscription
> feature at
> run-time, but at initialization. If cat, we should prohibit changing this
> post-
> initialization.
>
If we only need this to be configured at initialization time, then we can as
well take this flag in subport config API itself. Then there will be no need
for a new API. The purpose of new API was to enable/disable this feature at
runtime.
> Also the name of the feature should not be abbreviated in the patch title.
>
> I suggest you rework the title to:
> [PATCH] sched: enable traffic class oversubscription conditionally
>
> >
> > Added new API to enable or disable TC over subscription for best
> > effort traffic class at subport level.
> > Added changes after review and increased throughput.
> >
> > By default TC OV is disabled.
>
> It should be the other way around, the TC_OV should be enabled by default. The
> TC oversubscription is a more natural way to use this library, we usually
> want to
> disable this feature just for better performance in case this functionality
> is not
> needed. Please initialize the tc_ov flag accordingly.
>
In original code, this feature has always been disabled as it impacts
performance.
So, in my opinion we should keep it disabled by default and let user enable it
when required.
> >
> > Signed-off-by: Marcin Danilewicz
> > ---
> > lib/sched/rte_sched.c | 189
> > +++---
> > lib/sched/rte_sched.h | 18
> > lib/sched/version.map | 3 +
> > 3 files changed, 178 insertions(+), 32 deletions(-)
> >
> > diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c index
> > ec74bee939..6e7d81df46 100644
> > --- a/lib/sched/rte_sched.c
> > +++ b/lib/sched/rte_sched.c
> > @@ -213,6 +213,9 @@ struct rte_sched_subport {
> > uint8_t *bmp_array;
> > struct rte_mbuf **queue_array;
> > uint8_t memory[0] __rte_cache_aligned;
> > +
> > + /* TC oversubscription activation */
> > + int is_tc_ov_enabled;
>
> How about we simplify the name of this variable to: tc_ov_enabled ?
>
> > } __rte_cache_aligned;
> >
> > struct rte_sched_port {
> > @@ -1165,6 +1168,45 @@ rte_sched_cman_config(struct rte_sched_port
> > *port, } #endif
> >
> > +int
> > +rte_sched_subport_tc_ov_config(struct rte_sched_port *port,
> > + uint32_t subport_id,
> > + bool tc_ov_enable)
> > +{
> > + struct rte_sched_subport *s;
> > + struct rte_sched_subport_profile *profile;
> > +
> > + if (port == NULL) {
> > + RTE_LOG(ERR, SCHED,
> > + "%s: Incorrect value for parameter port\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (subport_id >= port->n_subports_per_port) {
> > + RTE_LOG(ERR, SCHED,
> > + "%s: Incorrect value for parameter subport id\n",
> > __func__);
> > + return -EINVAL;
> > + }
> > +
> > + s = port->subports[subport_id];
> > + s->is_tc_ov_enabled = tc_ov_enable ? 1 : 0;
> > +
> > + if (s->is_tc_ov_enabled) {
> > + /* TC oversubscription */
> > + s->tc_ov_wm_min = port->mtu;
> > + s->tc_ov_period_id = 0;
> > + s->tc_ov = 0;
> > + s->tc_ov_n = 0;
> > + s->tc_ov_rate = 0;
> > +
> > + profile = port->subport_profiles + s->profile;
> > + s->tc_ov_wm_max = rte_sched_time_ms_to_bytes(profile-
> > >tc_period,
> > + s->pipe_tc_be_rate_max);
> > + s->tc_ov_wm = s->tc_ov_wm_max;
> > + }
> > + return 0;
> > +}
>
> This function should not exist, please remove it and keep the initial code
> that
> computes the tc_ov related variable regardless of whether tc_ov is enabled or
> not.
>
> All the tc_ov related variables have the tc_ov particle in their name, so
> there is
> no clash. This is initialization code, so no performance overhead. Let's keep
> the
> code unmodified and compute both the tc_ov and the non-tc_ov varables at
> initialization, regardless of whether the feature is enabled or
RE: [PATCH v2] sched: Fix subport profile id not set correctly.
> > 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
RE: [PATCH v1 0/4] sched: HQoS Library cleanup.
Hi David, I have rebased patch-set with latest main branch code resolving the conflicts. Also updated cover letter. Regards, Megha -Original Message- From: David Marchand Sent: Friday, February 18, 2022 1:42 PM To: Ajmera, Megha Cc: dev ; Singh, Jasvinder ; Dumitrescu, Cristian ; Thomas Monjalon Subject: Re: [PATCH v1 0/4] sched: HQoS Library cleanup. On Fri, Feb 18, 2022 at 8:42 AM Megha Ajmera wrote: > > v1: > * Removed unused HQoS #defines from rte_config. > * Enabled stats in HQoS by default. > * TC subscription for best effort queues is always enabled in HQoS library. > * VECTOR defines are removed from HQoS library. This series does not apply on main, so we won't get tests from CI. Is this a followup of patches? https://patchwork.dpdk.org/project/dpdk/patch/20220121181459.1599739-1-megha.ajm...@intel.com/ (<-- which should be marked superseded..) https://patchwork.dpdk.org/project/dpdk/patch/20220125102105.1719667-1-megha.ajm...@intel.com/ I prefer to ask because I did not see much public discussion. Commitlogs are poor/nonexistent and the cover letter does not explain the rationale. -- David Marchand
RE: [PATCH] sched: fix integer handling issue
> From: Stephen Hemminger > Sent: Tuesday, February 22, 2022 9:33 PM > > On Tue, 22 Feb 2022 15:13:53 +0100 > Morten Brørup wrote: > > > > From: Megha Ajmera [mailto:megha.ajm...@intel.com] > > > Sent: Tuesday, 22 February 2022 14.19 > > > > > > Masking of core mask was incorrect. Instead of using 1U for > > > shifting, it should be using 1LU as the result is assigned to > > > uint64. > > > > > > CID 375859: Potentially overflowing expression "1U << app_main_core" > > > with > > > type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit > > > arithmetic, and then used in a context that expects an expression of > > > type "uint64_t" > > > (64 bits, unsigned). > > > > > > Coverity issue: 375859 > > > > > > Signed-off-by: Megha Ajmera > > > --- > > > examples/qos_sched/args.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c > > > index 10ca7bea61..44f2f5640e 100644 > > > --- a/examples/qos_sched/args.c > > > +++ b/examples/qos_sched/args.c > > > @@ -433,7 +433,7 @@ app_parse_args(int argc, char **argv) > > > return -1; > > > } > > > } > > > - app_used_core_mask |= 1u << app_main_core; > > > + app_used_core_mask |= 1lu << app_main_core; > > > > Still wrong on 32 bit platforms, where long unsigned int is still 32 bits. > > > > Use this instead: > > app_used_core_mask |= RTE_BIT64(app_main_core); > > DPDK now supports > 64 lcores. So all code using/assuming a 64 bit mask is > broken. I checked in dpdk-testpmd and looks like it still does not support more than 64 cores. Here is the snippet: else if (set_fwd_lcores_mask((uint64_t) cm) < 0) rte_exit(EXIT_FAILURE, "coremask is not valid\n"); It is type casting the core mask to uint64_t.
RE: [PATCH v1] sched: enable/disable TC OV at runtime
>
> > diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c index
> > ec74bee939..1d05089d00 100644
> > --- a/lib/sched/rte_sched.c
> > +++ b/lib/sched/rte_sched.c
> > @@ -155,6 +155,7 @@ struct rte_sched_subport {
> > uint64_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> >
> > /* TC oversubscription */
> > + uint8_t is_tc_ov_enabled;
> > uint64_t tc_ov_wm;
> > uint64_t tc_ov_wm_min;
> > uint64_t tc_ov_wm_max;
>
> Putting the field there creates a hole in the structure.
> Put it after tc_ov and fill an existing hole.
>
> This is pahole of current code, looks like this struct could use some work to
> be
> better packed and aligned.
>
Thanks Stephen for pointing this out. I agree there is some work needed to pack
this structure better.
Can we take this up in a separate patch in later release as it requires more
performance test runs to see the impact ?
