* [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation
@ 2016-09-07 6:15 Nikhil Jagtap
2016-09-19 15:51 ` Dumitrescu, Cristian
2016-09-21 5:57 ` [dpdk-dev] [PATCH v2] " Nikhil Jagtap
0 siblings, 2 replies; 6+ messages in thread
From: Nikhil Jagtap @ 2016-09-07 6:15 UTC (permalink / raw)
To: cristian.dumitrescu; +Cc: dev, kannan.babu.ramia, Nikhil Jagtap
As per srTCM RFC 2697, we should be updating the E bucket only after the
C bucket overflows. This patch fixes the current DPDK implementation,
where we are updating both the buckets simultaneously at the same rate
(CIR) which results in token accumulation rate of (2*CIR).
Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
---
lib/librte_meter/rte_meter.h | 26 ++++++++++++++++----------
1 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/librte_meter/rte_meter.h b/lib/librte_meter/rte_meter.h
index 2cd8d81..0ffcb60 100644
--- a/lib/librte_meter/rte_meter.h
+++ b/lib/librte_meter/rte_meter.h
@@ -232,13 +232,16 @@ rte_meter_srtcm_color_blind_check(struct rte_meter_srtcm *m,
n_periods = time_diff / m->cir_period;
m->time += n_periods * m->cir_period;
+ /* Put the tokens overflowing from tc into te bucket */
tc = m->tc + n_periods * m->cir_bytes_per_period;
- if (tc > m->cbs)
+ if (tc > m->cbs) {
+ te = m->te + (tc - m->cbs);
+ if (te > m->ebs)
+ te = m->ebs;
tc = m->cbs;
-
- te = m->te + n_periods * m->cir_bytes_per_period;
- if (te > m->ebs)
- te = m->ebs;
+ } else {
+ te = m->te;
+ }
/* Color logic */
if (tc >= pkt_len) {
@@ -271,13 +274,16 @@ rte_meter_srtcm_color_aware_check(struct rte_meter_srtcm *m,
n_periods = time_diff / m->cir_period;
m->time += n_periods * m->cir_period;
+ /* Put the tokens overflowing from tc into te bucket */
tc = m->tc + n_periods * m->cir_bytes_per_period;
- if (tc > m->cbs)
+ if (tc > m->cbs) {
+ te = m->te + (tc - m->cbs);
+ if (te > m->ebs)
+ te = m->ebs;
tc = m->cbs;
-
- te = m->te + n_periods * m->cir_bytes_per_period;
- if (te > m->ebs)
- te = m->ebs;
+ } else {
+ te = m->te;
+ }
/* Color logic */
if ((pkt_color == e_RTE_METER_GREEN) && (tc >= pkt_len)) {
--
1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation
2016-09-07 6:15 [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation Nikhil Jagtap
@ 2016-09-19 15:51 ` Dumitrescu, Cristian
2016-09-20 4:40 ` Nikhil Jagtap
2016-09-21 5:57 ` [dpdk-dev] [PATCH v2] " Nikhil Jagtap
1 sibling, 1 reply; 6+ messages in thread
From: Dumitrescu, Cristian @ 2016-09-19 15:51 UTC (permalink / raw)
To: Nikhil Jagtap; +Cc: dev, Ramia, Kannan Babu
> -----Original Message-----
> From: Nikhil Jagtap [mailto:nikhil.jagtap@gmail.com]
> Sent: Wednesday, September 7, 2016 7:15 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Ramia, Kannan Babu <kannan.babu.ramia@intel.com>;
> Nikhil Jagtap <nikhil.jagtap@gmail.com>
> Subject: [PATCH] meter: fix excess token bucket update in srtcm
> implementation
>
> As per srTCM RFC 2697, we should be updating the E bucket only after the
> C bucket overflows. This patch fixes the current DPDK implementation,
> where we are updating both the buckets simultaneously at the same rate
> (CIR) which results in token accumulation rate of (2*CIR).
>
> Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
> ---
> lib/librte_meter/rte_meter.h | 26 ++++++++++++++++----------
> 1 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_meter/rte_meter.h b/lib/librte_meter/rte_meter.h
> index 2cd8d81..0ffcb60 100644
> --- a/lib/librte_meter/rte_meter.h
> +++ b/lib/librte_meter/rte_meter.h
> @@ -232,13 +232,16 @@ rte_meter_srtcm_color_blind_check(struct
> rte_meter_srtcm *m,
> n_periods = time_diff / m->cir_period;
> m->time += n_periods * m->cir_period;
>
> + /* Put the tokens overflowing from tc into te bucket */
> tc = m->tc + n_periods * m->cir_bytes_per_period;
> - if (tc > m->cbs)
> + if (tc > m->cbs) {
> + te = m->te + (tc - m->cbs);
> + if (te > m->ebs)
> + te = m->ebs;
> tc = m->cbs;
> -
> - te = m->te + n_periods * m->cir_bytes_per_period;
> - if (te > m->ebs)
> - te = m->ebs;
> + } else {
> + te = m->te;
> + }
Just to avoid the final else, in order to have the critical path (Tc not overflowing) as the default fall-through code path, I suggest the following small change in the code (update the Te just after Tc update, for the case of Tc overflowing), which should favour the usage of the cmov instruction:
/* Put the tokens overflowing from tc into te bucket */
tc = m->tc + n_periods * m->cir_bytes_per_period;
te = m->te;
if (tc > m->cbs) {
te = m->te + (tc - m->cbs);
if (te > m->ebs)
te = m->ebs;
tc = m->cbs;
}
Are you OK with this change?
>
> /* Color logic */
> if (tc >= pkt_len) {
> @@ -271,13 +274,16 @@ rte_meter_srtcm_color_aware_check(struct
> rte_meter_srtcm *m,
> n_periods = time_diff / m->cir_period;
> m->time += n_periods * m->cir_period;
>
> + /* Put the tokens overflowing from tc into te bucket */
> tc = m->tc + n_periods * m->cir_bytes_per_period;
> - if (tc > m->cbs)
> + if (tc > m->cbs) {
> + te = m->te + (tc - m->cbs);
> + if (te > m->ebs)
> + te = m->ebs;
> tc = m->cbs;
> -
> - te = m->te + n_periods * m->cir_bytes_per_period;
> - if (te > m->ebs)
> - te = m->ebs;
> + } else {
> + te = m->te;
> + }
Same as above.
>
> /* Color logic */
> if ((pkt_color == e_RTE_METER_GREEN) && (tc >= pkt_len)) {
> --
> 1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation
2016-09-19 15:51 ` Dumitrescu, Cristian
@ 2016-09-20 4:40 ` Nikhil Jagtap
0 siblings, 0 replies; 6+ messages in thread
From: Nikhil Jagtap @ 2016-09-20 4:40 UTC (permalink / raw)
To: Dumitrescu, Cristian; +Cc: dev, Ramia, Kannan Babu
Hi Cristian,
My comments inline prefixed with [nikhil].
On 19 September 2016 at 21:21, Dumitrescu, Cristian <
cristian.dumitrescu@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Nikhil Jagtap [mailto:nikhil.jagtap@gmail.com]
> > Sent: Wednesday, September 7, 2016 7:15 AM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: dev@dpdk.org; Ramia, Kannan Babu <kannan.babu.ramia@intel.com>;
> > Nikhil Jagtap <nikhil.jagtap@gmail.com>
> > Subject: [PATCH] meter: fix excess token bucket update in srtcm
> > implementation
> >
> > As per srTCM RFC 2697, we should be updating the E bucket only after the
> > C bucket overflows. This patch fixes the current DPDK implementation,
> > where we are updating both the buckets simultaneously at the same rate
> > (CIR) which results in token accumulation rate of (2*CIR).
> >
> > Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
> > ---
> > lib/librte_meter/rte_meter.h | 26 ++++++++++++++++----------
> > 1 files changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_meter/rte_meter.h b/lib/librte_meter/rte_meter.h
> > index 2cd8d81..0ffcb60 100644
> > --- a/lib/librte_meter/rte_meter.h
> > +++ b/lib/librte_meter/rte_meter.h
> > @@ -232,13 +232,16 @@ rte_meter_srtcm_color_blind_check(struct
> > rte_meter_srtcm *m,
> > n_periods = time_diff / m->cir_period;
> > m->time += n_periods * m->cir_period;
> >
> > + /* Put the tokens overflowing from tc into te bucket */
> > tc = m->tc + n_periods * m->cir_bytes_per_period;
> > - if (tc > m->cbs)
> > + if (tc > m->cbs) {
> > + te = m->te + (tc - m->cbs);
> > + if (te > m->ebs)
> > + te = m->ebs;
> > tc = m->cbs;
> > -
> > - te = m->te + n_periods * m->cir_bytes_per_period;
> > - if (te > m->ebs)
> > - te = m->ebs;
> > + } else {
> > + te = m->te;
> > + }
>
> Just to avoid the final else, in order to have the critical path (Tc not
overflowing) as the default fall-through code path, I suggest the following
small change in the code (update the Te just after Tc update, for the case
of Tc overflowing), which should favour the usage of the cmov instruction:
>
> /* Put the tokens overflowing from tc into te bucket */
> tc = m->tc + n_periods * m->cir_bytes_per_period;
> te = m->te;
>
> if (tc > m->cbs) {
> te = m->te + (tc - m->cbs);
> if (te > m->ebs)
> te = m->ebs;
> tc = m->cbs;
> }
>
> Are you OK with this change?
[nikhil] I am not sure if "Tc not overflowing" is really the critical path,
as that will depend on the traffic rate. For traffic rates lower than CIR,
we will always hit the Tc overflow case. For example, with a CIR of 15 mbps
and a traffic rate of 5 mbps, Tc will be overflow at the rate of 10 mbps.
But anyway, I am ok with your suggestion. I will make the following change
in both places and resubmit the patch.
...
te = m->te;
if (tc > m->cbs) {
te += (tc - m->cbs);
...
>
> >
> > /* Color logic */
> > if (tc >= pkt_len) {
> > @@ -271,13 +274,16 @@ rte_meter_srtcm_color_aware_check(struct
> > rte_meter_srtcm *m,
> > n_periods = time_diff / m->cir_period;
> > m->time += n_periods * m->cir_period;
> >
> > + /* Put the tokens overflowing from tc into te bucket */
> > tc = m->tc + n_periods * m->cir_bytes_per_period;
> > - if (tc > m->cbs)
> > + if (tc > m->cbs) {
> > + te = m->te + (tc - m->cbs);
> > + if (te > m->ebs)
> > + te = m->ebs;
> > tc = m->cbs;
> > -
> > - te = m->te + n_periods * m->cir_bytes_per_period;
> > - if (te > m->ebs)
> > - te = m->ebs;
> > + } else {
> > + te = m->te;
> > + }
>
> Same as above.
>
> >
> > /* Color logic */
> > if ((pkt_color == e_RTE_METER_GREEN) && (tc >= pkt_len)) {
> > --
> > 1.7.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] meter: fix excess token bucket update in srtcm implementation
2016-09-07 6:15 [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation Nikhil Jagtap
2016-09-19 15:51 ` Dumitrescu, Cristian
@ 2016-09-21 5:57 ` Nikhil Jagtap
2016-09-21 8:57 ` Dumitrescu, Cristian
1 sibling, 1 reply; 6+ messages in thread
From: Nikhil Jagtap @ 2016-09-21 5:57 UTC (permalink / raw)
To: cristian.dumitrescu; +Cc: dev, kannan.babu.ramia, Nikhil Jagtap
As per srTCM RFC 2697, we should be updating the E bucket only after the
C bucket overflows. This patch fixes the current DPDK implementation,
where we are updating both the buckets simultaneously at the same rate
(CIR) which results in token accumulation rate of (2*CIR).
Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
---
v2:
Removed the else part and instead added a default initialization of te.
---
lib/librte_meter/rte_meter.h | 24 ++++++++++++++----------
1 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/lib/librte_meter/rte_meter.h b/lib/librte_meter/rte_meter.h
index 2cd8d81..2ab7184 100644
--- a/lib/librte_meter/rte_meter.h
+++ b/lib/librte_meter/rte_meter.h
@@ -232,13 +232,15 @@ rte_meter_srtcm_color_blind_check(struct rte_meter_srtcm *m,
n_periods = time_diff / m->cir_period;
m->time += n_periods * m->cir_period;
+ /* Put the tokens overflowing from tc into te bucket */
tc = m->tc + n_periods * m->cir_bytes_per_period;
- if (tc > m->cbs)
+ te = m->te;
+ if (tc > m->cbs) {
+ te += (tc - m->cbs);
+ if (te > m->ebs)
+ te = m->ebs;
tc = m->cbs;
-
- te = m->te + n_periods * m->cir_bytes_per_period;
- if (te > m->ebs)
- te = m->ebs;
+ }
/* Color logic */
if (tc >= pkt_len) {
@@ -271,13 +273,15 @@ rte_meter_srtcm_color_aware_check(struct rte_meter_srtcm *m,
n_periods = time_diff / m->cir_period;
m->time += n_periods * m->cir_period;
+ /* Put the tokens overflowing from tc into te bucket */
tc = m->tc + n_periods * m->cir_bytes_per_period;
- if (tc > m->cbs)
+ te = m->te;
+ if (tc > m->cbs) {
+ te += (tc - m->cbs);
+ if (te > m->ebs)
+ te = m->ebs;
tc = m->cbs;
-
- te = m->te + n_periods * m->cir_bytes_per_period;
- if (te > m->ebs)
- te = m->ebs;
+ }
/* Color logic */
if ((pkt_color == e_RTE_METER_GREEN) && (tc >= pkt_len)) {
--
1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] meter: fix excess token bucket update in srtcm implementation
2016-09-21 5:57 ` [dpdk-dev] [PATCH v2] " Nikhil Jagtap
@ 2016-09-21 8:57 ` Dumitrescu, Cristian
2016-09-21 21:19 ` Thomas Monjalon
0 siblings, 1 reply; 6+ messages in thread
From: Dumitrescu, Cristian @ 2016-09-21 8:57 UTC (permalink / raw)
To: Nikhil Jagtap; +Cc: dev, Ramia, Kannan Babu
> -----Original Message-----
> From: Nikhil Jagtap [mailto:nikhil.jagtap@gmail.com]
> Sent: Wednesday, September 21, 2016 6:58 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Ramia, Kannan Babu <kannan.babu.ramia@intel.com>;
> Nikhil Jagtap <nikhil.jagtap@gmail.com>
> Subject: [PATCH v2] meter: fix excess token bucket update in srtcm
> implementation
>
> As per srTCM RFC 2697, we should be updating the E bucket only after the
> C bucket overflows. This patch fixes the current DPDK implementation,
> where we are updating both the buckets simultaneously at the same rate
> (CIR) which results in token accumulation rate of (2*CIR).
>
> Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
> ---
>
> v2:
> Removed the else part and instead added a default initialization of te.
>
> ---
> lib/librte_meter/rte_meter.h | 24 ++++++++++++++----------
> 1 files changed, 14 insertions(+), 10 deletions(-)
>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Thanks, Nikhil!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2] meter: fix excess token bucket update in srtcm implementation
2016-09-21 8:57 ` Dumitrescu, Cristian
@ 2016-09-21 21:19 ` Thomas Monjalon
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2016-09-21 21:19 UTC (permalink / raw)
To: Nikhil Jagtap; +Cc: dev, Dumitrescu, Cristian, Ramia, Kannan Babu
> > As per srTCM RFC 2697, we should be updating the E bucket only after the
> > C bucket overflows. This patch fixes the current DPDK implementation,
> > where we are updating both the buckets simultaneously at the same rate
> > (CIR) which results in token accumulation rate of (2*CIR).
> >
> > Signed-off-by: Nikhil Jagtap <nikhil.jagtap@gmail.com>
>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Applied, thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-21 21:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 6:15 [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation Nikhil Jagtap
2016-09-19 15:51 ` Dumitrescu, Cristian
2016-09-20 4:40 ` Nikhil Jagtap
2016-09-21 5:57 ` [dpdk-dev] [PATCH v2] " Nikhil Jagtap
2016-09-21 8:57 ` Dumitrescu, Cristian
2016-09-21 21:19 ` Thomas Monjalon
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).