[PATCH 1/2] librte_sched: add post-init pipe profile api

[PATCH 1/2] librte_sched: add post-init pipe profile api

[Jasvinder Singh]

Add new API function to add more pipe configuration profiles
post initialization to the set of exisitng profiles specified during
the creation of scheduler port.

This API removes the current limitation that forces the user
to define the full set of pipe profiles as the part of port parameters
while port is being created.

Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
---
 lib/librte_sched/rte_sched.c           | 141 +++++++++++++++++++++++++++++++++
 lib/librte_sched/rte_sched.h           |  17 ++++
 lib/librte_sched/rte_sched_version.map |   6 ++
 3 files changed, 164 insertions(+)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 634486c..43728ec 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -932,6 +932,147 @@ rte_sched_pipe_config(struct rte_sched_port *port,
 	return 0;
 }
 
+static void
+rte_sched_pipe_profile_get(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params,
+	struct rte_sched_pipe_profile *p)
+{
+	uint32_t i;
+
+	/* Token Bucket */
+	if (params->tb_rate == port->rate) {
+		p->tb_credits_per_period = 1;
+		p->tb_period = 1;
+	} else {
+		double tb_rate = (double) params->tb_rate
+				/ (double) port->rate;
+		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
+
+		rte_approx(tb_rate, d,
+				   &p->tb_credits_per_period, &p->tb_period);
+	}
+
+	p->tb_size = params->tb_size;
+
+	/* Traffic Classes */
+	p->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
+		port->rate);
+
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
+		p->tc_credits_per_period[i]
+			= rte_sched_time_ms_to_bytes(params->tc_period,
+				params->tc_rate[i]);
+
+#ifdef RTE_SCHED_SUBPORT_TC_OV
+	p->tc_ov_weight = params->tc_ov_weight;
+#endif
+
+	/* WRR */
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+		uint32_t wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
+		uint32_t lcd, lcd1, lcd2;
+		uint32_t qindex;
+
+		qindex = i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
+
+		wrr_cost[0] = params->wrr_weights[qindex];
+		wrr_cost[1] = params->wrr_weights[qindex + 1];
+		wrr_cost[2] = params->wrr_weights[qindex + 2];
+		wrr_cost[3] = params->wrr_weights[qindex + 3];
+
+		lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
+		lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
+		lcd = rte_get_lcd(lcd1, lcd2);
+
+		wrr_cost[0] = lcd / wrr_cost[0];
+		wrr_cost[1] = lcd / wrr_cost[1];
+		wrr_cost[2] = lcd / wrr_cost[2];
+		wrr_cost[3] = lcd / wrr_cost[3];
+
+		p->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
+		p->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
+		p->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
+		p->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
+	}
+}
+
+int
+rte_sched_pipe_profile_add(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params,
+	int32_t *profile_id)
+{
+	struct rte_sched_pipe_profile pp;
+	uint32_t i;
+
+	/* Port */
+	if (port == NULL)
+		return -1;
+
+	/* Pipe parameters */
+	if (params == NULL)
+		return -2;
+
+	/* TB rate: non-zero, not greater than port rate */
+	if (params->tb_rate == 0 ||
+		params->tb_rate > port->rate)
+		return -3;
+
+	/* TB size: non-zero */
+	if (params->tb_size == 0)
+		return -4;
+
+	/* TC rate: non-zero, less than pipe rate */
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+		if (params->tc_rate[i] == 0 ||
+			params->tc_rate[i] > params->tb_rate)
+			return -5;
+	}
+
+	/* TC period: non-zero */
+	if (params->tc_period == 0)
+		return -6;
+
+#ifdef RTE_SCHED_SUBPORT_TC_OV
+	/* TC3 oversubscription weight: non-zero */
+	if (params->tc_ov_weight == 0)
+		return -7;
+#endif
+
+	/* Queue WRR weights: non-zero */
+	for (i = 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
+		if (params->wrr_weights[i] == 0)
+			return -8;
+	}
+
+	/* Pipe profiles not exceeds the max limit */
+	if (port->n_pipe_profiles >= RTE_SCHED_PIPE_PROFILES_PER_PORT)
+		return -9;
+
+	memset(&pp, 0, sizeof(struct rte_sched_pipe_profile));
+	rte_sched_pipe_profile_get(port, params, &pp);
+
+	/* Pipe profile not exists */
+	for (i = 0; i < port->n_pipe_profiles; i++) {
+		if (memcmp(port->pipe_profiles + i, &pp, sizeof(pp)) == 0)
+			return -10;
+	}
+
+	/* Set port params */
+	memcpy(port->pipe_profiles + port->n_pipe_profiles, &pp, sizeof(pp));
+
+	uint32_t pipe_tc3_rate = params->tc_rate[3];
+
+	if (port->pipe_tc3_rate_max < pipe_tc3_rate)
+		port->pipe_tc3_rate_max = pipe_tc3_rate;
+
+	*profile_id = port->n_pipe_profiles;
+	port->n_pipe_profiles += 1;
+
+	rte_sched_port_log_pipe_profile(port, *profile_id);
+
+	return 0;
+}
+
 void
 rte_sched_port_pkt_write(struct rte_mbuf *pkt,
 			 uint32_t subport, uint32_t pipe, uint32_t traffic_class,
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index 5d2a688..7edccbe 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -271,6 +271,23 @@ rte_sched_pipe_config(struct rte_sched_port *port,
 	int32_t pipe_profile);
 
 /**
+ * Hierarchical scheduler pipe profile add
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param params
+ *   Pipe configuration parameters
+ * @param pipe_profile_id
+ *   Set to valid profile id when profile is added successfully.
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int
+rte_sched_pipe_profile_add(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params,
+	int32_t *pipe_profile_id);
+
+/**
  * Hierarchical scheduler memory footprint size per port
  *
  * @param params
diff --git a/lib/librte_sched/rte_sched_version.map b/lib/librte_sched/rte_sched_version.map
index 3aa159a..b709cf8 100644
--- a/lib/librte_sched/rte_sched_version.map
+++ b/lib/librte_sched/rte_sched_version.map
@@ -29,3 +29,9 @@ DPDK_2.1 {
 	rte_sched_port_pkt_read_color;
 
 } DPDK_2.0;
+
+DPDK_18.05 {
+	global:
+
+	rte_sched_pipe_profile_add;
+} DPDK_2.1;
-- 
2.9.3

[PATCH 2/2] test/sched: add test for pipe profile add api

[Jasvinder Singh]

Update the unit test to check the working of new API.

Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
---
 test/test/test_sched.c | 91 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 69 insertions(+), 22 deletions(-)

diff --git a/test/test/test_sched.c b/test/test/test_sched.c
index 32e500b..5d30187 100644
--- a/test/test/test_sched.c
+++ b/test/test/test_sched.c
@@ -44,6 +44,18 @@ static struct rte_sched_pipe_params pipe_profile[] = {
 	},
 };
 
+static struct rte_sched_pipe_params pipe_profile1 = {
+	/* Profile #1 */
+	.tb_rate = 300000,
+	.tb_size = 100000,
+
+	.tc_rate = {300000, 300000, 300000, 300000},
+	.tc_period = 40,
+
+	.wrr_weights = {1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1},
+};
+
+
 static struct rte_sched_port_params port_param = {
 	.socket = 0, /* computed */
 	.rate = 0, /* computed */
@@ -102,7 +114,23 @@ prepare_pkt(struct rte_mbuf *mbuf)
 	mbuf->data_len = 60;
 }
 
