From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 58C501B494 for ; Fri, 4 Jan 2019 15:03:14 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B72AAC056794; Fri, 4 Jan 2019 14:03:13 +0000 (UTC) Received: from [10.36.117.29] (ovpn-117-29.ams2.redhat.com [10.36.117.29]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0C6AA171D0; Fri, 4 Jan 2019 14:03:12 +0000 (UTC) From: "Eelco Chaudron" To: "Dumitrescu, Cristian" Cc: dev@dpdk.org Date: Fri, 04 Jan 2019 15:03:10 +0100 Message-ID: In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891268E81857C@IRSMSX108.ger.corp.intel.com> References: <20181218153551.10961.2569.stgit@iMac> <20181218153842.10961.32000.stgit@iMac> <3EB4FA525960D640B5BDFFD6A3D891268E81857C@IRSMSX108.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; format=flowed X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 04 Jan 2019 14:03:13 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v3 1/2] lib/librte_meter: add RFC4115 trTCM meter support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Jan 2019 14:03:14 -0000 On 21 Dec 2018, at 17:15, Dumitrescu, Cristian wrote: > Hi Eelco, > > > >> >> +/** trTCM parameters per metered traffic flow. The CIR, EIT, CBS and >> EBS > > Small typo here: EIT to be replaced by EIR. > >> +parameters only count bytes of IP packets and do not include link >> specific >> +headers. The CBS and EBS need to be greater than zero if CIR and EIR >> are >> +none-zero respectively.*/ >> +struct rte_meter_trtcm_rfc4115_params { >> + uint64_t cir; /**< Committed Information Rate (CIR). Measured in >> bytes per second. */ >> + uint64_t eir; /**< Excess Information Rate (EIR). Measured in bytes >> per second. */ >> + uint64_t cbs; /**< Committed Burst Size (CBS). Measured in bytes. >> */ >> + uint64_t ebs; /**< Excess Burst Size (EBS). Measured in bytes. */ >> +}; >> + > > > >> +static inline enum rte_meter_color __rte_experimental >> +rte_meter_trtcm_rfc4115_color_blind_check( >> + struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + uint64_t time, >> + uint32_t pkt_len) >> +{ >> + uint64_t time_diff_tc, time_diff_te, n_periods_tc, n_periods_te, >> tc, >> te; >> + >> + /* Bucket update */ >> + time_diff_tc = time - m->time_tc; >> + time_diff_te = time - m->time_te; >> + n_periods_tc = time_diff_tc / p->cir_period; >> + n_periods_te = time_diff_te / p->eir_period; >> + m->time_tc += n_periods_tc * p->cir_period; >> + m->time_te += n_periods_te * p->eir_period; >> + >> + tc = m->tc + n_periods_tc * p->cir_bytes_per_period; >> + if (tc > p->cbs) >> + tc = p->cbs; >> + >> + te = m->te + n_periods_te * p->eir_bytes_per_period; >> + if (te > p->ebs) >> + te = p->ebs; >> + >> + /* Color logic */ >> + if (tc >= pkt_len) { >> + m->tc = tc - pkt_len; >> + m->te = te; >> + return e_RTE_METER_GREEN; >> + } else if (te >= pkt_len) { >> + m->tc = tc; >> + m->te = te - pkt_len; >> + return e_RTE_METER_YELLOW; >> + } >> + >> + /* If we end up here the color is RED */ >> + m->tc = tc; >> + m->te = te; >> + return e_RTE_METER_RED; >> +} >> + > > Since the branch (tc >= pkt_len) == TRUE always returns, I suggest we > remove the following "else", as it is redundant: > > /* Color logic */ > if (tc >= pkt_len) { > m->tc = tc - pkt_len; > m->te = te; > return e_RTE_METER_GREEN; > } > > if (te >= pkt_len) { > m->tc = tc; > m->te = te - pkt_len; > return e_RTE_METER_YELLOW; > } > > /* If we end up here the color is RED */ > m->tc = tc; > m->te = te; > return e_RTE_METER_RED; > > >> +static inline enum rte_meter_color __rte_experimental >> +rte_meter_trtcm_rfc4115_color_aware_check( >> + struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + uint64_t time, >> + uint32_t pkt_len, >> + enum rte_meter_color pkt_color) >> +{ >> + uint64_t time_diff_tc, time_diff_te, n_periods_tc, n_periods_te, >> tc, >> te; >> + >> + /* Bucket update */ >> + time_diff_tc = time - m->time_tc; >> + time_diff_te = time - m->time_te; >> + n_periods_tc = time_diff_tc / p->cir_period; >> + n_periods_te = time_diff_te / p->eir_period; >> + m->time_tc += n_periods_tc * p->cir_period; >> + m->time_te += n_periods_te * p->eir_period; >> + >> + tc = m->tc + n_periods_tc * p->cir_bytes_per_period; >> + if (tc > p->cbs) >> + tc = p->cbs; >> + >> + te = m->te + n_periods_te * p->eir_bytes_per_period; >> + if (te > p->ebs) >> + te = p->ebs; >> + >> + /* Color logic */ >> + if (pkt_color == e_RTE_METER_GREEN) { >> + if (tc >= pkt_len) { >> + m->tc = tc - pkt_len; >> + m->te = te; >> + return e_RTE_METER_GREEN; >> + } else if (te >= pkt_len) { >> + m->tc = tc; >> + m->te = te - pkt_len; >> + return e_RTE_METER_YELLOW; >> + } >> + } else if (pkt_color == e_RTE_METER_YELLOW && te >= pkt_len) { >> + m->tc = tc; >> + m->te = te - pkt_len; >> + return e_RTE_METER_YELLOW; >> + } >> + >> + /* If we end up here the color is RED */ >> + m->tc = tc; >> + m->te = te; >> + return e_RTE_METER_RED; >> +} >> + >> + > > I suggest we follow the logic from the diagram in the RFC rather than > the logic in the text preceding the diagram. Although the two > descriptions are equivalent (after a bit of thinking), the diagram > seems more optimal to me: > > /* Color logic */ > if ((pkt_color == e_RTE_METER_GREEN) && (tc >= pkt_len)) { > m->tc = tc - pkt_len; > m->te = te; > return e_RTE_METER_GREEN; > } > > if ((pkt_color != e_RTE_METER_RED) && (te >= pkt_len)) { > m->tc = tc; > m->te = te - pkt_len; > return e_RTE_METER_YELLOW; > } > > /* If we end up here the color is RED */ > m->tc = tc; > m->te = te; > return e_RTE_METER_RED; > > > > Thanks, > Cristian Asking all comments, sent out a V4. //Eelco