DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] sched: fix port time rounding error
@ 2020-04-16  8:48 alangordondewar
  2020-04-17 21:19 ` Dumitrescu, Cristian
  2020-06-25  9:59 ` [dpdk-dev] [PATCH v2] " alangordondewar
  0 siblings, 2 replies; 14+ messages in thread
From: alangordondewar @ 2020-04-16  8:48 UTC (permalink / raw)
  To: cristian.dumitrescu; +Cc: dev, Alan Dewar

From: Alan Dewar <alan.dewar@att.com>

The QoS scheduler works off port time that is computed from the number
of CPU cycles that have elapsed since the last time the port was
polled.   It divides the number of elapsed cycles to calculate how
many bytes can be sent, however this division can generate rounding
errors, where some fraction of a byte sent may be lost.

Lose enough of these fractional bytes and the QoS scheduler
underperforms.  The problem is worse with low bandwidths.

To compensate for this rounding error this fix doesn't advance the
port's time_cpu_cycles by the number of cycles that have elapsed,
but by multiplying the computed number of bytes that can be sent
(which has been rounded down) by number of cycles per byte.
This will mean that port's time_cpu_cycles will lag behind the CPU
cycles momentarily.  At the next poll, the lag will be taken into
account.

Fixes: de3cfa2c98 ("sched: initial import")