+static int pipe_profile_set(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params)
+{
+	uint32_t pipe;
+	int32_t profile_id, err;
 
+	err = rte_sched_pipe_profile_add(port, params, &profile_id);
+	if (err)
+		return err;
+
+	for (pipe = 0; pipe < port_param.n_pipes_per_subport; pipe++) {
+		err = rte_sched_pipe_config(port, SUBPORT, pipe, profile_id);
+		TEST_ASSERT_SUCCESS(err, "Error config sched pipe %u, err=%d\n",
+			pipe, err);
+	}
+	return 0;
+}
 /**
  * test main entrance for library sched
  */
@@ -114,7 +142,7 @@ test_sched(void)
 	uint32_t pipe;
 	struct rte_mbuf *in_mbufs[10];
 	struct rte_mbuf *out_mbufs[10];
-	int i;
+	int i, pipe_profile = 0;
 
 	int err;
 
@@ -135,35 +163,42 @@ test_sched(void)
 		TEST_ASSERT_SUCCESS(err, "Error config sched pipe %u, err=%d\n", pipe, err);
 	}
 
-	for (i = 0; i < 10; i++) {
-		in_mbufs[i] = rte_pktmbuf_alloc(mp);
-		TEST_ASSERT_NOT_NULL(in_mbufs[i], "Packet allocation failed\n");
-		prepare_pkt(in_mbufs[i]);
-	}
+	while (pipe_profile < 2) {
 
+		for (i = 0; i < 10; i++) {
+			in_mbufs[i] = rte_pktmbuf_alloc(mp);
+			TEST_ASSERT_NOT_NULL(in_mbufs[i],
+				"Packet allocation failed\n");
+			prepare_pkt(in_mbufs[i]);
+		}
 
-	err = rte_sched_port_enqueue(port, in_mbufs, 10);
-	TEST_ASSERT_EQUAL(err, 10, "Wrong enqueue, err=%d\n", err);
+		err = rte_sched_port_enqueue(port, in_mbufs, 10);
+		TEST_ASSERT_EQUAL(err, 10, "Wrong enqueue, err=%d\n", err);
 
-	err = rte_sched_port_dequeue(port, out_mbufs, 10);
-	TEST_ASSERT_EQUAL(err, 10, "Wrong dequeue, err=%d\n", err);
+		err = rte_sched_port_dequeue(port, out_mbufs, 10);
+		TEST_ASSERT_EQUAL(err, 10, "Wrong dequeue, err=%d\n", err);
 
-	for (i = 0; i < 10; i++) {
-		enum rte_meter_color color;
-		uint32_t subport, traffic_class, queue;
+		for (i = 0; i < 10; i++) {
+			enum rte_meter_color color;
+			uint32_t subport, traffic_class, queue;
 
-		color = rte_sched_port_pkt_read_color(out_mbufs[i]);
-		TEST_ASSERT_EQUAL(color, e_RTE_METER_YELLOW, "Wrong color\n");
+			color = rte_sched_port_pkt_read_color(out_mbufs[i]);
+			TEST_ASSERT_EQUAL(color,
+				e_RTE_METER_YELLOW, "Wrong color\n");
 
-		rte_sched_port_pkt_read_tree_path(out_mbufs[i],
-				&subport, &pipe, &traffic_class, &queue);
+			rte_sched_port_pkt_read_tree_path(out_mbufs[i],
+					&subport,
+					&pipe,
+					&traffic_class,
+					&queue);
 
-		TEST_ASSERT_EQUAL(subport, SUBPORT, "Wrong subport\n");
-		TEST_ASSERT_EQUAL(pipe, PIPE, "Wrong pipe\n");
-		TEST_ASSERT_EQUAL(traffic_class, TC, "Wrong traffic_class\n");
-		TEST_ASSERT_EQUAL(queue, QUEUE, "Wrong queue\n");
+			TEST_ASSERT_EQUAL(subport, SUBPORT, "Wrong subport\n");
+			TEST_ASSERT_EQUAL(pipe, PIPE, "Wrong pipe\n");
+			TEST_ASSERT_EQUAL(traffic_class, TC,
+				"Wrong traffic_class\n");
+			TEST_ASSERT_EQUAL(queue, QUEUE, "Wrong queue\n");
 
-	}
+		}
 
 
 	struct rte_sched_subport_stats subport_stats;
@@ -179,6 +214,18 @@ test_sched(void)
 	TEST_ASSERT_EQUAL(queue_stats.n_pkts, 10, "Wrong queue stats\n");
 #endif
 
+		for (i = 0; i < 10; i++)
+			rte_pktmbuf_free(in_mbufs[i]);
+
+		pipe_profile += 1;
+
+		if (pipe_profile == 1) {
+			err = pipe_profile_set(port, &pipe_profile1);
+			TEST_ASSERT_EQUAL(err, 0,
+				"Profile not added, err=%d\n", err);
+		}
+	}
+
 	rte_sched_port_free(port);
 
 	return 0;
