diff mbox series

[v6] sched: enable traffic class oversubscription conditionally

Message ID 20220530084520.827724-1-marcinx.danilewicz@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v6] sched: enable traffic class oversubscription conditionally |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Danilewicz, MarcinX May 30, 2022, 8:45 a.m. UTC 
  Added new API to enable or disable TC over subscription for best
effort traffic class at subport level.
Added changes after review and increased throughput.

By default TC OV is disabled.
History:
- v1 - TC OV disabled by default
- v2 - throughput improvements
- v3, v4, v5 - changes from comments
- v6 - removed rte_sched_subport_tc_ov_config declaration and map

Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
---
 lib/sched/rte_sched.c | 96 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 3 deletions(-)

Comments

Cristian Dumitrescu May 30, 2022, 10:35 a.m. UTC | #1 
> -----Original Message-----
> From: Danilewicz, MarcinX <marcinx.danilewicz@intel.com>
> Sent: Monday, May 30, 2022 9:45 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Ajmera, Megha <megha.ajmera@intel.com>
> Subject: [PATCH v6] sched: enable traffic class oversubscription conditionally
> 
> Added new API to enable or disable TC over subscription for best
> effort traffic class at subport level.
> Added changes after review and increased throughput.
> 
> By default TC OV is disabled.
> History:
> - v1 - TC OV disabled by default
> - v2 - throughput improvements
> - v3, v4, v5 - changes from comments
> - v6 - removed rte_sched_subport_tc_ov_config declaration and map
> 
> Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
> ---
>  lib/sched/rte_sched.c | 96
> +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 

Marcin,

This latest version of your patch only contains changes in rte_sched.c file. Since there is no new API in rte_sched.h, how are you going to configure the traffic class oversubscription feature?

Regards,
Cristian
Cristian Dumitrescu May 30, 2022, 10:54 a.m. UTC | #2 
Hi Marcin,

Comments inline below.

<snip>

