* [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).