-- 
2.9.3

Re: [PATCH 1/2] librte_sched: add post-init pipe profile api

[Dumitrescu, Cristian]

Hi Jasvinder,

Looks good, a few things to fix below:

> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Friday, March 9, 2018 6:41 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: [PATCH 1/2] librte_sched: add post-init pipe profile api
> 
> Add new API function to add more pipe configuration profiles
> post initialization to the set of exisitng profiles specified during
> the creation of scheduler port.
> 
> This API removes the current limitation that forces the user
> to define the full set of pipe profiles as the part of port parameters
> while port is being created.
> 
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> ---
>  lib/librte_sched/rte_sched.c           | 141
> +++++++++++++++++++++++++++++++++
>  lib/librte_sched/rte_sched.h           |  17 ++++
>  lib/librte_sched/rte_sched_version.map |   6 ++
>  3 files changed, 164 insertions(+)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 634486c..43728ec 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -932,6 +932,147 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
>  	return 0;
>  }
> 
> +static void
> +rte_sched_pipe_profile_get(struct rte_sched_port *port,
> +	struct rte_sched_pipe_params *params,
> +	struct rte_sched_pipe_profile *p)
> +{
> +	uint32_t i;
> +
> +	/* Token Bucket */
> +	if (params->tb_rate == port->rate) {
> +		p->tb_credits_per_period = 1;
> +		p->tb_period = 1;
> +	} else {
> +		double tb_rate = (double) params->tb_rate
> +				/ (double) port->rate;
> +		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> +
> +		rte_approx(tb_rate, d,
> +				   &p->tb_credits_per_period, &p-
> >tb_period);
> +	}
> +
> +	p->tb_size = params->tb_size;
> +
> +	/* Traffic Classes */
> +	p->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
> +		port->rate);
> +
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> +		p->tc_credits_per_period[i]
> +			= rte_sched_time_ms_to_bytes(params->tc_period,
> +				params->tc_rate[i]);
> +
> +#ifdef RTE_SCHED_SUBPORT_TC_OV
> +	p->tc_ov_weight = params->tc_ov_weight;
> +#endif
> +
> +	/* WRR */
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> +		uint32_t
> wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
> +		uint32_t lcd, lcd1, lcd2;
> +		uint32_t qindex;
> +
> +		qindex = i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
> +
> +		wrr_cost[0] = params->wrr_weights[qindex];
> +		wrr_cost[1] = params->wrr_weights[qindex + 1];
> +		wrr_cost[2] = params->wrr_weights[qindex + 2];
> +		wrr_cost[3] = params->wrr_weights[qindex + 3];
> +
> +		lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
> +		lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
> +		lcd = rte_get_lcd(lcd1, lcd2);
> +
> +		wrr_cost[0] = lcd / wrr_cost[0];
> +		wrr_cost[1] = lcd / wrr_cost[1];
> +		wrr_cost[2] = lcd / wrr_cost[2];
> +		wrr_cost[3] = lcd / wrr_cost[3];
> +
> +		p->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
> +		p->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
> +		p->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
> +		p->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
> +	}
> +}