Signed-off-by: Alan Dewar <alan.dewar@att.com>
---
 lib/librte_sched/rte_sched.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index c0983ddda..c656dba2d 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -222,6 +222,7 @@ struct rte_sched_port {
 	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
 	uint64_t time;                /* Current NIC TX time measured in bytes */
 	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
+	uint64_t cycles_per_byte;
 
 	/* Grinders */
 	struct rte_mbuf **pkts_out;
@@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
 	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
 		/ params->rate;
 	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
+	port->cycles_per_byte = cycles_per_byte;
 
 	/* Grinders */
 	port->pkts_out = NULL;
@@ -2673,20 +2675,26 @@ static inline void
 rte_sched_port_time_resync(struct rte_sched_port *port)
 {
 	uint64_t cycles = rte_get_tsc_cycles();
-	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
+	uint64_t cycles_diff;
 	uint64_t bytes_diff;
 	uint32_t i;
 
+	if (cycles < port->time_cpu_cycles)
+		goto end;
+
+	cycles_diff = cycles - port->time_cpu_cycles;
 	/* Compute elapsed time in bytes */
 	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
 					   port->inv_cycles_per_byte);
 
 	/* Advance port time */
-	port->time_cpu_cycles = cycles;
+	port->time_cpu_cycles +=
+		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
 	port->time_cpu_bytes += bytes_diff;
 	if (port->time < port->time_cpu_bytes)
 		port->time = port->time_cpu_bytes;
 
+end:
 	/* Reset pipe loop detection */
 	for (i = 0; i < port->n_subports_per_port; i++)
 		port->subports[i]->pipe_loop = RTE_SCHED_PIPE_INVALID;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-04-16  8:48 [dpdk-dev] [PATCH] sched: fix port time rounding error alangordondewar
@ 2020-04-17 21:19 ` Dumitrescu, Cristian
  2020-04-20 11:23   ` Singh, Jasvinder
  2020-06-25  9:59 ` [dpdk-dev] [PATCH v2] " alangordondewar
  1 sibling, 1 reply; 14+ messages in thread
From: Dumitrescu, Cristian @ 2020-04-17 21:19 UTC (permalink / raw)
  To: alangordondewar; +Cc: dev, Alan Dewar, Singh, Jasvinder



> -----Original Message-----
> From: alangordondewar@gmail.com <alangordondewar@gmail.com>
> Sent: Thursday, April 16, 2020 9:48 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>
> Subject: [PATCH] sched: fix port time rounding error
> 
> From: Alan Dewar <alan.dewar@att.com>
> 
> The QoS scheduler works off port time that is computed from the number
> of CPU cycles that have elapsed since the last time the port was
> polled.   It divides the number of elapsed cycles to calculate how
> many bytes can be sent, however this division can generate rounding
> errors, where some fraction of a byte sent may be lost.
> 
> Lose enough of these fractional bytes and the QoS scheduler
> underperforms.  The problem is worse with low bandwidths.
> 
> To compensate for this rounding error this fix doesn't advance the
> port's time_cpu_cycles by the number of cycles that have elapsed,
> but by multiplying the computed number of bytes that can be sent
> (which has been rounded down) by number of cycles per byte.
> This will mean that port's time_cpu_cycles will lag behind the CPU
> cycles momentarily.  At the next poll, the lag will be taken into
> account.
> 
> Fixes: de3cfa2c98 ("sched: initial import")
> 
> Signed-off-by: Alan Dewar <alan.dewar@att.com>
> ---
>  lib/librte_sched/rte_sched.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index c0983ddda..c656dba2d 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -222,6 +222,7 @@ struct rte_sched_port {
>  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> */
>  	uint64_t time;                /* Current NIC TX time measured in bytes */
>  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
> +	uint64_t cycles_per_byte;
> 
>  	/* Grinders */
>  	struct rte_mbuf **pkts_out;
> @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>  		/ params->rate;
>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> +	port->cycles_per_byte = cycles_per_byte;
> 
>  	/* Grinders */
>  	port->pkts_out = NULL;
> @@ -2673,20 +2675,26 @@ static inline void
>  rte_sched_port_time_resync(struct rte_sched_port *port)
>  {
>  	uint64_t cycles = rte_get_tsc_cycles();
> -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> +	uint64_t cycles_diff;
>  	uint64_t bytes_diff;
>  	uint32_t i;
> 
> +	if (cycles < port->time_cpu_cycles)
> +		goto end;
> +
> +	cycles_diff = cycles - port->time_cpu_cycles;
>  	/* Compute elapsed time in bytes */
>  	bytes_diff = rte_reciprocal_divide(cycles_diff <<
> RTE_SCHED_TIME_SHIFT,
>  					   port->inv_cycles_per_byte);
> 
>  	/* Advance port time */
> -	port->time_cpu_cycles = cycles;
> +	port->time_cpu_cycles +=
> +		(bytes_diff * port->cycles_per_byte) >>
> RTE_SCHED_TIME_SHIFT;
>  	port->time_cpu_bytes += bytes_diff;
>  	if (port->time < port->time_cpu_bytes)
>  		port->time = port->time_cpu_bytes;
> 
> +end:
>  	/* Reset pipe loop detection */
>  	for (i = 0; i < port->n_subports_per_port; i++)
>  		port->subports[i]->pipe_loop = RTE_SCHED_PIPE_INVALID;
> --
> 2.17.1

Adding Jasvinder.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-04-17 21:19 ` Dumitrescu, Cristian
@ 2020-04-20 11:23   ` Singh, Jasvinder
  2020-04-21  8:21     ` Dewar, Alan
  0 siblings, 1 reply; 14+ messages in thread
From: Singh, Jasvinder @ 2020-04-20 11:23 UTC (permalink / raw)
  To: Dumitrescu, Cristian, alangordondewar; +Cc: dev, Alan Dewar



> -----Original Message-----
> From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Sent: Friday, April 17, 2020 10:19 PM
> To: alangordondewar@gmail.com
> Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Subject: RE: [PATCH] sched: fix port time rounding error
> 
> 
> 
> > -----Original Message-----
> > From: alangordondewar@gmail.com <alangordondewar@gmail.com>
> > Sent: Thursday, April 16, 2020 9:48 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>
> > Subject: [PATCH] sched: fix port time rounding error
> >
> > From: Alan Dewar <alan.dewar@att.com>
> >
> > The QoS scheduler works off port time that is computed from the number
> > of CPU cycles that have elapsed since the last time the port was
> > polled.   It divides the number of elapsed cycles to calculate how
> > many bytes can be sent, however this division can generate rounding
> > errors, where some fraction of a byte sent may be lost.
> >
> > Lose enough of these fractional bytes and the QoS scheduler
> > underperforms.  The problem is worse with low bandwidths.
> >
> > To compensate for this rounding error this fix doesn't advance the
> > port's time_cpu_cycles by the number of cycles that have elapsed, but
> > by multiplying the computed number of bytes that can be sent (which
> > has been rounded down) by number of cycles per byte.
> > This will mean that port's time_cpu_cycles will lag behind the CPU
> > cycles momentarily.  At the next poll, the lag will be taken into
> > account.
> >
> > Fixes: de3cfa2c98 ("sched: initial import")
> >
> > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > ---
> >  lib/librte_sched/rte_sched.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_sched/rte_sched.c
> > b/lib/librte_sched/rte_sched.c index c0983ddda..c656dba2d 100644
> > --- a/lib/librte_sched/rte_sched.c
> > +++ b/lib/librte_sched/rte_sched.c
> > @@ -222,6 +222,7 @@ struct rte_sched_port {
> >  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> > */
> >  	uint64_t time;                /* Current NIC TX time measured in bytes */
> >  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
> > +	uint64_t cycles_per_byte;
> >
> >  	/* Grinders */
> >  	struct rte_mbuf **pkts_out;
> > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> rte_sched_port_params
> > *params)
> >  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> >  		/ params->rate;
> >  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> > +	port->cycles_per_byte = cycles_per_byte;
> >
> >  	/* Grinders */
> >  	port->pkts_out = NULL;
> > @@ -2673,20 +2675,26 @@ static inline void
> > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> >  	uint64_t cycles = rte_get_tsc_cycles();
> > -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > +	uint64_t cycles_diff;
> >  	uint64_t bytes_diff;
> >  	uint32_t i;
> >
> > +	if (cycles < port->time_cpu_cycles)
> > +		goto end;

Above check seems redundant as port->time_cpu_cycles will always be less than the current cycles due to roundoff in previous iteration.


> > +	cycles_diff = cycles - port->time_cpu_cycles;
> >  	/* Compute elapsed time in bytes */
> >  	bytes_diff = rte_reciprocal_divide(cycles_diff <<
> > RTE_SCHED_TIME_SHIFT,
> >  					   port->inv_cycles_per_byte);
> >
> >  	/* Advance port time */
> > -	port->time_cpu_cycles = cycles;
> > +	port->time_cpu_cycles +=
> > +		(bytes_diff * port->cycles_per_byte) >>
> > RTE_SCHED_TIME_SHIFT;
> >  	port->time_cpu_bytes += bytes_diff;
> >  	if (port->time < port->time_cpu_bytes)
> >  		port->time = port->time_cpu_bytes;
> >
> > +end:
> >  	/* Reset pipe loop detection */
> >  	for (i = 0; i < port->n_subports_per_port; i++)
> >  		port->subports[i]->pipe_loop = RTE_SCHED_PIPE_INVALID;
> > --
> > 2.17.1
> 
> Adding Jasvinder.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-04-20 11:23   ` Singh, Jasvinder
@ 2020-04-21  8:21     ` Dewar, Alan
  2020-06-24 22:50       ` Thomas Monjalon
  0 siblings, 1 reply; 14+ messages in thread
From: Dewar, Alan @ 2020-04-21  8:21 UTC (permalink / raw)
  To: 'Singh, Jasvinder', 'Dumitrescu, Cristian',
	'alangordondewar@gmail.com'
  Cc: 'dev@dpdk.org', 'Alan Dewar'

> -----Original Message-----
> From: Singh, Jasvinder <jasvinder.singh@intel.com> 
> Sent: Monday, April 20, 2020 12:23 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; alangordondewar@gmail.com
> Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>
> Subject: RE: [PATCH] sched: fix port time rounding error
> 
> 
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Sent: Friday, April 17, 2020 10:19 PM
> > To: alangordondewar@gmail.com
> > Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>; Singh, Jasvinder 
> > <jasvinder.singh@intel.com>
> > Subject: RE: [PATCH] sched: fix port time rounding error
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: alangordondewar@gmail.com <alangordondewar@gmail.com>
> > > Sent: Thursday, April 16, 2020 9:48 AM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>
> > > Subject: [PATCH] sched: fix port time rounding error
> > >
> > > From: Alan Dewar <alan.dewar@att.com>
> > >
> > > The QoS scheduler works off port time that is computed from the 
> > > number of CPU cycles that have elapsed since the last time the port was
> > > polled.   It divides the number of elapsed cycles to calculate how
> > > many bytes can be sent, however this division can generate rounding 
> > > errors, where some fraction of a byte sent may be lost.
> > >
> > > Lose enough of these fractional bytes and the QoS scheduler 
> > > underperforms.  The problem is worse with low bandwidths.
> > >
> > > To compensate for this rounding error this fix doesn't advance the 
> > > port's time_cpu_cycles by the number of cycles that have elapsed, 
> > > but by multiplying the computed number of bytes that can be sent 
> > > (which has been rounded down) by number of cycles per byte.
> > > This will mean that port's time_cpu_cycles will lag behind the CPU 
> > > cycles momentarily.  At the next poll, the lag will be taken into 
> > > account.
> > >
> > > Fixes: de3cfa2c98 ("sched: initial import")
> > >
> > > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > > ---
> > >  lib/librte_sched/rte_sched.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_sched/rte_sched.c 
> > > b/lib/librte_sched/rte_sched.c index c0983ddda..c656dba2d 100644
> > > --- a/lib/librte_sched/rte_sched.c
> > > +++ b/lib/librte_sched/rte_sched.c
> > > @@ -222,6 +222,7 @@ struct rte_sched_port {
> > >  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> > > */
> > >  	uint64_t time;                /* Current NIC TX time measured in bytes */
> > >  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte 
> > > */
> > > +	uint64_t cycles_per_byte;
> > >
> > >  	/* Grinders */
> > >  	struct rte_mbuf **pkts_out;
> > > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> > rte_sched_port_params
> > > *params)
> > >  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> > >  		/ params->rate;
> > >  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> > > +	port->cycles_per_byte = cycles_per_byte;
> > >
> > >  	/* Grinders */
> > >  	port->pkts_out = NULL;
> > > @@ -2673,20 +2675,26 @@ static inline void 
> > > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> > >  	uint64_t cycles = rte_get_tsc_cycles();
> > > -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > > +	uint64_t cycles_diff;
> > >  	uint64_t bytes_diff;
> > >  	uint32_t i;
> > >
> > > +	if (cycles < port->time_cpu_cycles)
> > > +		goto end;
> 
> Above check seems redundant as port->time_cpu_cycles will always be less than the current cycles due to roundoff in previous iteration.
> 

This was to catch the condition where the cycles wraps back to zero (after 100+ years?? depending on clock speed).  
Rather than just going to end: the conditional should at least reset port->time_cpu_cycles back to zero.
So there would be a very temporary glitch in accuracy once every 100+ years. 

>
> > > +	cycles_diff = cycles - port->time_cpu_cycles;
> > >  	/* Compute elapsed time in bytes */
> > >  	bytes_diff = rte_reciprocal_divide(cycles_diff << 
> > > RTE_SCHED_TIME_SHIFT,
> > >  					   port->inv_cycles_per_byte);
> > >
> > >  	/* Advance port time */
> > > -	port->time_cpu_cycles = cycles;
> > > +	port->time_cpu_cycles +=
> > > +		(bytes_diff * port->cycles_per_byte) >>
> > > RTE_SCHED_TIME_SHIFT;
> > >  	port->time_cpu_bytes += bytes_diff;
> > >  	if (port->time < port->time_cpu_bytes)
> > >  		port->time = port->time_cpu_bytes;
> > >
> > > +end:
> > >  	/* Reset pipe loop detection */
> > >  	for (i = 0; i < port->n_subports_per_port; i++)
> > >  		port->subports[i]->pipe_loop = RTE_SCHED_PIPE_INVALID;
> > > --
> > > 2.17.1
> > 
> > Adding Jasvinder.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-04-21  8:21     ` Dewar, Alan
@ 2020-06-24 22:50       ` Thomas Monjalon
  2020-06-25  8:32         ` Singh, Jasvinder
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2020-06-24 22:50 UTC (permalink / raw)
  To: 'Singh, Jasvinder'
  Cc: 'Dumitrescu, Cristian',
	'alangordondewar@gmail.com', dev, 'Alan Dewar',
	Dewar, Alan

Jasvinder, what is the conclusion of this patch?

21/04/2020 10:21, Dewar, Alan:
> From: Singh, Jasvinder <jasvinder.singh@intel.com> 
> > > > From: Alan Dewar <alan.dewar@att.com>
> > > >
> > > > The QoS scheduler works off port time that is computed from the 
> > > > number of CPU cycles that have elapsed since the last time the port was
> > > > polled.   It divides the number of elapsed cycles to calculate how
> > > > many bytes can be sent, however this division can generate rounding 
> > > > errors, where some fraction of a byte sent may be lost.
> > > >
> > > > Lose enough of these fractional bytes and the QoS scheduler 
> > > > underperforms.  The problem is worse with low bandwidths.
> > > >
> > > > To compensate for this rounding error this fix doesn't advance the 
> > > > port's time_cpu_cycles by the number of cycles that have elapsed, 
> > > > but by multiplying the computed number of bytes that can be sent 
> > > > (which has been rounded down) by number of cycles per byte.
> > > > This will mean that port's time_cpu_cycles will lag behind the CPU 
> > > > cycles momentarily.  At the next poll, the lag will be taken into 
> > > > account.
> > > >
> > > > Fixes: de3cfa2c98 ("sched: initial import")
> > > >
> > > > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > > > ---
> > > >  lib/librte_sched/rte_sched.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/librte_sched/rte_sched.c 
> > > > b/lib/librte_sched/rte_sched.c index c0983ddda..c656dba2d 100644
> > > > --- a/lib/librte_sched/rte_sched.c
> > > > +++ b/lib/librte_sched/rte_sched.c
> > > > @@ -222,6 +222,7 @@ struct rte_sched_port {
> > > >  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> > > > */
> > > >  	uint64_t time;                /* Current NIC TX time measured in bytes */
> > > >  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte 
> > > > */
> > > > +	uint64_t cycles_per_byte;
> > > >
> > > >  	/* Grinders */
> > > >  	struct rte_mbuf **pkts_out;
> > > > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> > > rte_sched_port_params
> > > > *params)
> > > >  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> > > >  		/ params->rate;
> > > >  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> > > > +	port->cycles_per_byte = cycles_per_byte;
> > > >
> > > >  	/* Grinders */
> > > >  	port->pkts_out = NULL;
> > > > @@ -2673,20 +2675,26 @@ static inline void 
> > > > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> > > >  	uint64_t cycles = rte_get_tsc_cycles();
> > > > -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > > > +	uint64_t cycles_diff;
> > > >  	uint64_t bytes_diff;
> > > >  	uint32_t i;
> > > >
> > > > +	if (cycles < port->time_cpu_cycles)
> > > > +		goto end;
> > 
> > Above check seems redundant as port->time_cpu_cycles will always be less than the current cycles due to roundoff in previous iteration.
> > 
> 
> This was to catch the condition where the cycles wraps back to zero (after 100+ years?? depending on clock speed).  
> Rather than just going to end: the conditional should at least reset port->time_cpu_cycles back to zero.
> So there would be a very temporary glitch in accuracy once every 100+ years. 




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-06-24 22:50       ` Thomas Monjalon
@ 2020-06-25  8:32         ` Singh, Jasvinder
  2020-06-25  8:40           ` Alan Dewar
  0 siblings, 1 reply; 14+ messages in thread
From: Singh, Jasvinder @ 2020-06-25  8:32 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Dumitrescu, Cristian, 'alangordondewar@gmail.com',
	dev, 'Alan Dewar',
	Dewar, Alan



> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 24, 2020 11:50 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> 'alangordondewar@gmail.com' <alangordondewar@gmail.com>;
> dev@dpdk.org; 'Alan Dewar' <alan.dewar@att.com>; Dewar, Alan
> <alan.dewar@intl.att.com>
> Subject: Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
> 
> Jasvinder, what is the conclusion of this patch?
> 
> 21/04/2020 10:21, Dewar, Alan:
> > From: Singh, Jasvinder <jasvinder.singh@intel.com>
> > > > > From: Alan Dewar <alan.dewar@att.com>
> > > > >
> > > > > The QoS scheduler works off port time that is computed from the
> > > > > number of CPU cycles that have elapsed since the last time the port
> was
> > > > > polled.   It divides the number of elapsed cycles to calculate how
> > > > > many bytes can be sent, however this division can generate
> > > > > rounding errors, where some fraction of a byte sent may be lost.
> > > > >
> > > > > Lose enough of these fractional bytes and the QoS scheduler
> > > > > underperforms.  The problem is worse with low bandwidths.
> > > > >
> > > > > To compensate for this rounding error this fix doesn't advance
> > > > > the port's time_cpu_cycles by the number of cycles that have
> > > > > elapsed, but by multiplying the computed number of bytes that
> > > > > can be sent (which has been rounded down) by number of cycles per
> byte.
> > > > > This will mean that port's time_cpu_cycles will lag behind the
> > > > > CPU cycles momentarily.  At the next poll, the lag will be taken
> > > > > into account.
> > > > >
> > > > > Fixes: de3cfa2c98 ("sched: initial import")
> > > > >
> > > > > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > > > > ---
> > > > >  lib/librte_sched/rte_sched.c | 12 ++++++++++--
> > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_sched/rte_sched.c
> > > > > b/lib/librte_sched/rte_sched.c index c0983ddda..c656dba2d 100644
> > > > > --- a/lib/librte_sched/rte_sched.c
> > > > > +++ b/lib/librte_sched/rte_sched.c
> > > > > @@ -222,6 +222,7 @@ struct rte_sched_port {
> > > > >  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> > > > > */
> > > > >  	uint64_t time;                /* Current NIC TX time measured in bytes */
> > > > >  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per
> > > > > byte */
> > > > > +	uint64_t cycles_per_byte;
> > > > >
> > > > >  	/* Grinders */
> > > > >  	struct rte_mbuf **pkts_out;
> > > > > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> > > > rte_sched_port_params
> > > > > *params)
> > > > >  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> > > > >  		/ params->rate;
> > > > >  	port->inv_cycles_per_byte =
> > > > > rte_reciprocal_value(cycles_per_byte);
> > > > > +	port->cycles_per_byte = cycles_per_byte;
> > > > >
> > > > >  	/* Grinders */
> > > > >  	port->pkts_out = NULL;
> > > > > @@ -2673,20 +2675,26 @@ static inline void
> > > > > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> > > > >  	uint64_t cycles = rte_get_tsc_cycles();
> > > > > -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > > > > +	uint64_t cycles_diff;
> > > > >  	uint64_t bytes_diff;
> > > > >  	uint32_t i;
> > > > >
> > > > > +	if (cycles < port->time_cpu_cycles)
> > > > > +		goto end;
> > >
> > > Above check seems redundant as port->time_cpu_cycles will always be
> less than the current cycles due to roundoff in previous iteration.
> > >
> >
> > This was to catch the condition where the cycles wraps back to zero (after
> 100+ years?? depending on clock speed).
> > Rather than just going to end: the conditional should at least reset port-
> >time_cpu_cycles back to zero.
> > So there would be a very temporary glitch in accuracy once every 100+
> years.
> 

Alan, Could you please resubmit the patch with above change? Other than that, patch looks good to me.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
  2020-06-25  8:32         ` Singh, Jasvinder
@ 2020-06-25  8:40           ` Alan Dewar
  0 siblings, 0 replies; 14+ messages in thread
From: Alan Dewar @ 2020-06-25  8:40 UTC (permalink / raw)
  To: Singh, Jasvinder
  Cc: Thomas Monjalon, Dumitrescu, Cristian, dev, Alan Dewar, Dewar, Alan

Okay will do.

On Thu, Jun 25, 2020 at 9:32 AM Singh, Jasvinder
<jasvinder.singh@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, June 24, 2020 11:50 PM
> > To: Singh, Jasvinder <jasvinder.singh@intel.com>
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> > 'alangordondewar@gmail.com' <alangordondewar@gmail.com>;
> > dev@dpdk.org; 'Alan Dewar' <alan.dewar@att.com>; Dewar, Alan
> > <alan.dewar@intl.att.com>
> > Subject: Re: [dpdk-dev] [PATCH] sched: fix port time rounding error
> >
> > Jasvinder, what is the conclusion of this patch?
> >
> > 21/04/2020 10:21, Dewar, Alan:
> > > From: Singh, Jasvinder <jasvinder.singh@intel.com>
> > > > > > From: Alan Dewar <alan.dewar@att.com>
> > > > > >
> > > > > > The QoS scheduler works off port time that is computed from the
> > > > > > number of CPU cycles that have elapsed since the last time the port
> > was
> > > > > > polled.   It divides the number of elapsed cycles to calculate how
> > > > > > many bytes can be sent, however this division can generate
> > > > > > rounding errors, where some fraction of a byte sent may be lost.
> > > > > >
> > > > > > Lose enough of these fractional bytes and the QoS scheduler
> > > > > > underperforms.  The problem is worse with low bandwidths.
> > > > > >
> > > > > > To compensate for this rounding error this fix doesn't advance
> > > > > > the port's time_cpu_cycles by the number of cycles that have
> > > > > > elapsed, but by multiplying the computed number of bytes that
> > > > > > can be sent (which has been rounded down) by number of cycles per
> > byte.
> > > > > > This will mean that port's time_cpu_cycles will lag behind the
> > > > > > CPU cycles momentarily.  At the next poll, the lag will be taken
> > > > > > into account.
> > > > > >
> > > > > > Fixes: de3cfa2c98 ("sched: initial import")
> > > > > >
> > > > > > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > > > > > ---
> > > > > >  lib/librte_sched/rte_sched.c | 12 ++++++++++--
> > > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/librte_sched/rte_sched.c
> > > > > > b/lib/librte_sched/rte_sched.c index c0983ddda..c656dba2d 100644
> > > > > > --- a/lib/librte_sched/rte_sched.c
> > > > > > +++ b/lib/librte_sched/rte_sched.c
> > > > > > @@ -222,6 +222,7 @@ struct rte_sched_port {
> > > > > >       uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> > > > > > */
> > > > > >       uint64_t time;                /* Current NIC TX time measured in bytes */
> > > > > >       struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per
> > > > > > byte */
> > > > > > +     uint64_t cycles_per_byte;
> > > > > >
> > > > > >       /* Grinders */
> > > > > >       struct rte_mbuf **pkts_out;
> > > > > > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> > > > > rte_sched_port_params
> > > > > > *params)
> > > > > >       cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> > > > > >               / params->rate;
> > > > > >       port->inv_cycles_per_byte =
> > > > > > rte_reciprocal_value(cycles_per_byte);
> > > > > > +     port->cycles_per_byte = cycles_per_byte;
> > > > > >
> > > > > >       /* Grinders */
> > > > > >       port->pkts_out = NULL;
> > > > > > @@ -2673,20 +2675,26 @@ static inline void
> > > > > > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> > > > > >       uint64_t cycles = rte_get_tsc_cycles();
> > > > > > -     uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > > > > > +     uint64_t cycles_diff;
> > > > > >       uint64_t bytes_diff;
> > > > > >       uint32_t i;
> > > > > >
> > > > > > +     if (cycles < port->time_cpu_cycles)
> > > > > > +             goto end;
> > > >
> > > > Above check seems redundant as port->time_cpu_cycles will always be
> > less than the current cycles due to roundoff in previous iteration.
> > > >
> > >
> > > This was to catch the condition where the cycles wraps back to zero (after
> > 100+ years?? depending on clock speed).
> > > Rather than just going to end: the conditional should at least reset port-
> > >time_cpu_cycles back to zero.
> > > So there would be a very temporary glitch in accuracy once every 100+
> > years.
> >
>
> Alan, Could you please resubmit the patch with above change? Other than that, patch looks good to me.
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-04-16  8:48 [dpdk-dev] [PATCH] sched: fix port time rounding error alangordondewar
  2020-04-17 21:19 ` Dumitrescu, Cristian
@ 2020-06-25  9:59 ` alangordondewar
  2020-07-05 20:41   ` Thomas Monjalon
  2020-08-20 14:32   ` Kevin Traynor
  1 sibling, 2 replies; 14+ messages in thread
From: alangordondewar @ 2020-06-25  9:59 UTC (permalink / raw)
  To: cristian.dumitrescu; +Cc: dev, Alan Dewar

From: Alan Dewar <alan.dewar@att.com>

The QoS scheduler works off port time that is computed from the number
of CPU cycles that have elapsed since the last time the port was
polled.   It divides the number of elapsed cycles to calculate how
many bytes can be sent, however this division can generate rounding
errors, where some fraction of a byte sent may be lost.

Lose enough of these fractional bytes and the QoS scheduler
underperforms.  The problem is worse with low bandwidths.

To compensate for this rounding error this fix doesn't advance the
port's time_cpu_cycles by the number of cycles that have elapsed,
but by multiplying the computed number of bytes that can be sent
(which has been rounded down) by number of cycles per byte.
This will mean that port's time_cpu_cycles will lag behind the CPU
cycles momentarily.  At the next poll, the lag will be taken into
account.

v2:
If the cycles value wraps (100 year+) reset the port's cpu cycle back
to zero.

Fixes: de3cfa2c98 ("sched: initial import")

Signed-off-by: Alan Dewar <alan.dewar@att.com>
---
 lib/librte_sched/rte_sched.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index c0983ddda..7c022cd61 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -222,6 +222,7 @@ struct rte_sched_port {
 	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
 	uint64_t time;                /* Current NIC TX time measured in bytes */
 	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
+	uint64_t cycles_per_byte;
 
 	/* Grinders */
 	struct rte_mbuf **pkts_out;
@@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
 	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
 		/ params->rate;
 	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
+	port->cycles_per_byte = cycles_per_byte;
 
 	/* Grinders */
 	port->pkts_out = NULL;
@@ -2673,16 +2675,21 @@ static inline void
 rte_sched_port_time_resync(struct rte_sched_port *port)
 {
 	uint64_t cycles = rte_get_tsc_cycles();
-	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
+	uint64_t cycles_diff;
 	uint64_t bytes_diff;
 	uint32_t i;
 
+	if (cycles < port->time_cpu_cycles)
+		port->time_cpu_cycles = 0;
+
+	cycles_diff = cycles - port->time_cpu_cycles;
 	/* Compute elapsed time in bytes */
 	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
 					   port->inv_cycles_per_byte);
 
 	/* Advance port time */
-	port->time_cpu_cycles = cycles;
+	port->time_cpu_cycles +=
+		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
 	port->time_cpu_bytes += bytes_diff;
 	if (port->time < port->time_cpu_bytes)
 		port->time = port->time_cpu_bytes;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-06-25  9:59 ` [dpdk-dev] [PATCH v2] " alangordondewar
@ 2020-07-05 20:41   ` Thomas Monjalon
  2020-07-06 21:20     ` Singh, Jasvinder
  2020-08-20 14:32   ` Kevin Traynor
  1 sibling, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2020-07-05 20:41 UTC (permalink / raw)
  To: cristian.dumitrescu, jasvinder.singh; +Cc: dev, Alan Dewar, alangordondewar

Cristian, Jasvinder, please review.

25/06/2020 11:59, alangordondewar@gmail.com:
> From: Alan Dewar <alan.dewar@att.com>
> 
> The QoS scheduler works off port time that is computed from the number
> of CPU cycles that have elapsed since the last time the port was
> polled.   It divides the number of elapsed cycles to calculate how
> many bytes can be sent, however this division can generate rounding
> errors, where some fraction of a byte sent may be lost.
> 
> Lose enough of these fractional bytes and the QoS scheduler
> underperforms.  The problem is worse with low bandwidths.
> 
> To compensate for this rounding error this fix doesn't advance the
> port's time_cpu_cycles by the number of cycles that have elapsed,
> but by multiplying the computed number of bytes that can be sent
> (which has been rounded down) by number of cycles per byte.
> This will mean that port's time_cpu_cycles will lag behind the CPU
> cycles momentarily.  At the next poll, the lag will be taken into
> account.
> 
> v2:
> If the cycles value wraps (100 year+) reset the port's cpu cycle back
> to zero.
> 
> Fixes: de3cfa2c98 ("sched: initial import")
> 
> Signed-off-by: Alan Dewar <alan.dewar@att.com>
> ---
>  lib/librte_sched/rte_sched.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index c0983ddda..7c022cd61 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -222,6 +222,7 @@ struct rte_sched_port {
>  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
>  	uint64_t time;                /* Current NIC TX time measured in bytes */
>  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
> +	uint64_t cycles_per_byte;
>  
>  	/* Grinders */
>  	struct rte_mbuf **pkts_out;
> @@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>  		/ params->rate;
>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> +	port->cycles_per_byte = cycles_per_byte;
>  
>  	/* Grinders */
>  	port->pkts_out = NULL;
> @@ -2673,16 +2675,21 @@ static inline void
>  rte_sched_port_time_resync(struct rte_sched_port *port)
>  {
>  	uint64_t cycles = rte_get_tsc_cycles();
> -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> +	uint64_t cycles_diff;
>  	uint64_t bytes_diff;
>  	uint32_t i;
>  
> +	if (cycles < port->time_cpu_cycles)
> +		port->time_cpu_cycles = 0;
> +
> +	cycles_diff = cycles - port->time_cpu_cycles;
>  	/* Compute elapsed time in bytes */
>  	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
>  					   port->inv_cycles_per_byte);
>  
>  	/* Advance port time */
> -	port->time_cpu_cycles = cycles;
> +	port->time_cpu_cycles +=
> +		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
>  	port->time_cpu_bytes += bytes_diff;
>  	if (port->time < port->time_cpu_bytes)
>  		port->time = port->time_cpu_bytes;
> 






^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-07-05 20:41   ` Thomas Monjalon
@ 2020-07-06 21:20     ` Singh, Jasvinder
  2020-07-06 23:01       ` Thomas Monjalon
  0 siblings, 1 reply; 14+ messages in thread
From: Singh, Jasvinder @ 2020-07-06 21:20 UTC (permalink / raw)
  To: dev, Alan Dewar; +Cc: alangordondewar, Thomas Monjalon, Dumitrescu, Cristian



> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Sunday, July 5, 2020 9:42 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Alan Dewar <alan.dewar@att.com>;
> alangordondewar@gmail.com
> Subject: Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
> 
> Cristian, Jasvinder, please review.
> 
> 25/06/2020 11:59, alangordondewar@gmail.com:
> > From: Alan Dewar <alan.dewar@att.com>
> >
> > The QoS scheduler works off port time that is computed from the number
> > of CPU cycles that have elapsed since the last time the port was
> > polled.   It divides the number of elapsed cycles to calculate how
> > many bytes can be sent, however this division can generate rounding
> > errors, where some fraction of a byte sent may be lost.
> >
> > Lose enough of these fractional bytes and the QoS scheduler
> > underperforms.  The problem is worse with low bandwidths.
> >
> > To compensate for this rounding error this fix doesn't advance the
> > port's time_cpu_cycles by the number of cycles that have elapsed, but
> > by multiplying the computed number of bytes that can be sent (which
> > has been rounded down) by number of cycles per byte.
> > This will mean that port's time_cpu_cycles will lag behind the CPU
> > cycles momentarily.  At the next poll, the lag will be taken into
> > account.
> >
> > v2:
> > If the cycles value wraps (100 year+) reset the port's cpu cycle back
> > to zero.
> >
> > Fixes: de3cfa2c98 ("sched: initial import")
> >
> > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> > ---
> >  lib/librte_sched/rte_sched.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_sched/rte_sched.c
> > b/lib/librte_sched/rte_sched.c index c0983ddda..7c022cd61 100644
> > --- a/lib/librte_sched/rte_sched.c
> > +++ b/lib/librte_sched/rte_sched.c
> > @@ -222,6 +222,7 @@ struct rte_sched_port {
> >  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes
> */
> >  	uint64_t time;                /* Current NIC TX time measured in bytes */
> >  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
> > +	uint64_t cycles_per_byte;
> >
> >  	/* Grinders */
> >  	struct rte_mbuf **pkts_out;
> > @@ -852,6 +853,7 @@ rte_sched_port_config(struct
> rte_sched_port_params *params)
> >  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
> >  		/ params->rate;
> >  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> > +	port->cycles_per_byte = cycles_per_byte;
> >
> >  	/* Grinders */
> >  	port->pkts_out = NULL;
> > @@ -2673,16 +2675,21 @@ static inline void
> > rte_sched_port_time_resync(struct rte_sched_port *port)  {
> >  	uint64_t cycles = rte_get_tsc_cycles();
> > -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> > +	uint64_t cycles_diff;
> >  	uint64_t bytes_diff;
> >  	uint32_t i;
> >
> > +	if (cycles < port->time_cpu_cycles)
> > +		port->time_cpu_cycles = 0;
> > +
> > +	cycles_diff = cycles - port->time_cpu_cycles;
> >  	/* Compute elapsed time in bytes */
> >  	bytes_diff = rte_reciprocal_divide(cycles_diff <<
> RTE_SCHED_TIME_SHIFT,
> >  					   port->inv_cycles_per_byte);
> >
> >  	/* Advance port time */
> > -	port->time_cpu_cycles = cycles;
> > +	port->time_cpu_cycles +=
> > +		(bytes_diff * port->cycles_per_byte) >>
> RTE_SCHED_TIME_SHIFT;
> >  	port->time_cpu_bytes += bytes_diff;
> >  	if (port->time < port->time_cpu_bytes)
> >  		port->time = port->time_cpu_bytes;
> >
> 

Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-07-06 21:20     ` Singh, Jasvinder
@ 2020-07-06 23:01       ` Thomas Monjalon
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Monjalon @ 2020-07-06 23:01 UTC (permalink / raw)
  To: Alan Dewar; +Cc: dev, alangordondewar, Dumitrescu, Cristian, Singh, Jasvinder

> > 25/06/2020 11:59, alangordondewar@gmail.com:
> > > From: Alan Dewar <alan.dewar@att.com>
> > >
> > > The QoS scheduler works off port time that is computed from the number
> > > of CPU cycles that have elapsed since the last time the port was
> > > polled.   It divides the number of elapsed cycles to calculate how
> > > many bytes can be sent, however this division can generate rounding
> > > errors, where some fraction of a byte sent may be lost.
> > >
> > > Lose enough of these fractional bytes and the QoS scheduler
> > > underperforms.  The problem is worse with low bandwidths.
> > >
> > > To compensate for this rounding error this fix doesn't advance the
> > > port's time_cpu_cycles by the number of cycles that have elapsed, but
> > > by multiplying the computed number of bytes that can be sent (which
> > > has been rounded down) by number of cycles per byte.
> > > This will mean that port's time_cpu_cycles will lag behind the CPU
> > > cycles momentarily.  At the next poll, the lag will be taken into
> > > account.
> > >
> > > v2:
> > > If the cycles value wraps (100 year+) reset the port's cpu cycle back
> > > to zero.
> > >
> > > Fixes: de3cfa2c98 ("sched: initial import")
> > >
> > > Signed-off-by: Alan Dewar <alan.dewar@att.com>
> 
> Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>

Applied, thanks



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-06-25  9:59 ` [dpdk-dev] [PATCH v2] " alangordondewar
  2020-07-05 20:41   ` Thomas Monjalon
@ 2020-08-20 14:32   ` Kevin Traynor
  2020-08-21 15:28     ` Kinsella, Ray
  1 sibling, 1 reply; 14+ messages in thread
From: Kevin Traynor @ 2020-08-20 14:32 UTC (permalink / raw)
  To: alangordondewar, cristian.dumitrescu
  Cc: dev, Alan Dewar, David Marchand, Ray Kinsella, Luca Boccassi,
	Jasvinder Singh

Hi,

On 25/06/2020 10:59, alangordondewar@gmail.com wrote:
> From: Alan Dewar <alan.dewar@att.com>
> 
> The QoS scheduler works off port time that is computed from the number
> of CPU cycles that have elapsed since the last time the port was
> polled.   It divides the number of elapsed cycles to calculate how
> many bytes can be sent, however this division can generate rounding
> errors, where some fraction of a byte sent may be lost.
> 
> Lose enough of these fractional bytes and the QoS scheduler
> underperforms.  The problem is worse with low bandwidths.
> 
> To compensate for this rounding error this fix doesn't advance the
> port's time_cpu_cycles by the number of cycles that have elapsed,
> but by multiplying the computed number of bytes that can be sent
> (which has been rounded down) by number of cycles per byte.
> This will mean that port's time_cpu_cycles will lag behind the CPU
> cycles momentarily.  At the next poll, the lag will be taken into
> account.
> 
> v2:
> If the cycles value wraps (100 year+) reset the port's cpu cycle back
> to zero.
> 
> Fixes: de3cfa2c98 ("sched: initial import")
> 
> Signed-off-by: Alan Dewar <alan.dewar@att.com>
> ---
>  lib/librte_sched/rte_sched.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index c0983ddda..7c022cd61 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -222,6 +222,7 @@ struct rte_sched_port {
>  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
>  	uint64_t time;                /* Current NIC TX time measured in bytes */
>  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
> +	uint64_t cycles_per_byte;
>  

I was backporting this patch to 18.11. The older ABI checker complains
about this structure change.

"cycles_per_byte has been added at the middle position of this
structural type."

Isn't this an ABI break? Dropping from 18.11 for time being.

>  	/* Grinders */
>  	struct rte_mbuf **pkts_out;
> @@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>  		/ params->rate;
>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
> +	port->cycles_per_byte = cycles_per_byte;
>  
>  	/* Grinders */
>  	port->pkts_out = NULL;
> @@ -2673,16 +2675,21 @@ static inline void
>  rte_sched_port_time_resync(struct rte_sched_port *port)
>  {
>  	uint64_t cycles = rte_get_tsc_cycles();
> -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
> +	uint64_t cycles_diff;
>  	uint64_t bytes_diff;
>  	uint32_t i;
>  
> +	if (cycles < port->time_cpu_cycles)
> +		port->time_cpu_cycles = 0;
> +
> +	cycles_diff = cycles - port->time_cpu_cycles;
>  	/* Compute elapsed time in bytes */
>  	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
>  					   port->inv_cycles_per_byte);
>  
>  	/* Advance port time */
> -	port->time_cpu_cycles = cycles;
> +	port->time_cpu_cycles +=
> +		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
>  	port->time_cpu_bytes += bytes_diff;
>  	if (port->time < port->time_cpu_bytes)
>  		port->time = port->time_cpu_bytes;
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-08-20 14:32   ` Kevin Traynor
@ 2020-08-21 15:28     ` Kinsella, Ray
  2020-09-07 10:09       ` Kevin Traynor
  0 siblings, 1 reply; 14+ messages in thread
From: Kinsella, Ray @ 2020-08-21 15:28 UTC (permalink / raw)
  To: Kevin Traynor, alangordondewar, cristian.dumitrescu
  Cc: dev, Alan Dewar, David Marchand, Luca Boccassi, Jasvinder Singh



On 20/08/2020 15:32, Kevin Traynor wrote:
> Hi,
> 
> On 25/06/2020 10:59, alangordondewar@gmail.com wrote:
>> From: Alan Dewar <alan.dewar@att.com>
>>
>> The QoS scheduler works off port time that is computed from the number
>> of CPU cycles that have elapsed since the last time the port was
>> polled.   It divides the number of elapsed cycles to calculate how
>> many bytes can be sent, however this division can generate rounding
>> errors, where some fraction of a byte sent may be lost.
>>
>> Lose enough of these fractional bytes and the QoS scheduler
>> underperforms.  The problem is worse with low bandwidths.
>>
>> To compensate for this rounding error this fix doesn't advance the
>> port's time_cpu_cycles by the number of cycles that have elapsed,
>> but by multiplying the computed number of bytes that can be sent
>> (which has been rounded down) by number of cycles per byte.
>> This will mean that port's time_cpu_cycles will lag behind the CPU
>> cycles momentarily.  At the next poll, the lag will be taken into
>> account.
>>
>> v2:
>> If the cycles value wraps (100 year+) reset the port's cpu cycle back
>> to zero.
>>
>> Fixes: de3cfa2c98 ("sched: initial import")
>>
>> Signed-off-by: Alan Dewar <alan.dewar@att.com>
>> ---
>>  lib/librte_sched/rte_sched.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
>> index c0983ddda..7c022cd61 100644
>> --- a/lib/librte_sched/rte_sched.c
>> +++ b/lib/librte_sched/rte_sched.c
>> @@ -222,6 +222,7 @@ struct rte_sched_port {
>>  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
>>  	uint64_t time;                /* Current NIC TX time measured in bytes */
>>  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
>> +	uint64_t cycles_per_byte;
>>  
> 
> I was backporting this patch to 18.11. The older ABI checker complains
> about this structure change.
> 
> "cycles_per_byte has been added at the middle position of this
> structural type."
> 
> Isn't this an ABI break? Dropping from 18.11 for time being.

So it looks like rte_sched_port * is an opaque pointers as it's contents are only
known to rte_sched.c and not outside. To everyone else it is an opaque data structure, 
so structural changes to it would not be an ABI breakage.

> 
>>  	/* Grinders */
>>  	struct rte_mbuf **pkts_out;
>> @@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
>>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>>  		/ params->rate;
>>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
>> +	port->cycles_per_byte = cycles_per_byte;
>>  
>>  	/* Grinders */
>>  	port->pkts_out = NULL;
>> @@ -2673,16 +2675,21 @@ static inline void
>>  rte_sched_port_time_resync(struct rte_sched_port *port)
>>  {
>>  	uint64_t cycles = rte_get_tsc_cycles();
>> -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
>> +	uint64_t cycles_diff;
>>  	uint64_t bytes_diff;
>>  	uint32_t i;
>>  
>> +	if (cycles < port->time_cpu_cycles)
>> +		port->time_cpu_cycles = 0;
>> +
>> +	cycles_diff = cycles - port->time_cpu_cycles;
>>  	/* Compute elapsed time in bytes */
>>  	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
>>  					   port->inv_cycles_per_byte);
>>  
>>  	/* Advance port time */
>> -	port->time_cpu_cycles = cycles;
>> +	port->time_cpu_cycles +=
>> +		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
>>  	port->time_cpu_bytes += bytes_diff;
>>  	if (port->time < port->time_cpu_bytes)
>>  		port->time = port->time_cpu_bytes;
>>
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH v2] sched: fix port time rounding error
  2020-08-21 15:28     ` Kinsella, Ray
@ 2020-09-07 10:09       ` Kevin Traynor
  0 siblings, 0 replies; 14+ messages in thread
From: Kevin Traynor @ 2020-09-07 10:09 UTC (permalink / raw)
  To: Kinsella, Ray, alangordondewar, cristian.dumitrescu
  Cc: dev, Alan Dewar, David Marchand, Luca Boccassi, Jasvinder Singh

On 21/08/2020 16:28, Kinsella, Ray wrote:
> 
> 
> On 20/08/2020 15:32, Kevin Traynor wrote:
>> Hi,
>>
>> On 25/06/2020 10:59, alangordondewar@gmail.com wrote:
>>> From: Alan Dewar <alan.dewar@att.com>
>>>
>>> The QoS scheduler works off port time that is computed from the number
>>> of CPU cycles that have elapsed since the last time the port was
>>> polled.   It divides the number of elapsed cycles to calculate how
>>> many bytes can be sent, however this division can generate rounding
>>> errors, where some fraction of a byte sent may be lost.
>>>
>>> Lose enough of these fractional bytes and the QoS scheduler
>>> underperforms.  The problem is worse with low bandwidths.
>>>
>>> To compensate for this rounding error this fix doesn't advance the
>>> port's time_cpu_cycles by the number of cycles that have elapsed,
>>> but by multiplying the computed number of bytes that can be sent
>>> (which has been rounded down) by number of cycles per byte.
>>> This will mean that port's time_cpu_cycles will lag behind the CPU
>>> cycles momentarily.  At the next poll, the lag will be taken into
>>> account.
>>>
>>> v2:
>>> If the cycles value wraps (100 year+) reset the port's cpu cycle back
>>> to zero.
>>>
>>> Fixes: de3cfa2c98 ("sched: initial import")
>>>
>>> Signed-off-by: Alan Dewar <alan.dewar@att.com>
>>> ---
>>>  lib/librte_sched/rte_sched.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
>>> index c0983ddda..7c022cd61 100644
>>> --- a/lib/librte_sched/rte_sched.c
>>> +++ b/lib/librte_sched/rte_sched.c
>>> @@ -222,6 +222,7 @@ struct rte_sched_port {
>>>  	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
>>>  	uint64_t time;                /* Current NIC TX time measured in bytes */
>>>  	struct rte_reciprocal inv_cycles_per_byte; /* CPU cycles per byte */
>>> +	uint64_t cycles_per_byte;
>>>  
>>
>> I was backporting this patch to 18.11. The older ABI checker complains
>> about this structure change.
>>
>> "cycles_per_byte has been added at the middle position of this
>> structural type."
>>
>> Isn't this an ABI break? Dropping from 18.11 for time being.
> 
> So it looks like rte_sched_port * is an opaque pointers as it's contents are only
> known to rte_sched.c and not outside. To everyone else it is an opaque data structure, 
> so structural changes to it would not be an ABI breakage.
> 

Thanks Ray, makes sense. I've included the fix in 18.11.10-rc1.

Kevin.

>>
>>>  	/* Grinders */
>>>  	struct rte_mbuf **pkts_out;
>>> @@ -852,6 +853,7 @@ rte_sched_port_config(struct rte_sched_port_params *params)
>>>  	cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
>>>  		/ params->rate;
>>>  	port->inv_cycles_per_byte = rte_reciprocal_value(cycles_per_byte);
>>> +	port->cycles_per_byte = cycles_per_byte;
>>>  
>>>  	/* Grinders */
>>>  	port->pkts_out = NULL;
>>> @@ -2673,16 +2675,21 @@ static inline void
>>>  rte_sched_port_time_resync(struct rte_sched_port *port)
>>>  {
>>>  	uint64_t cycles = rte_get_tsc_cycles();
>>> -	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
>>> +	uint64_t cycles_diff;
>>>  	uint64_t bytes_diff;
>>>  	uint32_t i;
>>>  
>>> +	if (cycles < port->time_cpu_cycles)
>>> +		port->time_cpu_cycles = 0;
>>> +
>>> +	cycles_diff = cycles - port->time_cpu_cycles;
>>>  	/* Compute elapsed time in bytes */
>>>  	bytes_diff = rte_reciprocal_divide(cycles_diff << RTE_SCHED_TIME_SHIFT,
>>>  					   port->inv_cycles_per_byte);
>>>  
>>>  	/* Advance port time */
>>> -	port->time_cpu_cycles = cycles;
>>> +	port->time_cpu_cycles +=
>>> +		(bytes_diff * port->cycles_per_byte) >> RTE_SCHED_TIME_SHIFT;
>>>  	port->time_cpu_bytes += bytes_diff;
>>>  	if (port->time < port->time_cpu_bytes)
>>>  		port->time = port->time_cpu_bytes;
>>>
>>
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-09-07 10:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-16  8:48 [dpdk-dev] [PATCH] sched: fix port time rounding error alangordondewar
2020-04-17 21:19 ` Dumitrescu, Cristian
2020-04-20 11:23   ` Singh, Jasvinder
2020-04-21  8:21     ` Dewar, Alan
2020-06-24 22:50       ` Thomas Monjalon
2020-06-25  8:32         ` Singh, Jasvinder
2020-06-25  8:40           ` Alan Dewar
2020-06-25  9:59 ` [dpdk-dev] [PATCH v2] " alangordondewar
2020-07-05 20:41   ` Thomas Monjalon
2020-07-06 21:20     ` Singh, Jasvinder
2020-07-06 23:01       ` Thomas Monjalon
2020-08-20 14:32   ` Kevin Traynor
2020-08-21 15:28     ` Kinsella, Ray
2020-09-07 10:09       ` Kevin Traynor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).