From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f194.google.com (mail-io0-f194.google.com [209.85.223.194]) by dpdk.org (Postfix) with ESMTP id 48FFF5588 for ; Tue, 20 Sep 2016 06:40:19 +0200 (CEST) Received: by mail-io0-f194.google.com with SMTP id r145so536708ior.0 for ; Mon, 19 Sep 2016 21:40:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=hkOwOdAZZUkG1RlVrVaJpFHpnaYBmHynXui6u4cIz80=; b=lkH45GKCk0FliG1nbD6gKu8Udinthble4hEi+pBLlhPD+zfs1geChKaGPL49usizt1 fUs2xl9tE/4DUnhhZPoTytHw0AYSQXDjrDuxDvKo1gtbx1xR/72bn58Vsh28YTJuBGAI i7Cjccp2UHU+1BxVj1RJkUsEDgtJUNS1erjljln/iKquVEB/11N+wclUKAhdHdXxgnvY JOwjDW1QU8mSMfoH69BoFwby+mBTwnuj1jVS1S2NiTtL6eJ+RNfLi14DzOkmJdr3w2OK o3ekpNKiD0zh0xai1LKN51tC2bZvDw4jJ6uiKmdnnjkTzI/5+9K5/N0TbpKUBWvT6MJm sHLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=hkOwOdAZZUkG1RlVrVaJpFHpnaYBmHynXui6u4cIz80=; b=mdCcwJo+Pp+QAsONL333gUbifXbfnvGU7gkmpSSUdYFp6/AOd1Etf89M/3PvAAAUye MxaqezDZcYdq1BifvD8ii6CWKsoQCarbQUxKn9cOqtlcEP8LpIkPM/AoYGtqrLXMwT4D H/nEq+pnKDzIsVmDlJlsAWMTTU+eItXJQATKDD21idWtBncW1L2B9gWp9ERDiAKzmbJ9 OobLd961/zAPWt1YmOCq5SNb1ohSy0DUYIAfdcvTMTETxorYLPh95klRxP2E5kTgLCsH rHslTuDoAx33fqBLGQk5CPATAWQ6dNRRs1Z8WeVOD/JegzASZgEIj7Fd7xJmDNiCbeh6 GW3w== X-Gm-Message-State: AE9vXwPF8jYJEGw+gwg4BDf6byz249ZIyaXhmvNprvS86yPVoOW4AzvhsFmeo99SzOdb3X6sFrzS/dskKEHPGg== X-Received: by 10.107.41.81 with SMTP id p78mr39228297iop.78.1474346418631; Mon, 19 Sep 2016 21:40:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.53.23 with HTTP; Mon, 19 Sep 2016 21:40:18 -0700 (PDT) In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D8912647A83BBA@IRSMSX108.ger.corp.intel.com> References: <1473228910-10429-1-git-send-email-nikhil.jagtap@gmail.com> <3EB4FA525960D640B5BDFFD6A3D8912647A83BBA@IRSMSX108.ger.corp.intel.com> From: Nikhil Jagtap Date: Tue, 20 Sep 2016 10:10:18 +0530 Message-ID: To: "Dumitrescu, Cristian" Cc: "dev@dpdk.org" , "Ramia, Kannan Babu" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] meter: fix excess token bucket update in srtcm implementation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Sep 2016 04:40:19 -0000 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 > > Cc: dev@dpdk.org; Ramia, Kannan Babu ; > > Nikhil Jagtap > > 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 > > --- > > 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 >