[dpdk-dev] [PATCH 2/2] sched: modify internal structs and functions for 64 bit values

Singh, Jasvinder jasvinder.singh at intel.com
Tue Oct 15 18:01:48 CEST 2019



> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Tuesday, October 15, 2019 4:47 PM
> To: Singh, Jasvinder <jasvinder.singh at intel.com>; dev at dpdk.org
> Cc: Krakowiak, LukaszX <lukaszx.krakowiak at intel.com>
> Subject: RE: [PATCH 2/2] sched: modify internal structs and functions for 64 bit
> values
> 
> Hi Jasvinder,
> 
> > -----Original Message-----
> > From: Singh, Jasvinder
> > Sent: Monday, October 14, 2019 6:25 PM
> > To: dev at dpdk.org
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; Krakowiak,
> > LukaszX <lukaszx.krakowiak at intel.com>
> > Subject: [PATCH 2/2] sched: modify internal structs and functions for
> > 64 bit values
> >
> > Modify internal structure and functions to support 64-bit values for
> > rates and stats parameters.
> >
> > Signed-off-by: Jasvinder Singh <jasvinder.singh at intel.com>
> > Signed-off-by: Lukasz Krakowiak <lukaszx.krakowiak at intel.com>
> > ---
> >  lib/librte_sched/rte_approx.c       |  57 ++++----
> >  lib/librte_sched/rte_approx.h       |   3 +-
> >  lib/librte_sched/rte_sched.c        | 211 +++++++++++++++-------------
> >  lib/librte_sched/rte_sched_common.h |  12 +-
> >  4 files changed, 156 insertions(+), 127 deletions(-)
> >
> > diff --git a/lib/librte_sched/rte_approx.c
> > b/lib/librte_sched/rte_approx.c index 30620b83d..4883d3969 100644
> > --- a/lib/librte_sched/rte_approx.c
> > +++ b/lib/librte_sched/rte_approx.c
> > @@ -18,22 +18,23 @@
> >   */
> >
> >  /* fraction comparison: compare (a/b) and (c/d) */ -static inline
> > uint32_t -less(uint32_t a, uint32_t b, uint32_t c, uint32_t d)
> > +static inline sched_counter_t
> > +less(sched_counter_t a, sched_counter_t b, sched_counter_t c,
> > sched_counter_t d)
> >  {
> >  	return a*d < b*c;
> >  }
> >
> > -static inline uint32_t
> > -less_or_equal(uint32_t a, uint32_t b, uint32_t c, uint32_t d)
> > +static inline sched_counter_t
> > +less_or_equal(sched_counter_t a, sched_counter_t b, sched_counter_t c,
> > +	sched_counter_t d)
> >  {
> >  	return a*d <= b*c;
> >  }
> >
> >  /* check whether a/b is a valid approximation */ -static inline
> > uint32_t -matches(uint32_t a, uint32_t b,
> > -	uint32_t alpha_num, uint32_t d_num, uint32_t denum)
> > +static inline sched_counter_t
> > +matches(sched_counter_t a, sched_counter_t b,
> > +	sched_counter_t alpha_num, sched_counter_t d_num,
> > sched_counter_t denum)
> >  {
> >  	if (less_or_equal(a, b, alpha_num - d_num, denum))
> >  		return 0;
> > @@ -45,33 +46,39 @@ matches(uint32_t a, uint32_t b,  }
> >
> >  static inline void
> > -find_exact_solution_left(uint32_t p_a, uint32_t q_a, uint32_t p_b,
> > uint32_t q_b,
> > -	uint32_t alpha_num, uint32_t d_num, uint32_t denum, uint32_t *p,
> > uint32_t *q)
> > +find_exact_solution_left(sched_counter_t p_a, sched_counter_t q_a,
> > +	sched_counter_t p_b, sched_counter_t q_b, sched_counter_t
> > alpha_num,
> > +	sched_counter_t d_num, sched_counter_t denum,
> > sched_counter_t *p,
> > +	sched_counter_t *q)
> >  {
> > -	uint32_t k_num = denum * p_b - (alpha_num + d_num) * q_b;
> > -	uint32_t k_denum = (alpha_num + d_num) * q_a - denum * p_a;
> > -	uint32_t k = (k_num / k_denum) + 1;
> > +	sched_counter_t k_num = denum * p_b - (alpha_num + d_num) *
> > q_b;
> > +	sched_counter_t k_denum = (alpha_num + d_num) * q_a - denum *
> > p_a;
> > +	sched_counter_t k = (k_num / k_denum) + 1;
> >
> >  	*p = p_b + k * p_a;
> >  	*q = q_b + k * q_a;
> >  }
> >
> >  static inline void
> > -find_exact_solution_right(uint32_t p_a, uint32_t q_a, uint32_t p_b,
> > uint32_t q_b,
> > -	uint32_t alpha_num, uint32_t d_num, uint32_t denum, uint32_t *p,
> > uint32_t *q)
> > +find_exact_solution_right(sched_counter_t p_a, sched_counter_t q_a,
> > +	sched_counter_t p_b, sched_counter_t q_b, sched_counter_t
> > alpha_num,
> > +	sched_counter_t d_num, sched_counter_t denum,
> > sched_counter_t *p,
> > +	sched_counter_t *q)
> >  {
> > -	uint32_t k_num = - denum * p_b + (alpha_num - d_num) * q_b;
> > -	uint32_t k_denum = - (alpha_num - d_num) * q_a + denum * p_a;
> > -	uint32_t k = (k_num / k_denum) + 1;
> > +	sched_counter_t k_num = -denum * p_b + (alpha_num - d_num) *
> > q_b;
> > +	sched_counter_t k_denum = -(alpha_num - d_num) * q_a + denum
> > * p_a;
> > +	sched_counter_t k = (k_num / k_denum) + 1;
> >
> >  	*p = p_b + k * p_a;
> >  	*q = q_b + k * q_a;
> >  }
> >
> >  static int
> > -find_best_rational_approximation(uint32_t alpha_num, uint32_t d_num,
> > uint32_t denum, uint32_t *p, uint32_t *q)
> > +find_best_rational_approximation(sched_counter_t alpha_num,
> > +	sched_counter_t d_num, sched_counter_t denum,
> > sched_counter_t *p,
> > +	sched_counter_t *q)
> >  {
> > -	uint32_t p_a, q_a, p_b, q_b;
> > +	sched_counter_t p_a, q_a, p_b, q_b;
> >
> >  	/* check assumptions on the inputs */
> >  	if (!((0 < d_num) && (d_num < alpha_num) && (alpha_num <
> > denum) && (d_num + alpha_num < denum))) { @@ -85,8 +92,8 @@
> > find_best_rational_approximation(uint32_t alpha_num, uint32_t d_num,
> > uint32_t de
> >  	q_b = 1;
> >
> >  	while (1) {
> > -		uint32_t new_p_a, new_q_a, new_p_b, new_q_b;
> > -		uint32_t x_num, x_denum, x;
> > +		sched_counter_t new_p_a, new_q_a, new_p_b, new_q_b;
> > +		sched_counter_t x_num, x_denum, x;
> >  		int aa, bb;
> >
> >  		/* compute the number of steps to the left */ @@ -139,9
> +146,9 @@
> > find_best_rational_approximation(uint32_t
> > alpha_num, uint32_t d_num, uint32_t de
> >  	}
> >  }
> >
> > -int rte_approx(double alpha, double d, uint32_t *p, uint32_t *q)
> > +int rte_approx(double alpha, double d, sched_counter_t *p,
> > sched_counter_t *q)
> >  {
> > -	uint32_t alpha_num, d_num, denum;
> > +	sched_counter_t alpha_num, d_num, denum;
> >
> >  	/* Check input arguments */
> >  	if (!((0.0 < d) && (d < alpha) && (alpha < 1.0))) { @@ -159,8 +166,8
> > @@ int rte_approx(double alpha, double d, uint32_t *p, uint32_t *q)
> >  		d *= 10;
> >  		denum *= 10;
> >  	}
> > -	alpha_num = (uint32_t) alpha;
> > -	d_num = (uint32_t) d;
> > +	alpha_num = (sched_counter_t) alpha;
> > +	d_num = (sched_counter_t) d;
> >
> >  	/* Perform approximation */
> >  	return find_best_rational_approximation(alpha_num, d_num, denum,
> p,
> > q); diff --git a/lib/librte_sched/rte_approx.h
> > b/lib/librte_sched/rte_approx.h index 0244d98f1..e591e122d 100644
> > --- a/lib/librte_sched/rte_approx.h
> > +++ b/lib/librte_sched/rte_approx.h
> > @@ -20,6 +20,7 @@ extern "C" {
> >   ***/
> >
> >  #include <stdint.h>
> > +#include "rte_sched_common.h"
> >
> >  /**
> >   * Find best rational approximation
> > @@ -37,7 +38,7 @@ extern "C" {
> >   * @return
> >   *   0 upon success, error code otherwise
> >   */
> > -int rte_approx(double alpha, double d, uint32_t *p, uint32_t *q);
> > +int rte_approx(double alpha, double d, sched_counter_t *p,
> > sched_counter_t *q);
> >
> >  #ifdef __cplusplus
> >  }
> 
> Please keep the rte_approx.[hc] independent of the librte_sched library, so use
> unit32_t or uint64_t instead of sched_counter_t that is librte_sched dependent.
> Also, for the same reason, remove the above inclusion of rte_sched_common.h.

Ok, will make these changes.

 
> Please keep the existing 32-bit functions with their current name & prototype
> and create new 64-bit functions that have the "64" suffix to their name, and use
> the 64-bit versions in the rte_sched.c implementation. Makes sense?


Yes, will add new functions with suffix "64"  in rte_approx.[hc].

> The rte_approx.[hc] files represent the implementation of an arithmetic
> algorithm that is completely independent of the scheduler library. In fact, they
> could be moved to a more generic location in DPDK where they could be
> leveraged by other libraries without the need to create a (fake) dependency to
> librte_sched.
> 
> > diff --git a/lib/librte_sched/rte_sched.c
> > b/lib/librte_sched/rte_sched.c index 710ecf65a..11d1febe2 100644
> > --- a/lib/librte_sched/rte_sched.c
> > +++ b/lib/librte_sched/rte_sched.c
> > @@ -49,13 +49,13 @@
> >
> >  struct rte_sched_pipe_profile {
> >  	/* Token bucket (TB) */
> > -	uint32_t tb_period;
> > -	uint32_t tb_credits_per_period;
> > -	uint32_t tb_size;
> > +	sched_counter_t tb_period;
> > +	sched_counter_t tb_credits_per_period;
> > +	sched_counter_t tb_size;
> >
> >  	/* Pipe traffic classes */
> > -	uint32_t tc_period;
> > -	uint32_t
> > tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t tc_period;
> > +	sched_counter_t
> > tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> >  	uint8_t tc_ov_weight;
> >
> >  	/* Pipe best-effort traffic class queues */ @@ -65,20 +65,20 @@
> > struct rte_sched_pipe_profile {  struct rte_sched_pipe {
> >  	/* Token bucket (TB) */
> >  	uint64_t tb_time; /* time of last update */
> > -	uint32_t tb_credits;
> > +	sched_counter_t tb_credits;
> >
> >  	/* Pipe profile and flags */
> >  	uint32_t profile;
> >
> >  	/* Traffic classes (TCs) */
> >  	uint64_t tc_time; /* time of next update */
> > -	uint32_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t
> > tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> >
> >  	/* Weighted Round Robin (WRR) */
> >  	uint8_t wrr_tokens[RTE_SCHED_BE_QUEUES_PER_PIPE];
> >
> >  	/* TC oversubscription */
> > -	uint32_t tc_ov_credits;
> > +	sched_counter_t tc_ov_credits;
> >  	uint8_t tc_ov_period_id;
> >  } __rte_cache_aligned;
> >
> > @@ -141,28 +141,28 @@ struct rte_sched_grinder {  struct
> > rte_sched_subport {
> >  	/* Token bucket (TB) */
> >  	uint64_t tb_time; /* time of last update */
> > -	uint32_t tb_period;
> > -	uint32_t tb_credits_per_period;
> > -	uint32_t tb_size;
> > -	uint32_t tb_credits;
> > +	sched_counter_t tb_period;
> > +	sched_counter_t tb_credits_per_period;
> > +	sched_counter_t tb_size;
> > +	sched_counter_t tb_credits;
> >
> >  	/* Traffic classes (TCs) */
> >  	uint64_t tc_time; /* time of next update */
> > -	uint32_t
> > tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > -	uint32_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > -	uint32_t tc_period;
> > +	sched_counter_t
> > tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t
> > tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t tc_period;
> >
> >  	/* TC oversubscription */
> > -	uint32_t tc_ov_wm;
> > -	uint32_t tc_ov_wm_min;
> > -	uint32_t tc_ov_wm_max;
> > +	sched_counter_t tc_ov_wm;
> > +	sched_counter_t tc_ov_wm_min;
> > +	sched_counter_t tc_ov_wm_max;
> >  	uint8_t tc_ov_period_id;
> >  	uint8_t tc_ov;
> >  	uint32_t tc_ov_n;
> >  	double tc_ov_rate;
> >
> >  	/* Statistics */
> > -	struct rte_sched_subport_stats stats;
> > +	struct rte_sched_subport_stats stats __rte_cache_aligned;
> >
> >  	/* Subport pipes */
> >  	uint32_t n_pipes_per_subport_enabled; @@ -170,7 +170,7 @@ struct
> > rte_sched_subport {
> >  	uint32_t n_max_pipe_profiles;
> >
> >  	/* Pipe best-effort TC rate */
> > -	uint32_t pipe_tc_be_rate_max;
> > +	sched_counter_t pipe_tc_be_rate_max;
> >
> >  	/* Pipe queues size */
> >  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > @@ -212,7 +212,7 @@ struct rte_sched_port {
> >  	uint16_t pipe_queue[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> >  	uint8_t pipe_tc[RTE_SCHED_QUEUES_PER_PIPE];
> >  	uint8_t tc_queue[RTE_SCHED_QUEUES_PER_PIPE];
> > -	uint32_t rate;
> > +	sched_counter_t rate;
> >  	uint32_t mtu;
> >  	uint32_t frame_overhead;
> >  	int socket;
> > @@ -517,33 +517,35 @@ rte_sched_port_log_pipe_profile(struct
> > rte_sched_subport *subport, uint32_t i)
> >  	struct rte_sched_pipe_profile *p = subport->pipe_profiles + i;
> >
> >  	RTE_LOG(DEBUG, SCHED, "Low level config for pipe profile %u:\n"
> > -		"	Token bucket: period = %u, credits per period = %u,
> > size = %u\n"
> > -		"	Traffic classes: period = %u,\n"
> > -		"	credits per period = [%u, %u, %u, %u, %u, %u, %u,
> > %u, %u, %u, %u, %u, %u]\n"
> > +		"	Token bucket: period = %"PRIu64", credits per period
> > = %"PRIu64", size = %"PRIu64"\n"
> > +		"	Traffic classes: period = %"PRIu64",\n"
> > +		"	credits per period = [%"PRIu64", %"PRIu64",
> > %"PRIu64", %"PRIu64
> > +		", %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64",
> > %"PRIu64
> > +		", %"PRIu64", %"PRIu64", %"PRIu64"]\n"
> >  		"	Best-effort traffic class oversubscription: weight =
> > %hhu\n"
> >  		"	WRR cost: [%hhu, %hhu, %hhu, %hhu]\n",
> >  		i,
> >
> >  		/* Token bucket */
> > -		p->tb_period,
> > -		p->tb_credits_per_period,
> > -		p->tb_size,
> > +		(uint64_t)p->tb_period,
> > +		(uint64_t)p->tb_credits_per_period,
> > +		(uint64_t)p->tb_size,
> >
> >  		/* Traffic classes */
> > -		p->tc_period,
> > -		p->tc_credits_per_period[0],
> > -		p->tc_credits_per_period[1],
> > -		p->tc_credits_per_period[2],
> > -		p->tc_credits_per_period[3],
> > -		p->tc_credits_per_period[4],
> > -		p->tc_credits_per_period[5],
> > -		p->tc_credits_per_period[6],
> > -		p->tc_credits_per_period[7],
> > -		p->tc_credits_per_period[8],
> > -		p->tc_credits_per_period[9],
> > -		p->tc_credits_per_period[10],
> > -		p->tc_credits_per_period[11],
> > -		p->tc_credits_per_period[12],
> > +		(uint64_t)p->tc_period,
> > +		(uint64_t)p->tc_credits_per_period[0],
> > +		(uint64_t)p->tc_credits_per_period[1],
> > +		(uint64_t)p->tc_credits_per_period[2],
> > +		(uint64_t)p->tc_credits_per_period[3],
> > +		(uint64_t)p->tc_credits_per_period[4],
> > +		(uint64_t)p->tc_credits_per_period[5],
> > +		(uint64_t)p->tc_credits_per_period[6],
> > +		(uint64_t)p->tc_credits_per_period[7],
> > +		(uint64_t)p->tc_credits_per_period[8],
> > +		(uint64_t)p->tc_credits_per_period[9],
> > +		(uint64_t)p->tc_credits_per_period[10],
> > +		(uint64_t)p->tc_credits_per_period[11],
> > +		(uint64_t)p->tc_credits_per_period[12],
> >
> >  		/* Best-effort traffic class oversubscription */
> >  		p->tc_ov_weight,
> > @@ -553,7 +555,7 @@ rte_sched_port_log_pipe_profile(struct
> > rte_sched_subport *subport, uint32_t i)  }
> >
> >  static inline uint64_t
> > -rte_sched_time_ms_to_bytes(uint32_t time_ms, uint32_t rate)
> > +rte_sched_time_ms_to_bytes(sched_counter_t time_ms,
> > sched_counter_t rate)
> >  {
> >  	uint64_t time = time_ms;
> >
> > @@ -566,7 +568,7 @@ static void
> >  rte_sched_pipe_profile_convert(struct rte_sched_subport *subport,
> >  	struct rte_sched_pipe_params *src,
> >  	struct rte_sched_pipe_profile *dst,
> > -	uint32_t rate)
> > +	sched_counter_t rate)
> >  {
> >  	uint32_t wrr_cost[RTE_SCHED_BE_QUEUES_PER_PIPE];
> >  	uint32_t lcd1, lcd2, lcd;
> > @@ -581,8 +583,8 @@ rte_sched_pipe_profile_convert(struct
> > rte_sched_subport *subport,
> >  				/ (double) rate;
> >  		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> >
> > -		rte_approx(tb_rate, d,
> > -			&dst->tb_credits_per_period, &dst->tb_period);
> > +		rte_approx(tb_rate, d, &dst->tb_credits_per_period,
> > +			&dst->tb_period);
> >  	}
> >
> >  	dst->tb_size = src->tb_size;
> > @@ -594,8 +596,8 @@ rte_sched_pipe_profile_convert(struct
> > rte_sched_subport *subport,
> >  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> >  		if (subport->qsize[i])
> >  			dst->tc_credits_per_period[i]
> > -				= rte_sched_time_ms_to_bytes(src-
> > >tc_period,
> > -					src->tc_rate[i]);
> > +				= (sched_counter_t)
> > rte_sched_time_ms_to_bytes(
> > +					src->tc_period, src->tc_rate[i]);
> >
> >  	dst->tc_ov_weight = src->tc_ov_weight;
> >
> > @@ -637,7 +639,8 @@ rte_sched_subport_config_pipe_profile_table(struct
> > rte_sched_subport *subport,
> >  	subport->pipe_tc_be_rate_max = 0;
> >  	for (i = 0; i < subport->n_pipe_profiles; i++) {
> >  		struct rte_sched_pipe_params *src = params->pipe_profiles
> > + i;
> > -		uint32_t pipe_tc_be_rate = src-
> > >tc_rate[RTE_SCHED_TRAFFIC_CLASS_BE];
> > +		sched_counter_t pipe_tc_be_rate =
> > +			src->tc_rate[RTE_SCHED_TRAFFIC_CLASS_BE];
> >
> >  		if (subport->pipe_tc_be_rate_max < pipe_tc_be_rate)
> >  			subport->pipe_tc_be_rate_max = pipe_tc_be_rate;
> @@ -647,7 +650,7
> > @@ rte_sched_subport_config_pipe_profile_table(struct
> > rte_sched_subport *subport,
> >  static int
> >  rte_sched_subport_check_params(struct rte_sched_subport_params
> > *params,
> >  	uint32_t n_max_pipes_per_subport,
> > -	uint32_t rate)
> > +	sched_counter_t rate)
> >  {
> >  	uint32_t i;
> >
> > @@ -684,7 +687,7 @@ rte_sched_subport_check_params(struct
> > rte_sched_subport_params *params,
> >  	}
> >
> >  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> > -		uint32_t tc_rate = params->tc_rate[i];
> > +		sched_counter_t tc_rate = params->tc_rate[i];
> >  		uint16_t qsize = params->qsize[i];
> >
> >  		if ((qsize == 0 && tc_rate != 0) || @@ -910,36 +913,40 @@
> > rte_sched_port_log_subport_config(struct
> > rte_sched_port *port, uint32_t i)
> >  	struct rte_sched_subport *s = port->subports[i];
> >
> >  	RTE_LOG(DEBUG, SCHED, "Low level config for subport %u:\n"
> > -		"	Token bucket: period = %u, credits per period = %u,
> > size = %u\n"
> > -		"	Traffic classes: period = %u\n"
> > -		"	credits per period = [%u, %u, %u, %u, %u, %u, %u,
> > %u, %u, %u, %u, %u, %u]\n"
> > -		"	Best effort traffic class oversubscription: wm min =
> > %u, wm max = %u\n",
> > +		"	Token bucket: period = %"PRIu64", credits per period
> > = %"PRIu64
> > +		", size = %"PRIu64"\n"
> > +		"	Traffic classes: period = %"PRIu64"\n"
> > +		"	credits per period = [%"PRIu64", %"PRIu64",
> > %"PRIu64", %"PRIu64
> > +		", %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64", %"PRIu64",
> > %"PRIu64
> > +		", %"PRIu64", %"PRIu64", %"PRIu64"]\n"
> > +		"	Best effort traffic class oversubscription: wm min =
> > %"PRIu64
> > +		", wm max = %"PRIu64"\n",
> >  		i,
> >
> >  		/* Token bucket */
> > -		s->tb_period,
> > -		s->tb_credits_per_period,
> > -		s->tb_size,
> > +		(uint64_t)s->tb_period,
> > +		(uint64_t)s->tb_credits_per_period,
> > +		(uint64_t)s->tb_size,
> >
> >  		/* Traffic classes */
> > -		s->tc_period,
> > -		s->tc_credits_per_period[0],
> > -		s->tc_credits_per_period[1],
> > -		s->tc_credits_per_period[2],
> > -		s->tc_credits_per_period[3],
> > -		s->tc_credits_per_period[4],
> > -		s->tc_credits_per_period[5],
> > -		s->tc_credits_per_period[6],
> > -		s->tc_credits_per_period[7],
> > -		s->tc_credits_per_period[8],
> > -		s->tc_credits_per_period[9],
> > -		s->tc_credits_per_period[10],
> > -		s->tc_credits_per_period[11],
> > -		s->tc_credits_per_period[12],
> > +		(uint64_t)s->tc_period,
> > +		(uint64_t)s->tc_credits_per_period[0],
> > +		(uint64_t)s->tc_credits_per_period[1],
> > +		(uint64_t)s->tc_credits_per_period[2],
> > +		(uint64_t)s->tc_credits_per_period[3],
> > +		(uint64_t)s->tc_credits_per_period[4],
> > +		(uint64_t)s->tc_credits_per_period[5],
> > +		(uint64_t)s->tc_credits_per_period[6],
> > +		(uint64_t)s->tc_credits_per_period[7],
> > +		(uint64_t)s->tc_credits_per_period[8],
> > +		(uint64_t)s->tc_credits_per_period[9],
> > +		(uint64_t)s->tc_credits_per_period[10],
> > +		(uint64_t)s->tc_credits_per_period[11],
> > +		(uint64_t)s->tc_credits_per_period[12],
> >
> >  		/* Best effort traffic class oversubscription */
> > -		s->tc_ov_wm_min,
> > -		s->tc_ov_wm_max);
> > +		(uint64_t)s->tc_ov_wm_min,
> > +		(uint64_t)s->tc_ov_wm_max);
> >  }
> >
> >  static void
> > @@ -1023,7 +1030,8 @@ rte_sched_subport_config(struct rte_sched_port
> > *port,
> >  		double tb_rate = ((double) params->tb_rate) / ((double)
> > port->rate);
> >  		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> >
> > -		rte_approx(tb_rate, d, &s->tb_credits_per_period, &s-
> > >tb_period);
> > +		rte_approx(tb_rate, d, &s->tb_credits_per_period,
> > +			&s->tb_period);
> >  	}
> >
> >  	s->tb_size = params->tb_size;
> > @@ -1035,8 +1043,8 @@ rte_sched_subport_config(struct rte_sched_port
> > *port,
> >  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> >  		if (params->qsize[i])
> >  			s->tc_credits_per_period[i]
> > -				= rte_sched_time_ms_to_bytes(params-
> > >tc_period,
> > -					params->tc_rate[i]);
> > +				= (sched_counter_t)
> > rte_sched_time_ms_to_bytes(
> > +					params->tc_period, params-
> > >tc_rate[i]);
> >  	}
> >  	s->tc_time = port->time + s->tc_period;
> >  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) @@ -
> 1970,13
> > +1978,15 @@ grinder_credits_update(struct rte_sched_port *port,
> >  	/* Subport TB */
> >  	n_periods = (port->time - subport->tb_time) / subport->tb_period;
> >  	subport->tb_credits += n_periods * subport-
> > >tb_credits_per_period;
> > -	subport->tb_credits = rte_sched_min_val_2_u32(subport-
> > >tb_credits, subport->tb_size);
> > +	subport->tb_credits = rte_sched_min_val_2(subport->tb_credits,
> > +				subport->tb_size);
> >  	subport->tb_time += n_periods * subport->tb_period;
> >
> >  	/* Pipe TB */
> >  	n_periods = (port->time - pipe->tb_time) / params->tb_period;
> >  	pipe->tb_credits += n_periods * params->tb_credits_per_period;
> > -	pipe->tb_credits = rte_sched_min_val_2_u32(pipe->tb_credits,
> > params->tb_size);
> > +	pipe->tb_credits = rte_sched_min_val_2(pipe->tb_credits,
> > +				params->tb_size);
> 
> Can we remove all the usages of rte_sched_min_val() (including its definition in
> rte_sched_common.h) and replace it with RTE_MIN, please?
> 
> >  	pipe->tb_time += n_periods * params->tb_period;
> >
> >  	/* Subport TCs */
> > @@ -1998,13 +2008,13 @@ grinder_credits_update(struct rte_sched_port
> > *port,
> >
> >  #else
> >
> > -static inline uint32_t
> > +static inline sched_counter_t
> >  grinder_tc_ov_credits_update(struct rte_sched_port *port,
> >  	struct rte_sched_subport *subport)
> >  {
> > -	uint32_t
> > tc_ov_consumption[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > -	uint32_t tc_consumption = 0, tc_ov_consumption_max;
> > -	uint32_t tc_ov_wm = subport->tc_ov_wm;
> > +	sched_counter_t
> > tc_ov_consumption[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t tc_consumption = 0, tc_ov_consumption_max;
> > +	sched_counter_t tc_ov_wm = subport->tc_ov_wm;
> >  	uint32_t i;
> >
> >  	if (subport->tc_ov == 0)
> > @@ -2053,13 +2063,15 @@ grinder_credits_update(struct rte_sched_port
> > *port,
> >  	/* Subport TB */
> >  	n_periods = (port->time - subport->tb_time) / subport->tb_period;
> >  	subport->tb_credits += n_periods * subport-
> > >tb_credits_per_period;
> > -	subport->tb_credits = rte_sched_min_val_2_u32(subport-
> > >tb_credits, subport->tb_size);
> > +	subport->tb_credits = rte_sched_min_val_2(subport->tb_credits,
> > +				subport->tb_size);
> >  	subport->tb_time += n_periods * subport->tb_period;
> >
> >  	/* Pipe TB */
> >  	n_periods = (port->time - pipe->tb_time) / params->tb_period;
> >  	pipe->tb_credits += n_periods * params->tb_credits_per_period;
> > -	pipe->tb_credits = rte_sched_min_val_2_u32(pipe->tb_credits,
> > params->tb_size);
> > +	pipe->tb_credits = rte_sched_min_val_2(pipe->tb_credits,
> > +				params->tb_size);
> >  	pipe->tb_time += n_periods * params->tb_period;
> >
> >  	/* Subport TCs */
> > @@ -2101,11 +2113,11 @@ grinder_credits_check(struct rte_sched_port
> > *port,
> >  	struct rte_sched_pipe *pipe = grinder->pipe;
> >  	struct rte_mbuf *pkt = grinder->pkt;
> >  	uint32_t tc_index = grinder->tc_index;
> > -	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
> > -	uint32_t subport_tb_credits = subport->tb_credits;
> > -	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
> > -	uint32_t pipe_tb_credits = pipe->tb_credits;
> > -	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
> > +	sched_counter_t pkt_len = pkt->pkt_len + port->frame_overhead;
> > +	sched_counter_t subport_tb_credits = subport->tb_credits;
> > +	sched_counter_t subport_tc_credits = subport-
> > >tc_credits[tc_index];
> > +	sched_counter_t pipe_tb_credits = pipe->tb_credits;
> > +	sched_counter_t pipe_tc_credits = pipe->tc_credits[tc_index];
> >  	int enough_credits;
> >
> >  	/* Check queue credits */
> > @@ -2136,21 +2148,22 @@ grinder_credits_check(struct rte_sched_port
> > *port,
> >  	struct rte_sched_pipe *pipe = grinder->pipe;
> >  	struct rte_mbuf *pkt = grinder->pkt;
> >  	uint32_t tc_index = grinder->tc_index;
> > -	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
> > -	uint32_t subport_tb_credits = subport->tb_credits;
> > -	uint32_t subport_tc_credits = subport->tc_credits[tc_index];
> > -	uint32_t pipe_tb_credits = pipe->tb_credits;
> > -	uint32_t pipe_tc_credits = pipe->tc_credits[tc_index];
> > -	uint32_t
> > pipe_tc_ov_mask1[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > -	uint32_t
> > pipe_tc_ov_mask2[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE] = {0};
> > -	uint32_t pipe_tc_ov_credits, i;
> > +	sched_counter_t pkt_len = pkt->pkt_len + port->frame_overhead;
> > +	sched_counter_t subport_tb_credits = subport->tb_credits;
> > +	sched_counter_t subport_tc_credits = subport-
> > >tc_credits[tc_index];
> > +	sched_counter_t pipe_tb_credits = pipe->tb_credits;
> > +	sched_counter_t pipe_tc_credits = pipe->tc_credits[tc_index];
> > +	sched_counter_t
> > pipe_tc_ov_mask1[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> > +	sched_counter_t
> > pipe_tc_ov_mask2[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE] = {0};
> > +	sched_counter_t pipe_tc_ov_credits;
> > +	uint32_t i;
> >  	int enough_credits;
> >
> >  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> > -		pipe_tc_ov_mask1[i] = UINT32_MAX;
> > +		pipe_tc_ov_mask1[i] = ~0;
> 
> Please use ~0LLU (or UINT64_MAX) to cover the 64-bit case gracefully. Please
> also double-check that there are no usages of UINT32_MAX left in this code,
> unless there is a reason for it. Translation from 32-bit to 64-bit arithmetic can
> be very tricky and yield some very difficult to debug issues.


Ok, will make this change. Also will make sure the there are no usage of UINT32_MAX left.


> >
> >  	pipe_tc_ov_mask1[RTE_SCHED_TRAFFIC_CLASS_BE] = pipe-
> > >tc_ov_credits;
> > -	pipe_tc_ov_mask2[RTE_SCHED_TRAFFIC_CLASS_BE] =
> > UINT32_MAX;
> > +	pipe_tc_ov_mask2[RTE_SCHED_TRAFFIC_CLASS_BE] = ~0;
> >  	pipe_tc_ov_credits = pipe_tc_ov_mask1[tc_index];
> >
> >  	/* Check pipe and subport credits */ diff --git
> > a/lib/librte_sched/rte_sched_common.h
> > b/lib/librte_sched/rte_sched_common.h
> > index 8c191a9b8..06520a686 100644
> > --- a/lib/librte_sched/rte_sched_common.h
> > +++ b/lib/librte_sched/rte_sched_common.h
> > @@ -14,8 +14,16 @@ extern "C" {
> >
> >  #define __rte_aligned_16 __attribute__((__aligned__(16)))
> >
> > -static inline uint32_t
> > -rte_sched_min_val_2_u32(uint32_t x, uint32_t y)
> > +//#define COUNTER_SIZE_64
> > +
> > +#ifdef COUNTER_SIZE_64
> > +typedef uint64_t sched_counter_t;
> > +#else
> > +typedef uint32_t sched_counter_t;
> > +#endif
> > +
> > +static inline sched_counter_t
> > +rte_sched_min_val_2(sched_counter_t x, sched_counter_t y)
> >  {
> >  	return (x < y)? x : y;
> >  }
> > --
> > 2.21.0
> 
> I know I have previously suggested the creation of sched_counter_t, but this
> was meant to be a temporary solution until the full implementation is made
> available. Now that 19.11 is meant to be an ABI stable release, we cannot
> really afford this trick (which might not be necessary either, since you have the
> full implementation), as this #ifdef COUNTER_SIZE is a massive ABI breakage.
> 
> Therefore, I strongly suggest we remove the sched_counter_t and use uint64_t
> everywhere throughout the implementation. Agree?

Yes, will make changes.
Thank you for the detailed review. I'll send revised version. 

Regards,
Jasvinder



More information about the dev mailing list