Re: [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation

[Nikhil Jagtap <nikhil.jagtap@gmail.com>]

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
>