From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Eelco Chaudron" Subject: Re: [PATCH v2] lib/librte_meter: fix divide by zero for RFC4115 meter Date: Tue, 19 Mar 2019 12:00:04 +0100 Message-ID: <806845F2-2DD7-4954-BB52-448DA85D5891@redhat.com> References: <154903721465.46418.12675922836824541692.stgit@dbuild> <3EB4FA525960D640B5BDFFD6A3D891268E846048@IRSMSX108.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: "Dumitrescu, Cristian" Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 443B82956 for ; Tue, 19 Mar 2019 12:00:09 +0100 (CET) In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891268E846048@IRSMSX108.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 28 Feb 2019, at 19:50, Dumitrescu, Cristian wrote: > Hi Eelco, > > Sorry for my delayed reply No problem I was OOO also so hence my late reply ;) >> -----Original Message----- >> From: Eelco Chaudron [mailto:echaudro@redhat.com] >> Sent: Friday, February 1, 2019 4:07 PM >> To: Dumitrescu, Cristian >> Cc: dev@dpdk.org >> Subject: [PATCH v2] lib/librte_meter: fix divide by zero for RFC4115 >> meter >> >> RFC 4115 allows a meter with either cir and/or eir configured. >> When only one is configured a divide by zero would occur. >> >> Fixes: 655796d2b5fb ("meter: support RFC4115 trTCM") >> Cc: echaudro@redhat.com >> >> Signed-off-by: Eelco Chaudron >> --- >> >> v2 - Removed configuration change that got included by accident >> >> lib/librte_meter/rte_meter.h | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/lib/librte_meter/rte_meter.h >> b/lib/librte_meter/rte_meter.h >> index 005e4eeee..56d85ecf0 100644 >> --- a/lib/librte_meter/rte_meter.h >> +++ b/lib/librte_meter/rte_meter.h >> @@ -597,8 +597,8 @@ rte_meter_trtcm_rfc4115_color_blind_check( >> /* 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; >> + n_periods_tc = p->cir_period != 0 ? time_diff_tc / p->cir_period : >> 0; >> + n_periods_te = p->eir_period != 0 ? time_diff_te / p->eir_period : >> 0; >> m->time_tc += n_periods_tc * p->cir_period; >> m->time_te += n_periods_te * p->eir_period; >> >> @@ -641,8 +641,8 @@ rte_meter_trtcm_rfc4115_color_aware_check( >> /* 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; >> + n_periods_tc = p->cir_period != 0 ? time_diff_tc / p->cir_period : >> 0; >> + n_periods_te = p->eir_period != 0 ? time_diff_te / p->eir_period : >> 0; >> m->time_tc += n_periods_tc * p->cir_period; >> m->time_te += n_periods_te * p->eir_period; >> > > Yes, this is indeed an issue, good catch! > > For performance reasons, we'd like to avoid a test on the fast path > (rte_meter_xyz_check) and replace it with more work done on the > configuration stage (rte_meter_profile_xyz_config), if possible. > > We can intercept the null CIR or EIR cases very early in > rte_meter_trtcm_rfc4115_profile_config() and deal with them separately > by setting the CIR/EIR period and bytes_per_period to some neutral > values and skipping the call to rte_meter_get_tb_params(): period = > RTE_METER_TB_PERIOD_MIN, bytes_per_period = 0. Excellent idea, will sent out a v3 soon, after some testing… //Eelco