I suggest a slightly different name: convert instead of get; I encourage using dst and src as the names for the args, as it makes the code easier to follow.

Please call this function as part of rte_sched_port_config_pipe_profile_table() to avoid duplicating this code.

> +
> +int
> +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> +	struct rte_sched_pipe_params *params,
> +	int32_t *profile_id)
> +{
> +	struct rte_sched_pipe_profile pp;
> +	uint32_t i;
> +
> +	/* Port */
> +	if (port == NULL)
> +		return -1;
> +
> +	/* Pipe parameters */
> +	if (params == NULL)
> +		return -2;
> +
> +	/* TB rate: non-zero, not greater than port rate */
> +	if (params->tb_rate == 0 ||
> +		params->tb_rate > port->rate)
> +		return -3;
> +
> +	/* TB size: non-zero */
> +	if (params->tb_size == 0)
> +		return -4;
> +
> +	/* TC rate: non-zero, less than pipe rate */
> +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> +		if (params->tc_rate[i] == 0 ||
> +			params->tc_rate[i] > params->tb_rate)
> +			return -5;
> +	}
> +
> +	/* TC period: non-zero */
> +	if (params->tc_period == 0)
> +		return -6;
> +
> +#ifdef RTE_SCHED_SUBPORT_TC_OV
> +	/* TC3 oversubscription weight: non-zero */
> +	if (params->tc_ov_weight == 0)
> +		return -7;
> +#endif
> +
> +	/* Queue WRR weights: non-zero */
> +	for (i = 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
> +		if (params->wrr_weights[i] == 0)
> +			return -8;
> +	}
> +

Please put this checks into a new pipe_profile_check() function and call it from rte_sched_port_check_params() to avoid code duplication.

> +	/* Pipe profiles not exceeds the max limit */
> +	if (port->n_pipe_profiles >= RTE_SCHED_PIPE_PROFILES_PER_PORT)
> +		return -9;
> +
> +	memset(&pp, 0, sizeof(struct rte_sched_pipe_profile));

There is no need to use a temporary local copy, you can work straight on top of the real &port->pipe_profiles[port->n_pipe_profiles], as long as you only increment port->n_pipe_profiles when you are completely sure everything is OK.

> +	rte_sched_pipe_profile_get(port, params, &pp);
> +
> +	/* Pipe profile not exists */
> +	for (i = 0; i < port->n_pipe_profiles; i++) {
> +		if (memcmp(port->pipe_profiles + i, &pp, sizeof(pp)) == 0)
> +			return -10;
> +	}
> +
> +	/* Set port params */
> +	memcpy(port->pipe_profiles + port->n_pipe_profiles, &pp,
> sizeof(pp));

Based on the above comment of pp temp not needed, this memcpy can be eliminated.

> +
> +	uint32_t pipe_tc3_rate = params->tc_rate[3];
> +
> +	if (port->pipe_tc3_rate_max < pipe_tc3_rate)
> +		port->pipe_tc3_rate_max = pipe_tc3_rate;
> +
> +	*profile_id = port->n_pipe_profiles;
> +	port->n_pipe_profiles += 1;

Maybe move this port->n_pipe_profiles++ earlier, just after we performed the "pipe profile exists" check, easier to read the code. I would also put a comment similar to "pipe profile commit" for this.

