[RFC,1/2] sched: add dynamic config of subport bandwidth profile

  This patch modifies the subport level data structures
and add new API to allow dynamic configuration of the
subport bandwidth profile.

Signed-off-by: Savinay Dharmappa <savinay.dharmappa@intel.com>
Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
---
 lib/librte_sched/rte_sched.c           | 486 ++++++++++++++++++++++++---------
 lib/librte_sched/rte_sched.h           |  82 +++++-
 lib/librte_sched/rte_sched_version.map |   2 +
 3 files changed, 424 insertions(+), 146 deletions(-)
  

> -----Original Message-----
> From: Dharmappa, Savinay <savinay.dharmappa@intel.com>
> Sent: Wednesday, July 15, 2020 7:28 PM
> To: Dharmappa, Savinay <savinay.dharmappa@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: [RFC PATCH 1/2] sched: add dynamic config of subport bandwidth
> profile
> 
> This patch modifies the subport level data structures and add new API to
> allow dynamic configuration of the subport bandwidth profile.
> 
> Signed-off-by: Savinay Dharmappa <savinay.dharmappa@intel.com>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> ---
>  lib/librte_sched/rte_sched.c           | 486 ++++++++++++++++++++++++---------
>  lib/librte_sched/rte_sched.h           |  82 +++++-
>  lib/librte_sched/rte_sched_version.map |   2 +
>  3 files changed, 424 insertions(+), 146 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c index
> c0983dd..5bb0d2b 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
> 
> +#include <stdint.h>
>  #include <stdio.h>
>  #include <string.h>
> 
> @@ -101,6 +102,16 @@ enum grinder_state {
>  	e_GRINDER_READ_MBUF
>  };
> 
> +struct rte_sched_subport_profile {
> +	/* Token bucket (TB) */
> +	uint64_t tb_period;
> +	uint64_t tb_credits_per_period;
> +	uint64_t tb_size;
> +
> +	uint64_t
> tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> +	uint64_t tc_period;
> +};
> +
>  struct rte_sched_grinder {
>  	/* Pipe cache */
>  	uint16_t pcache_qmask[RTE_SCHED_GRINDER_PCACHE_SIZE];
> @@ -113,6 +124,7 @@ struct rte_sched_grinder {
>  	uint32_t productive;
>  	uint32_t pindex;
>  	struct rte_sched_subport *subport;
> +	struct rte_sched_subport_profile *subport_params;
>  	struct rte_sched_pipe *pipe;
>  	struct rte_sched_pipe_profile *pipe_params;
> 
> @@ -139,18 +151,13 @@ struct rte_sched_grinder {  };
> 
>  struct rte_sched_subport {
> -	/* Token bucket (TB) */
> +
>  	uint64_t tb_time; /* time of last update */
> -	uint64_t tb_period;
> -	uint64_t tb_credits_per_period;
> -	uint64_t tb_size;
>  	uint64_t tb_credits;
> 
>  	/* Traffic classes (TCs) */
>  	uint64_t tc_time; /* time of next update */
> -	uint64_t
> tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint64_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> -	uint64_t tc_period;
> 
>  	/* TC oversubscription */
>  	uint64_t tc_ov_wm;
> @@ -164,6 +171,8 @@ struct rte_sched_subport {
>  	/* Statistics */
>  	struct rte_sched_subport_stats stats __rte_cache_aligned;
> 
> +	/* subport profile flag */
> +	uint32_t profile;
>  	/* Subport pipes */
>  	uint32_t n_pipes_per_subport_enabled;
>  	uint32_t n_pipe_profiles;
> @@ -212,6 +221,8 @@ 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 n_subport_profiles;
> +	uint32_t n_max_subport_profiles;
>  	uint64_t rate;
>  	uint32_t mtu;
>  	uint32_t frame_overhead;
> @@ -229,6 +240,7 @@ struct rte_sched_port {
>  	uint32_t subport_id;
> 
>  	/* Large data structures */
> +	struct rte_sched_subport_profile *subport_profiles;
>  	struct rte_sched_subport *subports[0] __rte_cache_aligned;  }
> __rte_cache_aligned;
> 
> @@ -375,8 +387,60 @@ pipe_profile_check(struct rte_sched_pipe_params
> *params,  }
> 
>  static int
> +subport_profile_check(struct rte_sched_subport_profile_params *params,
> +	uint64_t rate)
> +{
> +	uint32_t i;
> +
> +	/* Check user parameters */
> +	if (params == NULL) {
> +		RTE_LOG(ERR, SCHED,
> +			"%s: Incorrect value for parameter params\n",
> __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (params->tb_rate == 0 || params->tb_rate > rate) {
> +		RTE_LOG(ERR, SCHED,
> +			"%s: Incorrect value for tb rate\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (params->tb_size == 0) {
> +		RTE_LOG(ERR, SCHED,
> +			"%s: Incorrect value for tb size\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> +		uint64_t tc_rate = params->tc_rate[i];
> +
> +		if (tc_rate == 0 || (tc_rate > params->tb_rate)) {
> +			RTE_LOG(ERR, SCHED,
> +				"%s: Incorrect value for tc rate\n", __func__);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (params->tc_rate[RTE_SCHED_TRAFFIC_CLASS_BE] == 0) {
> +		RTE_LOG(ERR, SCHED,
> +			"%s: Incorrect tc rate(best effort)\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (params->tc_period == 0) {
> +		RTE_LOG(ERR, SCHED,
> +			"%s: Incorrect value for tc period\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
>  rte_sched_port_check_params(struct rte_sched_port_params *params)  {
> +	uint32_t i;
> +
>  	if (params == NULL) {
>  		RTE_LOG(ERR, SCHED,
>  			"%s: Incorrect value for parameter params\n",
> __func__); @@ -413,6 +477,29 @@ rte_sched_port_check_params(struct
> rte_sched_port_params *params)
>  		return -EINVAL;
>  	}
> 
> +	if (params->subport_profiles == NULL ||
> +		params->n_subport_profiles == 0 ||
> +		params->n_max_subport_profiles == 0 ||
> +		params->n_subport_profiles > params-
> >n_max_subport_profiles) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: Incorrect value for subport profiles\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < params->n_subport_profiles; i++) {
> +		struct rte_sched_subport_profile_params *p =
> +						params->subport_profiles + i;
> +		int status;
> +
> +		status = subport_profile_check(p, params->rate);
> +		if (status != 0) {
> +			RTE_LOG(ERR, SCHED,
> +			"%s: subport profile check failed(%d)\n",
> +			__func__, status);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	/* n_pipes_per_subport: non-zero, power of 2 */
>  	if (params->n_pipes_per_subport == 0 ||
>  	    !rte_is_power_of_2(params->n_pipes_per_subport)) { @@ -554,6
> +641,42 @@ rte_sched_port_log_pipe_profile(struct rte_sched_subport
> *subport, uint32_t i)
>  		p->wrr_cost[0], p->wrr_cost[1], p->wrr_cost[2], p-
> >wrr_cost[3]);  }
> 
> +static void
> +rte_sched_port_log_subport_profile(struct rte_sched_port *port,
> +uint32_t i) {
> +	struct rte_sched_subport_profile *p = port->subport_profiles + i;
> +
> +	RTE_LOG(DEBUG, SCHED, "Low level config for subport profile
> %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",
> +	i,
> +
> +	/* Token bucket */
> +	p->tb_period,
> +	p->tb_credits_per_period,
> +	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]);
> +}
> +
>  static inline uint64_t
>  rte_sched_time_ms_to_bytes(uint64_t time_ms, uint64_t rate)  { @@ -623,6
> +746,37 @@ rte_sched_pipe_profile_convert(struct rte_sched_subport
> *subport,  }
> 
>  static void
> +rte_sched_subport_profile_convert(struct
> rte_sched_subport_profile_params *src,
> +	struct rte_sched_subport_profile *dst,
> +	uint64_t rate)
> +{
> +	uint32_t i;
> +
> +	/* Token Bucket */
> +	if (src->tb_rate == rate) {
> +		dst->tb_credits_per_period = 1;
> +		dst->tb_period = 1;
> +	} else {
> +		double tb_rate = (double) src->tb_rate
> +				/ (double) rate;
> +		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> +
> +		rte_approx_64(tb_rate, d, &dst->tb_credits_per_period,
> +			&dst->tb_period);
> +	}
> +
> +	dst->tb_size = src->tb_size;
> +
> +	/* Traffic Classes */
> +	dst->tc_period = rte_sched_time_ms_to_bytes(src->tc_period, rate);
> +
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> +		dst->tc_credits_per_period[i]
> +			= rte_sched_time_ms_to_bytes(src->tc_period,
> +				src->tc_rate[i]);
> +}
> +
> +static void
>  rte_sched_subport_config_pipe_profile_table(struct rte_sched_subport
> *subport,
>  	struct rte_sched_subport_params *params, uint32_t rate)  { @@ -
> 646,6 +800,24 @@ rte_sched_subport_config_pipe_profile_table(struct
> rte_sched_subport *subport,
>  	}
>  }
> 
> +static void
> +rte_sched_port_config_subport_profile_table(struct rte_sched_port *port,
> +	struct rte_sched_port_params *params,
> +	uint64_t rate)
> +{
> +	uint32_t i;
> +
> +	for (i = 0; i < port->n_subport_profiles; i++) {
> +		struct rte_sched_subport_profile_params *src
> +				= params->subport_profiles + i;
> +		struct rte_sched_subport_profile *dst
> +				= port->subport_profiles + i;
> +
> +		rte_sched_subport_profile_convert(src, dst, rate);
> +		rte_sched_port_log_subport_profile(port, i);
> +	}
> +}
> +
>  static int
>  rte_sched_subport_check_params(struct rte_sched_subport_params
> *params,
>  	uint32_t n_max_pipes_per_subport,
> @@ -660,18 +832,6 @@ rte_sched_subport_check_params(struct
> rte_sched_subport_params *params,
>  		return -EINVAL;
>  	}
> 
> -	if (params->tb_rate == 0 || params->tb_rate > rate) {
> -		RTE_LOG(ERR, SCHED,
> -			"%s: Incorrect value for tb rate\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	if (params->tb_size == 0) {
> -		RTE_LOG(ERR, SCHED,
> -			"%s: Incorrect value for tb size\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	/* qsize: if non-zero, power of 2,
>  	 * no bigger than 32K (due to 16-bit read/write pointers)
>  	 */
> @@ -685,32 +845,12 @@ rte_sched_subport_check_params(struct
> rte_sched_subport_params *params,
>  		}
>  	}
> 
> -	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> -		uint64_t tc_rate = params->tc_rate[i];
> -		uint16_t qsize = params->qsize[i];
> -
> -		if ((qsize == 0 && tc_rate != 0) ||
> -			(qsize != 0 && tc_rate == 0) ||
> -			(tc_rate > params->tb_rate)) {
> -			RTE_LOG(ERR, SCHED,
> -				"%s: Incorrect value for tc rate\n", __func__);
> -			return -EINVAL;
> -		}
> -	}
> -
> -	if (params->qsize[RTE_SCHED_TRAFFIC_CLASS_BE] == 0 ||
> -		params->tc_rate[RTE_SCHED_TRAFFIC_CLASS_BE] == 0) {
> +	if (params->qsize[RTE_SCHED_TRAFFIC_CLASS_BE] == 0) {
>  		RTE_LOG(ERR, SCHED,
>  			"%s: Incorrect qsize or tc rate(best effort)\n",
> __func__);
>  		return -EINVAL;
>  	}
> 
> -	if (params->tc_period == 0) {
> -		RTE_LOG(ERR, SCHED,
> -			"%s: Incorrect value for tc period\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	/* n_pipes_per_subport: non-zero, power of 2 */
>  	if (params->n_pipes_per_subport_enabled == 0 ||
>  		params->n_pipes_per_subport_enabled >
> n_max_pipes_per_subport || @@ -792,7 +932,7 @@ struct rte_sched_port
> *  rte_sched_port_config(struct rte_sched_port_params *params)  {
>  	struct rte_sched_port *port = NULL;
> -	uint32_t size0, size1;
> +	uint32_t size0, size1, size2;
>  	uint32_t cycles_per_byte;
>  	uint32_t i, j;
>  	int status;
> @@ -807,10 +947,21 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
> 
>  	size0 = sizeof(struct rte_sched_port);
>  	size1 = params->n_subports_per_port * sizeof(struct
> rte_sched_subport *);
> +	size2 = params->n_max_subport_profiles *
> +		sizeof(struct rte_sched_subport_profile);
> 
>  	/* Allocate memory to store the data structures */
> -	port = rte_zmalloc_socket("qos_params", size0 + size1,
> RTE_CACHE_LINE_SIZE,
> -		params->socket);
> +	port = rte_zmalloc_socket("qos_params", size0 + size1,
> +				 RTE_CACHE_LINE_SIZE, params->socket);
> +	if (port == NULL) {
> +		RTE_LOG(ERR, SCHED, "%s: Memory allocation fails\n",
> __func__);
> +
> +		return NULL;
> +	}
> +
> +	/* Allocate memory to store the subport profile */
> +	port->subport_profiles  = rte_zmalloc_socket("subport_profile",
> size2,
> +					RTE_CACHE_LINE_SIZE, params-
> >socket);
>  	if (port == NULL) {
>  		RTE_LOG(ERR, SCHED, "%s: Memory allocation fails\n",
> __func__);
> 
> @@ -819,6 +970,8 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
> 
>  	/* User parameters */
>  	port->n_subports_per_port = params->n_subports_per_port;
> +	port->n_subport_profiles = params->n_subport_profiles;
> +	port->n_max_subport_profiles = params->n_max_subport_profiles;
>  	port->n_pipes_per_subport = params->n_pipes_per_subport;
>  	port->n_pipes_per_subport_log2 =
>  			__builtin_ctz(params->n_pipes_per_subport);
> @@ -849,6 +1002,9 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
>  	port->time_cpu_bytes = 0;
>  	port->time = 0;
> 
> +	/* Subport profile table */
> +	rte_sched_port_config_subport_profile_table(port, params, port-
> >rate);
> +
>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>  		/ params->rate;
>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> @@ -907,48 +1063,6 @@ rte_sched_port_free(struct rte_sched_port *port)
> }
> 
>  static void
> -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 = %"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,
> -
> -		/* 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],
> -
> -		/* Best effort traffic class oversubscription */
> -		s->tc_ov_wm_min,
> -		s->tc_ov_wm_max);
> -}
> -
> -static void
>  rte_sched_free_memory(struct rte_sched_port *port, uint32_t n_subports)
> {
>  	uint32_t i;
> @@ -1021,33 +1135,7 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
>  	/* Port */
>  	port->subports[subport_id] = s;
> 
> -	/* Token Bucket (TB) */
> -	if (params->tb_rate == port->rate) {
> -		s->tb_credits_per_period = 1;
> -		s->tb_period = 1;
> -	} else {
> -		double tb_rate = ((double) params->tb_rate) / ((double)
> port->rate);
> -		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> -
> -		rte_approx_64(tb_rate, d, &s->tb_credits_per_period, &s-
> >tb_period);
> -	}
> -
> -	s->tb_size = params->tb_size;
>  	s->tb_time = port->time;
> -	s->tb_credits = s->tb_size / 2;
> -
> -	/* Traffic Classes (TCs) */
> -	s->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
> port->rate);
> -	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]);
> -	}
> -	s->tc_time = port->time + s->tc_period;
> -	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> -		if (params->qsize[i])
> -			s->tc_credits[i] = s->tc_credits_per_period[i];
> 
>  	/* compile time checks */
>  	RTE_BUILD_BUG_ON(RTE_SCHED_PORT_N_GRINDERS == 0); @@ -
> 1137,8 +1225,6 @@ rte_sched_subport_config(struct rte_sched_port *port,
> #ifdef RTE_SCHED_SUBPORT_TC_OV
>  	/* TC oversubscription */
>  	s->tc_ov_wm_min = port->mtu;
> -	s->tc_ov_wm_max = rte_sched_time_ms_to_bytes(params-
> >tc_period,
> -						     s->pipe_tc_be_rate_max);
>  	s->tc_ov_wm = s->tc_ov_wm_max;
>  	s->tc_ov_period_id = 0;
>  	s->tc_ov = 0;
> @@ -1146,7 +1232,54 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
>  	s->tc_ov_rate = 0;
>  #endif
> 
> -	rte_sched_port_log_subport_config(port, subport_id);
> +	return 0;
> +}
> +
> +int
> +rte_sched_subport_profile_config(struct rte_sched_port *port,
> +	uint32_t subport_id,
> +	uint32_t profile_id)
> +{
> +	int i;
> +	struct rte_sched_subport_profile *params;
> +	uint32_t n_subports = subport_id + 1;
> +	struct rte_sched_subport *s;
> +
> +	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__);
> +
> +		rte_sched_free_memory(port, n_subports);
> +		return -EINVAL;
> +	}
> +
> +	params =  port->subport_profiles + profile_id;
> +
> +	s = port->subports[subport_id];
> +
> +	s->tb_credits = params->tb_size / 2;
> +
> +	s->tc_time = port->time + params->tc_period;
> +
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> +		if (s->qsize[i])
> +			s->tc_credits[i] = params->tc_credits_per_period[i];
> +		else
> +			params->tc_credits_per_period[i] = 0;
> +
> +#ifdef RTE_SCHED_SUBPORT_TC_OV
> +	s->tc_ov_wm_max = rte_sched_time_ms_to_bytes(params-
> >tc_period,
> +						     s->pipe_tc_be_rate_max);
> +#endif
> +	s->profile = profile_id;
> +
> +	rte_sched_port_log_subport_profile(port, profile_id);
> 
>  	return 0;
>  }
> @@ -1158,6 +1291,7 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
>  	int32_t pipe_profile)
>  {
>  	struct rte_sched_subport *s;
> +	struct rte_sched_subport_profile *sp;
>  	struct rte_sched_pipe *p;
>  	struct rte_sched_pipe_profile *params;
>  	uint32_t n_subports = subport_id + 1;
> @@ -1198,14 +1332,16 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
>  		return -EINVAL;
>  	}
> 
> +	sp = port->subport_profiles + s->profile;
> +
>  	/* Handle the case when pipe already has a valid configuration */
>  	p = s->pipe + pipe_id;
>  	if (p->tb_time) {
>  		params = s->pipe_profiles + p->profile;
> 
>  		double subport_tc_be_rate =
> -			(double) s-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
> -			/ (double) s->tc_period;
> +		(double) sp-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
> +		/ (double) sp->tc_period;
>  		double pipe_tc_be_rate =
>  			(double) params-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
>  			/ (double) params->tc_period;
> @@ -1247,8 +1383,8 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
>  	{
>  		/* Subport best effort tc oversubscription */
>  		double subport_tc_be_rate =
> -			(double) s-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
> -			/ (double) s->tc_period;
> +		(double) sp-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
> +			/ (double) sp->tc_period;
>  		double pipe_tc_be_rate =
>  			(double) params-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE]
>  			/ (double) params->tc_period;
> @@ -1260,8 +1396,9 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
> 
>  		if (s->tc_ov != tc_be_ov) {
>  			RTE_LOG(DEBUG, SCHED,
> -				"Subport %u Best effort TC oversubscription
> is ON (%.4lf < %.4lf)\n",
> -				subport_id, subport_tc_be_rate, s-
> >tc_ov_rate);
> +			"Subport %u Best effort TC oversubscription is ON \
> +			(%.4lf < %.4lf)\n",
> +			subport_id, subport_tc_be_rate, s->tc_ov_rate);
>  		}
>  		p->tc_ov_period_id = s->tc_ov_period_id;
>  		p->tc_ov_credits = s->tc_ov_wm;
> @@ -1335,6 +1472,72 @@ rte_sched_subport_pipe_profile_add(struct
> rte_sched_port *port,
>  	return 0;
>  }
> 
> +int
> +rte_sched_port_subport_profile_add(struct rte_sched_port *port,
> +	struct rte_sched_subport_profile_params *params,
> +	uint32_t *subport_profile_id)
> +{
> +	int status;
> +	uint32_t i;
> +	struct rte_sched_subport_profile *dst;
> +
> +	/* Port */
> +	if (port == NULL) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: Incorrect value for parameter port\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (params == NULL) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: Incorrect value for parameter profile\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (subport_profile_id == NULL) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: Incorrect value for parameter \
> +		subport_profile_id\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	dst = port->subport_profiles + port->n_subport_profiles;
> +
> +	/* Subport profiles exceeds the max limit */
> +	if (port->n_subport_profiles >= port->n_max_subport_profiles) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: Number of subport profiles exceeds \
> +		the max limit\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	status = subport_profile_check(params, port->rate);
> +	if (status != 0) {
> +		RTE_LOG(ERR, SCHED,
> +		"%s: subport profile check failed(%d)\n", __func__, status);
> +		return -EINVAL;
> +	}
> +
> +	rte_sched_subport_profile_convert(params, dst, port->rate);
> +
> +	/* Subport profile should not exists */
> +	for (i = 0; i < port->n_subport_profiles; i++)
> +		if (memcmp(port->subport_profiles + i,
> +		    params, sizeof(*params)) == 0) {
> +			RTE_LOG(ERR, SCHED,
> +			"%s: subport profile exists\n", __func__);
> +			return -EINVAL;
> +		}
> +
> +	/* Subport profile commit */
> +	*subport_profile_id = port->n_subport_profiles;
> +	port->n_subport_profiles++;
> +
> +	rte_sched_port_log_subport_profile(port, *subport_profile_id);
> +
> +	return 0;
> +}
> +
>  static inline uint32_t
>  rte_sched_port_qindex(struct rte_sched_port *port,
>  	uint32_t subport,
> @@ -1970,14 +2173,18 @@ grinder_credits_update(struct rte_sched_port
> *port,
>  	struct rte_sched_grinder *grinder = subport->grinder + pos;
>  	struct rte_sched_pipe *pipe = grinder->pipe;
>  	struct rte_sched_pipe_profile *params = grinder->pipe_params;
> +	struct rte_sched_subport_profile *subport_params =
> +						grinder->subport_params;
>  	uint64_t n_periods;
>  	uint32_t i;
> 
>  	/* 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_MIN(subport->tb_credits, subport-
> >tb_size);
> -	subport->tb_time += n_periods * subport->tb_period;
> +	n_periods = (port->time - subport->tb_time) / subport_params-
> >tb_period;
> +	subport->tb_credits += n_periods *
> +			       subport_params->tb_credits_per_period;
> +	subport->tb_credits = RTE_MIN(subport->tb_credits,
> +				      subport_params->tb_size);
> +	subport->tb_time += n_periods * subport_params->tb_period;
> 
>  	/* Pipe TB */
>  	n_periods = (port->time - pipe->tb_time) / params->tb_period; @@ -
> 1988,9 +2195,10 @@ grinder_credits_update(struct rte_sched_port *port,
>  	/* Subport TCs */
>  	if (unlikely(port->time >= subport->tc_time)) {
>  		for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> -			subport->tc_credits[i] = subport-
> >tc_credits_per_period[i];
> +			subport->tc_credits[i] =
> +				subport_params->tc_credits_per_period[i];
> 
> -		subport->tc_time = port->time + subport->tc_period;
> +		subport->tc_time = port->time + subport_params->tc_period;
>  	}
> 
>  	/* Pipe TCs */
> @@ -2008,6 +2216,8 @@ static inline uint64_t
> grinder_tc_ov_credits_update(struct rte_sched_port *port,
>  	struct rte_sched_subport *subport)
>  {
> +	struct rte_sched_subport_profile *subport_params =
> +						grinder->subport_params;
>  	uint64_t
> tc_ov_consumption[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint64_t tc_consumption = 0, tc_ov_consumption_max;
>  	uint64_t tc_ov_wm = subport->tc_ov_wm; @@ -2018,16 +2228,17
> @@ grinder_tc_ov_credits_update(struct rte_sched_port *port,
> 
>  	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASS_BE; i++) {
>  		tc_ov_consumption[i] =
> -			subport->tc_credits_per_period[i] - subport-
> >tc_credits[i];
> +			subport_params->tc_credits_per_period[i] -
> +						subport->tc_credits[i];
>  		tc_consumption += tc_ov_consumption[i];
>  	}
> 
>  	tc_ov_consumption[RTE_SCHED_TRAFFIC_CLASS_BE] =
> -		subport-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE] -
> +	subport_params-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE] -
>  		subport->tc_credits[RTE_SCHED_TRAFFIC_CLASS_BE];
> 
>  	tc_ov_consumption_max =
> -		subport-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE] -
> +	subport_params-
> >tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASS_BE] -
>  			tc_consumption;
> 
>  	if (tc_ov_consumption[RTE_SCHED_TRAFFIC_CLASS_BE] > @@ -
> 2053,14 +2264,18 @@ grinder_credits_update(struct rte_sched_port *port,
>  	struct rte_sched_grinder *grinder = subport->grinder + pos;
>  	struct rte_sched_pipe *pipe = grinder->pipe;
>  	struct rte_sched_pipe_profile *params = grinder->pipe_params;
> +	struct rte_sched_subport_profile *subport_params =
> +						grinder->subport_params;
>  	uint64_t n_periods;
>  	uint32_t i;
> 
>  	/* 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_MIN(subport->tb_credits, subport-
> >tb_size);
> -	subport->tb_time += n_periods * subport->tb_period;
> +	n_periods = (port->time - subport->tb_time) / subport_params-
> >tb_period;
> +	subport->tb_credits += n_periods *
> +				subport_params->tb_credits_per_period;
> +	subport->tb_credits = RTE_MIN(subport->tb_credits,
> +				subport_params->tb_size);
> +	subport->tb_time += n_periods * subport_params->tb_period;
> 
>  	/* Pipe TB */
>  	n_periods = (port->time - pipe->tb_time) / params->tb_period; @@ -
> 2073,9 +2288,10 @@ grinder_credits_update(struct rte_sched_port *port,
>  		subport->tc_ov_wm = grinder_tc_ov_credits_update(port,
> subport);
> 
>  		for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> -			subport->tc_credits[i] = subport-
> >tc_credits_per_period[i];
> +			subport->tc_credits[i] =
> +				subport_params->tc_credits_per_period[i];
> 
> -		subport->tc_time = port->time + subport->tc_period;
> +		subport->tc_time = port->time + subport_params->tc_period;
>  		subport->tc_ov_period_id++;
>  	}
> 
> @@ -2598,6 +2814,8 @@ grinder_handle(struct rte_sched_port *port,
>  		struct rte_sched_pipe *pipe = grinder->pipe;
> 
>  		grinder->pipe_params = subport->pipe_profiles + pipe-
> >profile;
> +		grinder->subport_params = port->subport_profiles +
> +					  subport->profile;
>  		grinder_prefetch_tc_queue_arrays(subport, pos);
>  		grinder_credits_update(port, subport, pos);
> 
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h index
> 8a5a93c..7623919 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -149,18 +149,6 @@ struct rte_sched_pipe_params {
>   * byte.
>   */
>  struct rte_sched_subport_params {
> -	/** Token bucket rate (measured in bytes per second) */
> -	uint64_t tb_rate;
> -
> -	/** Token bucket size (measured in credits) */
> -	uint64_t tb_size;
> -
> -	/** Traffic class rates (measured in bytes per second) */
> -	uint64_t tc_rate[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> -
> -	/** Enforcement period for rates (measured in milliseconds) */
> -	uint64_t tc_period;
> -
>  	/** Number of subport pipes.
>  	 * The subport can enable/allocate fewer pipes than the maximum
>  	 * number set through struct
> port_params::n_max_pipes_per_subport,
> @@ -192,6 +180,20 @@ struct rte_sched_subport_params {  #endif  };
> 
> +struct rte_sched_subport_profile_params {
> +	/** Token bucket rate (measured in bytes per second) */
> +	uint64_t tb_rate;
> +
> +	/** Token bucket size (measured in credits) */
> +	uint64_t tb_size;
> +
> +	/** Traffic class rates (measured in bytes per second) */
> +	uint64_t tc_rate[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> +
> +	/** Enforcement period for rates (measured in milliseconds) */
> +	uint64_t tc_period;
> +};
> +
>  /** Subport statistics */
>  struct rte_sched_subport_stats {
>  	/** Number of packets successfully written */ @@ -254,6 +256,17
> @@ struct rte_sched_port_params {
>  	/** Number of subports */
>  	uint32_t n_subports_per_port;
> 
> +	/** subport profile table.
> +	 * Every pipe is configured using one of the profiles from this table.
> +	 */
> +	struct rte_sched_subport_profile_params *subport_profiles;
> +
> +	/** Profiles in the pipe profile table */
> +	uint32_t n_subport_profiles;
> +
> +	/** Max allowed profiles in the pipe profile table */
> +	uint32_t n_max_subport_profiles;
> +
>  	/** Maximum number of subport pipes.
>  	 * This parameter is used to reserve a fixed number of bits
>  	 * in struct rte_mbuf::sched.queue_id for the pipe_id for all @@ -
> 312,6 +325,29 @@ rte_sched_subport_pipe_profile_add(struct
> rte_sched_port *port,
>  	uint32_t *pipe_profile_id);
> 
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Hierarchical scheduler subport bandwidth profile add
> + * Note that this function is safe to use in runtime for adding new
> + * subport bandwidth profile as it doesn't have any impact on
> +hiearchical
> + * structure of the scheduler.
> + * @param port
> + *   Handle to port scheduler instance
> + * @param struct rte_sched_subport_profile
> + *   Subport bandwidth profile
> + * @param subport_profile_d
> + *   Subport profile id
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +__rte_experimental
> +int
> +rte_sched_port_subport_profile_add(struct rte_sched_port *port,
> +	struct rte_sched_subport_profile_params *profile,
> +	uint32_t *subport_profile_id);
> +
> +/**
>   * Hierarchical scheduler subport configuration
>   *
>   * @param port
> @@ -329,6 +365,28 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
>  	struct rte_sched_subport_params *params);
> 
>  /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Hierarchical scheduler subport profile configuration
> + * Note that this function is safe to use in runtime for applying any
> +specific
> + * subport bandwidth profile as it doesn't have any impact on
> +hiearchical
> + * structure of the scheduler.
> + * @param port
> + *   Handle to port scheduler instance
> + * @param subport_id
> + *   Subport ID
> + * @param profile_d
> + *   Subport profile id
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +__rte_experimental
> +int
> +rte_sched_subport_profile_config(struct rte_sched_port *port,
> +	uint32_t subport_id,
> +	uint32_t profile_id);
> +/**
>   * Hierarchical scheduler pipe configuration
>   *
>   * @param port
> diff --git a/lib/librte_sched/rte_sched_version.map
> b/lib/librte_sched/rte_sched_version.map
> index cefd990..c02a223 100644
> --- a/lib/librte_sched/rte_sched_version.map
> +++ b/lib/librte_sched/rte_sched_version.map
> @@ -28,4 +28,6 @@ EXPERIMENTAL {
>  	global:
> 
>  	rte_sched_subport_pipe_profile_add;
> +	rte_sched_port_subport_profile_add;
> +	rte_sched_subport_profile_config;
>  };
> --
> 2.7.4

+ Cristian
  

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Savinay Dharmappa
> Sent: Wednesday, July 15, 2020 7:28 PM
> To: Dharmappa, Savinay <savinay.dharmappa@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: [dpdk-dev] [RFC PATCH 1/2] sched: add dynamic config of subport
> bandwidth profile
> 
> This patch modifies the subport level data structures
> and add new API to allow dynamic configuration of the
> subport bandwidth profile.
> 
> Signed-off-by: Savinay Dharmappa <savinay.dharmappa@intel.com>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> ---
>  lib/librte_sched/rte_sched.c           | 486 ++++++++++++++++++++++++------
> ---
>  lib/librte_sched/rte_sched.h           |  82 +++++-
>  lib/librte_sched/rte_sched_version.map |   2 +
>  3 files changed, 424 insertions(+), 146 deletions(-)
> 

Hi Savinay,

Your patch makes sense, but it is very hard to review, as you put a massive amount of changes (~500 LOCs) in a single patch file.

Can you please split your patch into a patchset with incremental changes that can be reviewed easier? Some suggestions: split the internal changes in multiple increments first before changing the API (like move the check code into separate functions in individual patches, etc), change the API afterwards.

Thanks,
Cristian
  

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Monday, July 20, 2020 12:21 PM
> To: Savinay Dharmappa <savinay.dharmappa@intel.com>; Dharmappa,
> Savinay <Savinay.Dharmappa@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [RFC PATCH 1/2] sched: add dynamic config of
> subport bandwidth profile
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Savinay Dharmappa
> > Sent: Wednesday, July 15, 2020 7:28 PM
> > To: Dharmappa, Savinay <savinay.dharmappa@intel.com>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; dev@dpdk.org
> > Subject: [dpdk-dev] [RFC PATCH 1/2] sched: add dynamic config of subport
> > bandwidth profile
> >
> > This patch modifies the subport level data structures
> > and add new API to allow dynamic configuration of the
> > subport bandwidth profile.
> >
> > Signed-off-by: Savinay Dharmappa <savinay.dharmappa@intel.com>
> > Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> > ---
> >  lib/librte_sched/rte_sched.c           | 486 ++++++++++++++++++++++++----
> --
> > ---
> >  lib/librte_sched/rte_sched.h           |  82 +++++-
> >  lib/librte_sched/rte_sched_version.map |   2 +
> >  3 files changed, 424 insertions(+), 146 deletions(-)
> >
> 
> Hi Savinay,
> 
> Your patch makes sense, but it is very hard to review, as you put a massive
> amount of changes (~500 LOCs) in a single patch file.
> 
> Can you please split your patch into a patchset with incremental changes that
> can be reviewed easier? Some suggestions: split the internal changes in
> multiple increments first before changing the API (like move the check code
> into separate functions in individual patches, etc), change the API
> afterwards.
> 
> Thanks,
> Cristian

Also please send v1 (non-RFC) as part of the next iteration, thanks!