> @@ -2403,8 +2481,16 @@ grinder_schedule(struct rte_sched_port *port,
>  	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
>  	uint32_t be_tc_active;
> 
> -	if (!grinder_credits_check(port, subport, pos))
> -		return 0;
> +	switch (subport->tc_ov_enabled) {
> +	case 1:
> +		if (!grinder_credits_check_with_tc_ov(port, subport, pos))
> +			return 0;
> +		break;
> +	case 0:
> +		if (!grinder_credits_check(port, subport, pos))
> +			return 0;
> +		break;
> +	}
> 

Using a switch statement for a binary condition instead of if-else does not make sense to me. I know you mention you saw better performance with the switch, but I am pretty sure it is not the switch providing the performance increase. You are using if-else for testing the new subport->tc_ov_enabled throughout the code (an example is just below in your patch), so I suggest you do the same here:

if (subport->tc_ov_enabled) {
	if (!grinder_credits_check_with_tc_ov(port, subport, pos))
		return 0;
} else {
	if (!grinder_credits_check(port, subport, pos))
		return 0;
}

>  	/* Advance port time */
>  	port->time += pkt_len;
> @@ -2770,7 +2856,11 @@ grinder_handle(struct rte_sched_port *port,
>  						subport->profile;
> 
>  		grinder_prefetch_tc_queue_arrays(subport, pos);
> -		grinder_credits_update(port, subport, pos);
> +
> +		if (subport->tc_ov_enabled)
> +			grinder_credits_update_with_tc_ov(port, subport,
> pos);
> +		else
> +			grinder_credits_update(port, subport, pos);
> 
>  		grinder->state = e_GRINDER_PREFETCH_MBUF;
>  		return 0;
> --
> 2.25.1

Regards,
Cristian
Cristian Dumitrescu May 30, 2022, 10:58 a.m. UTC | #3 
> -----Original Message-----
> From: Danilewicz, MarcinX <marcinx.danilewicz@intel.com>
> Sent: Monday, May 30, 2022 9:45 AM
> To: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Ajmera, Megha <megha.ajmera@intel.com>
> Subject: [PATCH v6] sched: enable traffic class oversubscription conditionally
> 
> Added new API to enable or disable TC over subscription for best
> effort traffic class at subport level.
> Added changes after review and increased throughput.
> 
> By default TC OV is disabled.
> History:
> - v1 - TC OV disabled by default
> - v2 - throughput improvements
> - v3, v4, v5 - changes from comments
> - v6 - removed rte_sched_subport_tc_ov_config declaration and map

The place of the history log is not here. If you put it here, it will be picked up by the commit, which is not what you want.

> 
> Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
> ---

This is the place to put the history log.

>  lib/sched/rte_sched.c | 96
> +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
>
Danilewicz, MarcinX May 30, 2022, 11:59 a.m. UTC | #4 
Hi Cristian,

Thank you for comments. Please find my response inline.

> > Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
> > ---
> >  lib/sched/rte_sched.c | 96
> > +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 93 insertions(+), 3 deletions(-)
> >
> This latest version of your patch only contains changes in rte_sched.c file.
> Since there is no new API in rte_sched.h, how are you going to configure the
> traffic class oversubscription feature?
> 

I agree, that's my omission reviewing my changes.

BR,
/Marcin
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
Danilewicz, MarcinX May 30, 2022, 12:02 p.m. UTC | #5 
Hi Cristian,



> 
> Using a switch statement for a binary condition instead of if-else does not
> make sense to me. I know you mention you saw better performance with the
> switch, but I am pretty sure it is not the switch providing the performance
> increase. You are using if-else for testing the new subport->tc_ov_enabled
> throughout the code (an example is just below in your patch), so I suggest
> you do the same here:
> 
> if (subport->tc_ov_enabled) {
> 	if (!grinder_credits_check_with_tc_ov(port, subport, pos))
> 		return 0;
> } else {
> 	if (!grinder_credits_check(port, subport, pos))
> 		return 0;
> 

I've changed this snip to what you requested, but I've spent some time on this comparing, if, unlike, switch and switch-case was the fastest. We can focus on that later on.. because there is clear difference.

Regards,
/Marcin
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
Danilewicz, MarcinX May 30, 2022, 12:04 p.m. UTC | #6 
Hi Cristian,


> > By default TC OV is disabled.
> > History:
> > - v1 - TC OV disabled by default
> > - v2 - throughput improvements
> > - v3, v4, v5 - changes from comments
> > - v6 - removed rte_sched_subport_tc_ov_config declaration and map
> 
> The place of the history log is not here. If you put it here, it will be picked up
> by the commit, which is not what you want.
> 
> >
> > Signed-off-by: Marcin Danilewicz <marcinx.danilewicz@intel.com>
> > ---
> 
> This is the place to put the history log.

It's been moved where it should be.

Regards,
/Marcin
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
diff mbox series

Patch

diff --git a/lib/sched/rte_sched.c b/lib/sched/rte_sched.c
index ec74bee939..eb71b27c99 100644
--- a/lib/sched/rte_sched.c
+++ b/lib/sched/rte_sched.c
@@ -213,6 +213,9 @@  struct rte_sched_subport {
 	uint8_t *bmp_array;
 	struct rte_mbuf **queue_array;
 	uint8_t memory[0] __rte_cache_aligned;
+
+	/* TC oversubscription activation */
+	int tc_ov_enabled;
 } __rte_cache_aligned;
 
 struct rte_sched_port {
@@ -1254,6 +1257,9 @@  rte_sched_subport_config(struct rte_sched_port *port,
 		s->n_pipe_profiles = params->n_pipe_profiles;
 		s->n_max_pipe_profiles = params->n_max_pipe_profiles;
 
+		/* TC over-subscription is enabled by default */
+		s->tc_ov_enabled = 1;
+
 #ifdef RTE_SCHED_CMAN
 		if (params->cman_params != NULL) {
 			s->cman_enabled = true;
@@ -2318,6 +2324,45 @@  grinder_credits_update(struct rte_sched_port *port,
 	pipe->tb_credits = RTE_MIN(pipe->tb_credits, params->tb_size);
 	pipe->tb_time += n_periods * params->tb_period;
 
+	/* Subport TCs */
+	if (unlikely(port->time >= subport->tc_time)) {
+		for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
+			subport->tc_credits[i] = sp->tc_credits_per_period[i];
+
+		subport->tc_time = port->time + sp->tc_period;
+	}
+
+	/* Pipe TCs */
+	if (unlikely(port->time >= pipe->tc_time)) {
+		for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
+			pipe->tc_credits[i] = params->tc_credits_per_period[i];
+		pipe->tc_time = port->time + params->tc_period;
+	}
+}
+
+static inline void
+grinder_credits_update_with_tc_ov(struct rte_sched_port *port,
+	struct rte_sched_subport *subport, uint32_t pos)
+{
+	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 *sp = grinder->subport_params;
+	uint64_t n_periods;
+	uint32_t i;
+
+	/* Subport TB */
+	n_periods = (port->time - subport->tb_time) / sp->tb_period;
+	subport->tb_credits += n_periods * sp->tb_credits_per_period;
+	subport->tb_credits = RTE_MIN(subport->tb_credits, sp->tb_size);
+	subport->tb_time += n_periods * sp->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_MIN(pipe->tb_credits, params->tb_size);
+	pipe->tb_time += n_periods * params->tb_period;
+
 	/* Subport TCs */
 	if (unlikely(port->time >= subport->tc_time)) {
 		subport->tc_ov_wm =
@@ -2348,6 +2393,39 @@  grinder_credits_update(struct rte_sched_port *port,
 static inline int
 grinder_credits_check(struct rte_sched_port *port,
 	struct rte_sched_subport *subport, uint32_t pos)
+{
+	struct rte_sched_grinder *grinder = subport->grinder + pos;
+	struct rte_sched_pipe *pipe = grinder->pipe;
+	struct rte_mbuf *pkt = grinder->pkt;
+	uint32_t tc_index = grinder->tc_index;
+	uint64_t pkt_len = pkt->pkt_len + port->frame_overhead;
+	uint64_t subport_tb_credits = subport->tb_credits;
+	uint64_t subport_tc_credits = subport->tc_credits[tc_index];
+	uint64_t pipe_tb_credits = pipe->tb_credits;
+	uint64_t pipe_tc_credits = pipe->tc_credits[tc_index];
+	int enough_credits;
+
+	/* Check pipe and subport credits */
+	enough_credits = (pkt_len <= subport_tb_credits) &&
+		(pkt_len <= subport_tc_credits) &&
+		(pkt_len <= pipe_tb_credits) &&
+		(pkt_len <= pipe_tc_credits);
+
+	if (!enough_credits)
+		return 0;
+
+	/* Update pipe and subport credits */
+	subport->tb_credits -= pkt_len;
+	subport->tc_credits[tc_index] -= pkt_len;
+	pipe->tb_credits -= pkt_len;
+	pipe->tc_credits[tc_index] -= pkt_len;
+
+	return 1;
+}
+
+static inline int
+grinder_credits_check_with_tc_ov(struct rte_sched_port *port,
+	struct rte_sched_subport *subport, uint32_t pos)
 {
 	struct rte_sched_grinder *grinder = subport->grinder + pos;
 	struct rte_sched_pipe *pipe = grinder->pipe;
@@ -2403,8 +2481,16 @@  grinder_schedule(struct rte_sched_port *port,
 	uint32_t pkt_len = pkt->pkt_len + port->frame_overhead;
 	uint32_t be_tc_active;
 
-	if (!grinder_credits_check(port, subport, pos))
-		return 0;
+	switch (subport->tc_ov_enabled) {
+	case 1:
+		if (!grinder_credits_check_with_tc_ov(port, subport, pos))
+			return 0;
+		break;
+	case 0:
+		if (!grinder_credits_check(port, subport, pos))
+			return 0;
+		break;
+	}
 
 	/* Advance port time */
 	port->time += pkt_len;
@@ -2770,7 +2856,11 @@  grinder_handle(struct rte_sched_port *port,
 						subport->profile;
 
 		grinder_prefetch_tc_queue_arrays(subport, pos);
-		grinder_credits_update(port, subport, pos);
+
+		if (subport->tc_ov_enabled)
+			grinder_credits_update_with_tc_ov(port, subport, pos);
+		else
+			grinder_credits_update(port, subport, pos);
 
 		grinder->state = e_GRINDER_PREFETCH_MBUF;
 		return 0;