> +
> +	rte_sched_port_log_pipe_profile(port, *profile_id);
> +
> +	return 0;
> +}
> +
>  void
>  rte_sched_port_pkt_write(struct rte_mbuf *pkt,
>  			 uint32_t subport, uint32_t pipe, uint32_t
> traffic_class,
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index 5d2a688..7edccbe 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -271,6 +271,23 @@ rte_sched_pipe_config(struct rte_sched_port
> *port,
>  	int32_t pipe_profile);
> 
>  /**
> + * Hierarchical scheduler pipe profile add
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param params
> + *   Pipe configuration parameters
> + * @param pipe_profile_id
> + *   Set to valid profile id when profile is added successfully.
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> +	struct rte_sched_pipe_params *params,
> +	int32_t *pipe_profile_id);

Why is pipe_profile_id of type int32_t instead of uint32_t? The port-> n_pipe_profiles is uint32_t.

> +
> +/**
>   * Hierarchical scheduler memory footprint size per port
>   *
>   * @param params
> diff --git a/lib/librte_sched/rte_sched_version.map
> b/lib/librte_sched/rte_sched_version.map
> index 3aa159a..b709cf8 100644
> --- a/lib/librte_sched/rte_sched_version.map
> +++ b/lib/librte_sched/rte_sched_version.map
> @@ -29,3 +29,9 @@ DPDK_2.1 {
>  	rte_sched_port_pkt_read_color;
> 
>  } DPDK_2.0;
> +
> +DPDK_18.05 {
> +	global:
> +
> +	rte_sched_pipe_profile_add;
> +} DPDK_2.1;
> --

We probably need to increment LIBABIVER in the Makefile and bump the .so number in release notes.

> 2.9.3


Thanks very much!

Regards,
Cristian

Re: [PATCH 1/2] librte_sched: add post-init pipe profile api

[Singh, Jasvinder]

<snip>

> > +static void
> > +rte_sched_pipe_profile_get(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	struct rte_sched_pipe_profile *p)
> > +{
> > +	uint32_t i;
> > +
> > +	/* Token Bucket */
> > +	if (params->tb_rate == port->rate) {
> > +		p->tb_credits_per_period = 1;
> > +		p->tb_period = 1;
> > +	} else {
> > +		double tb_rate = (double) params->tb_rate
> > +				/ (double) port->rate;
> > +		double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> > +
> > +		rte_approx(tb_rate, d,
> > +				   &p->tb_credits_per_period, &p-
> > >tb_period);
> > +	}
> > +
> > +	p->tb_size = params->tb_size;
> > +
> > +	/* Traffic Classes */
> > +	p->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
> > +		port->rate);
> > +
> > +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> > +		p->tc_credits_per_period[i]
> > +			= rte_sched_time_ms_to_bytes(params->tc_period,
> > +				params->tc_rate[i]);
> > +
> > +#ifdef RTE_SCHED_SUBPORT_TC_OV
> > +	p->tc_ov_weight = params->tc_ov_weight; #endif
> > +
> > +	/* WRR */
> > +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> > +		uint32_t
> > wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
> > +		uint32_t lcd, lcd1, lcd2;
> > +		uint32_t qindex;
> > +
> > +		qindex = i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
> > +
> > +		wrr_cost[0] = params->wrr_weights[qindex];
> > +		wrr_cost[1] = params->wrr_weights[qindex + 1];
> > +		wrr_cost[2] = params->wrr_weights[qindex + 2];
> > +		wrr_cost[3] = params->wrr_weights[qindex + 3];
> > +
> > +		lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
> > +		lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
> > +		lcd = rte_get_lcd(lcd1, lcd2);
> > +
> > +		wrr_cost[0] = lcd / wrr_cost[0];
> > +		wrr_cost[1] = lcd / wrr_cost[1];
> > +		wrr_cost[2] = lcd / wrr_cost[2];
> > +		wrr_cost[3] = lcd / wrr_cost[3];
> > +
> > +		p->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
> > +		p->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
> > +		p->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
> > +		p->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
> > +	}
> > +}
> 
> I suggest a slightly different name: convert instead of get; I encourage using dst
> and src as the names for the args, as it makes the code easier to follow.
> 
> Please call this function as part of rte_sched_port_config_pipe_profile_table()
> to avoid duplicating this code.


> > +
> > +int
> > +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	int32_t *profile_id)
> > +{
> > +	struct rte_sched_pipe_profile pp;
> > +	uint32_t i;
> > +
> > +	/* Port */
> > +	if (port == NULL)
> > +		return -1;
> > +
> > +	/* Pipe parameters */
> > +	if (params == NULL)
> > +		return -2;
> > +
> > +	/* TB rate: non-zero, not greater than port rate */
> > +	if (params->tb_rate == 0 ||
> > +		params->tb_rate > port->rate)
> > +		return -3;
> > +
> > +	/* TB size: non-zero */
> > +	if (params->tb_size == 0)
> > +		return -4;
> > +
> > +	/* TC rate: non-zero, less than pipe rate */
> > +	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> > +		if (params->tc_rate[i] == 0 ||
> > +			params->tc_rate[i] > params->tb_rate)
> > +			return -5;
> > +	}
> > +
> > +	/* TC period: non-zero */
> > +	if (params->tc_period == 0)
> > +		return -6;
> > +
> > +#ifdef RTE_SCHED_SUBPORT_TC_OV
> > +	/* TC3 oversubscription weight: non-zero */
> > +	if (params->tc_ov_weight == 0)
> > +		return -7;
> > +#endif
> > +
> > +	/* Queue WRR weights: non-zero */
> > +	for (i = 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
> > +		if (params->wrr_weights[i] == 0)
> > +			return -8;
> > +	}
> > +
> 
> Please put this checks into a new pipe_profile_check() function and call it from
> rte_sched_port_check_params() to avoid code duplication.
> 
> > +	/* Pipe profiles not exceeds the max limit */
> > +	if (port->n_pipe_profiles >= RTE_SCHED_PIPE_PROFILES_PER_PORT)
> > +		return -9;
> > +
> > +	memset(&pp, 0, sizeof(struct rte_sched_pipe_profile));
> 
> There is no need to use a temporary local copy, you can work straight on top of
> the real &port->pipe_profiles[port->n_pipe_profiles], as long as you only
> increment port->n_pipe_profiles when you are completely sure everything is
> OK.
> 
> > +	rte_sched_pipe_profile_get(port, params, &pp);
> > +
> > +	/* Pipe profile not exists */
> > +	for (i = 0; i < port->n_pipe_profiles; i++) {
> > +		if (memcmp(port->pipe_profiles + i, &pp, sizeof(pp)) == 0)
> > +			return -10;
> > +	}
> > +
> > +	/* Set port params */
> > +	memcpy(port->pipe_profiles + port->n_pipe_profiles, &pp,
> > sizeof(pp));
> 
> Based on the above comment of pp temp not needed, this memcpy can be
> eliminated.
> 
> > +
> > +	uint32_t pipe_tc3_rate = params->tc_rate[3];
> > +
> > +	if (port->pipe_tc3_rate_max < pipe_tc3_rate)
> > +		port->pipe_tc3_rate_max = pipe_tc3_rate;
> > +
> > +	*profile_id = port->n_pipe_profiles;
> > +	port->n_pipe_profiles += 1;
> 
> Maybe move this port->n_pipe_profiles++ earlier, just after we performed the
> "pipe profile exists" check, easier to read the code. I would also put a comment
> similar to "pipe profile commit" for this.
> 
> > +
> > +	rte_sched_port_log_pipe_profile(port, *profile_id);
> > +
> > +	return 0;
> > +}
> > +
> >  void
> >  rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> >  			 uint32_t subport, uint32_t pipe, uint32_t traffic_class,
> diff
> > --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> > index 5d2a688..7edccbe 100644
> > --- a/lib/librte_sched/rte_sched.h
> > +++ b/lib/librte_sched/rte_sched.h
> > @@ -271,6 +271,23 @@ rte_sched_pipe_config(struct rte_sched_port
> > *port,
> >  	int32_t pipe_profile);
> >
> >  /**
> > + * Hierarchical scheduler pipe profile add
> > + *
> > + * @param port
> > + *   Handle to port scheduler instance
> > + * @param params
> > + *   Pipe configuration parameters
> > + * @param pipe_profile_id
> > + *   Set to valid profile id when profile is added successfully.
> > + * @return
> > + *   0 upon success, error code otherwise
> > + */
> > +int
> > +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	int32_t *pipe_profile_id);
> 
> Why is pipe_profile_id of type int32_t instead of uint32_t? The port->
> n_pipe_profiles is uint32_t.
> 
> > +
> > +/**
> >   * Hierarchical scheduler memory footprint size per port
> >   *
> >   * @param params
> > diff --git a/lib/librte_sched/rte_sched_version.map
> > b/lib/librte_sched/rte_sched_version.map
> > index 3aa159a..b709cf8 100644
> > --- a/lib/librte_sched/rte_sched_version.map
> > +++ b/lib/librte_sched/rte_sched_version.map
> > @@ -29,3 +29,9 @@ DPDK_2.1 {
> >  	rte_sched_port_pkt_read_color;
> >
> >  } DPDK_2.0;
> > +
> > +DPDK_18.05 {
> > +	global:
> > +
> > +	rte_sched_pipe_profile_add;
> > +} DPDK_2.1;
> > --
> 
> We probably need to increment LIBABIVER in the Makefile and bump the .so
> number in release notes.
> 

I will revise the patch and send v2 with suggested changes. Thank you.

Jasvinder

[PATCH v2] librte_sched: add post-init pipe profile api

[Jasvinder Singh]

Add new API function to add more pipe configuration profiles
post initialization to the set of exisitng profiles specified during
the creation of scheduler port.

This API removes the current limitation that forces the user
to define the full set of pipe profiles as the part of port parameters
while port is being created.

Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
---
v2:
- remove duplicated parameter checks
- change api name to rte_sched_port_pipe_profile_add 

 lib/librte_sched/rte_sched.c           | 239 +++++++++++++++++++++------------
 lib/librte_sched/rte_sched.h           |  18 +++
 lib/librte_sched/rte_sched_version.map |   6 +
 3 files changed, 180 insertions(+), 83 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 634486c..8917f24 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -276,9 +276,54 @@ rte_sched_port_qsize(struct rte_sched_port *port, uint32_t qindex)
 }
 
 static int
+pipe_profile_check(struct rte_sched_pipe_params *params,
+	uint32_t rate)
+{
+	uint32_t i;
+
+	/* Pipe parameters */
+	if (params == NULL)
+		return -10;
+
+	/* TB rate: non-zero, not greater than port rate */
+	if (params->tb_rate == 0 ||
+		params->tb_rate > rate)
+		return -11;
+
+	/* TB size: non-zero */
+	if (params->tb_size == 0)
+		return -12;
+
+	/* TC rate: non-zero, less than pipe rate */
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+		if (params->tc_rate[i] == 0 ||
+			params->tc_rate[i] > params->tb_rate)
+			return -13;
+	}
+
+	/* TC period: non-zero */
+	if (params->tc_period == 0)
+		return -14;
+
+#ifdef RTE_SCHED_SUBPORT_TC_OV
+	/* TC3 oversubscription weight: non-zero */
+	if (params->tc_ov_weight == 0)
+		return -15;
+#endif
+
+	/* Queue WRR weights: non-zero */
+	for (i = 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
+		if (params->wrr_weights[i] == 0)
+			return -16;
+	}
+
+	return 0;
+}
+
+static int
 rte_sched_port_check_params(struct rte_sched_port_params *params)
 {
-	uint32_t i, j;
+	uint32_t i;
 
 	if (params == NULL)
 		return -1;
@@ -324,36 +369,11 @@ rte_sched_port_check_params(struct rte_sched_port_params *params)
 
 	for (i = 0; i < params->n_pipe_profiles; i++) {
 		struct rte_sched_pipe_params *p = params->pipe_profiles + i;
+		int status;
 
-		/* TB rate: non-zero, not greater than port rate */
-		if (p->tb_rate == 0 || p->tb_rate > params->rate)
-			return -10;
-
-		/* TB size: non-zero */
-		if (p->tb_size == 0)
-			return -11;
-
-		/* TC rate: non-zero, less than pipe rate */
-		for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++) {
-			if (p->tc_rate[j] == 0 || p->tc_rate[j] > p->tb_rate)
-				return -12;
-		}
-
-		/* TC period: non-zero */
-		if (p->tc_period == 0)
-			return -13;
-
-#ifdef RTE_SCHED_SUBPORT_TC_OV
-		/* TC3 oversubscription weight: non-zero */
-		if (p->tc_ov_weight == 0)
-			return -14;
-#endif
-
-		/* Queue WRR weights: non-zero */
-		for (j = 0; j < RTE_SCHED_QUEUES_PER_PIPE; j++) {
-			if (p->wrr_weights[j] == 0)
-				return -15;
-		}
+		status = pipe_profile_check(p, params->rate);
+		if (status != 0)
+			return status;
 	}
 
 	return 0;
@@ -514,69 +534,80 @@ rte_sched_time_ms_to_bytes(uint32_t time_ms, uint32_t rate)
 }
 
 static void
-rte_sched_port_config_pipe_profile_table(struct rte_sched_port *port, struct rte_sched_port_params *params)
+rte_sched_pipe_profile_convert(struct rte_sched_pipe_params *src,
+	struct rte_sched_pipe_profile *dst,
+	uint32_t rate)
 {
-	uint32_t i, j;
+	uint32_t i;
 
-	for (i = 0; i < port->n_pipe_profiles; i++) {
-		struct rte_sched_pipe_params *src = params->pipe_profiles + i;
-		struct rte_sched_pipe_profile *dst = port->pipe_profiles + 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;
 
-		/* Token Bucket */
-		if (src->tb_rate == params->rate) {
-			dst->tb_credits_per_period = 1;
-			dst->tb_period = 1;
-		} else {
-			double tb_rate = (double) src->tb_rate
-				/ (double) params->rate;
-			double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
-
-			rte_approx(tb_rate, d,
-				   &dst->tb_credits_per_period, &dst->tb_period);
-		}
-		dst->tb_size = src->tb_size;
+		rte_approx(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,
-							    params->rate);
+	/* Traffic Classes */
+	dst->tc_period = rte_sched_time_ms_to_bytes(src->tc_period,
+						rate);
 
-		for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++)
-			dst->tc_credits_per_period[j]
-				= rte_sched_time_ms_to_bytes(src->tc_period,
-							     src->tc_rate[j]);
+	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]);
 
 #ifdef RTE_SCHED_SUBPORT_TC_OV
-		dst->tc_ov_weight = src->tc_ov_weight;
+	dst->tc_ov_weight = src->tc_ov_weight;
 #endif
 
-		/* WRR */
-		for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++) {
-			uint32_t wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
-			uint32_t lcd, lcd1, lcd2;
-			uint32_t qindex;
-
-			qindex = j * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
-
-			wrr_cost[0] = src->wrr_weights[qindex];
-			wrr_cost[1] = src->wrr_weights[qindex + 1];
-			wrr_cost[2] = src->wrr_weights[qindex + 2];
-			wrr_cost[3] = src->wrr_weights[qindex + 3];
-
-			lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
-			lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
-			lcd = rte_get_lcd(lcd1, lcd2);
-
-			wrr_cost[0] = lcd / wrr_cost[0];
-			wrr_cost[1] = lcd / wrr_cost[1];
-			wrr_cost[2] = lcd / wrr_cost[2];
-			wrr_cost[3] = lcd / wrr_cost[3];
-
-			dst->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
-			dst->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
-			dst->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
-			dst->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
-		}
+	/* WRR */
+	for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
+		uint32_t wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
+		uint32_t lcd, lcd1, lcd2;
+		uint32_t qindex;
+
+		qindex = i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
+
+		wrr_cost[0] = src->wrr_weights[qindex];
+		wrr_cost[1] = src->wrr_weights[qindex + 1];
+		wrr_cost[2] = src->wrr_weights[qindex + 2];
+		wrr_cost[3] = src->wrr_weights[qindex + 3];
+
+		lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
+		lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
+		lcd = rte_get_lcd(lcd1, lcd2);
+
+		wrr_cost[0] = lcd / wrr_cost[0];
+		wrr_cost[1] = lcd / wrr_cost[1];
+		wrr_cost[2] = lcd / wrr_cost[2];
+		wrr_cost[3] = lcd / wrr_cost[3];
+
+		dst->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
+		dst->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
+		dst->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
+		dst->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
+	}
+}
+
+static void
+rte_sched_port_config_pipe_profile_table(struct rte_sched_port *port,
+	struct rte_sched_port_params *params)
+{
+	uint32_t i;
+
+	for (i = 0; i < port->n_pipe_profiles; i++) {
+		struct rte_sched_pipe_params *src = params->pipe_profiles + i;
+		struct rte_sched_pipe_profile *dst = port->pipe_profiles + i;
 
+		rte_sched_pipe_profile_convert(src, dst, params->rate);
 		rte_sched_port_log_pipe_profile(port, i);
 	}
 
@@ -932,6 +963,48 @@ rte_sched_pipe_config(struct rte_sched_port *port,
 	return 0;
 }
 
+int
+rte_sched_port_pipe_profile_add(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params,
+	uint32_t *pipe_profile_id)
+{
+	struct rte_sched_pipe_profile *pp;
+	uint32_t i;
+	int status;
+
+	/* Port */
+	if (port == NULL)
+		return -1;
+
+	/* Pipe profiles not exceeds the max limit */
+	if (port->n_pipe_profiles >= RTE_SCHED_PIPE_PROFILES_PER_PORT)
+		return -2;
+
+	/* Pipe params */
+	status = pipe_profile_check(params, port->rate);
+	if (status != 0)
+		return status;
+
+	pp = &port->pipe_profiles[port->n_pipe_profiles];
+	rte_sched_pipe_profile_convert(params, pp, port->rate);
+
+	/* Pipe profile not exists */
+	for (i = 0; i < port->n_pipe_profiles; i++)
+		if (memcmp(port->pipe_profiles + i, pp, sizeof(*pp)) == 0)
+			return -3;
+
+	/* Pipe profile commit */
+	*pipe_profile_id = port->n_pipe_profiles;
+	port->n_pipe_profiles++;
+
+	if (port->pipe_tc3_rate_max < params->tc_rate[3])
+		port->pipe_tc3_rate_max = params->tc_rate[3];
+
+	rte_sched_port_log_pipe_profile(port, *pipe_profile_id);
+
+	return 0;
+}
+
 void
 rte_sched_port_pkt_write(struct rte_mbuf *pkt,
 			 uint32_t subport, uint32_t pipe, uint32_t traffic_class,
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index 5d2a688..b3d508f 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -57,6 +57,7 @@ extern "C" {
  */
 
 #include <sys/types.h>
+#include <rte_compat.h>
 #include <rte_mbuf.h>
 #include <rte_meter.h>
 
@@ -234,6 +235,23 @@ void
 rte_sched_port_free(struct rte_sched_port *port);
 
 /**
+ * Hierarchical scheduler pipe profile add
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param params
+ *   Pipe profile parameters
+ * @param pipe_profile_id
+ *   Set to valid profile id when profile is added successfully.
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int __rte_experimental
+rte_sched_port_pipe_profile_add(struct rte_sched_port *port,
+	struct rte_sched_pipe_params *params,
+	uint32_t *pipe_profile_id);
+
+/**
  * Hierarchical scheduler subport configuration
  *
  * @param port
diff --git a/lib/librte_sched/rte_sched_version.map b/lib/librte_sched/rte_sched_version.map
index 3aa159a..1530af1 100644
--- a/lib/librte_sched/rte_sched_version.map
+++ b/lib/librte_sched/rte_sched_version.map
@@ -29,3 +29,9 @@ DPDK_2.1 {
 	rte_sched_port_pkt_read_color;
 
 } DPDK_2.0;
+
+EXPERIMENTAL {
+	global:
+
+	rte_sched_port_pipe_profile_add;
+} DPDK_2.1;
-- 
2.9.3

Re: [PATCH v2] librte_sched: add post-init pipe profile api

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Friday, May 4, 2018 3:10 PM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Subject: [PATCH v2] librte_sched: add post-init pipe profile api
> 
> Add new API function to add more pipe configuration profiles
> post initialization to the set of exisitng profiles specified during
> the creation of scheduler port.
> 
> This API removes the current limitation that forces the user
> to define the full set of pipe profiles as the part of port parameters
> while port is being created.
> 
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> ---
> v2:
> - remove duplicated parameter checks
> - change api name to rte_sched_port_pipe_profile_add
> 

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Applied to next-qos tree, thanks!