From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 685FD2A62 for ; Thu, 30 Nov 2017 11:47:54 +0100 (CET) Received: by mail-wm0-f68.google.com with SMTP id 9so12113144wme.4 for ; Thu, 30 Nov 2017 02:47:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:content-transfer-encoding:mime-version; bh=fTJT5H9I8kHPgAOrFflG6huMrTAjG7QjFJ0Er2m2IEw=; b=fiiNJp5WQJqfEUH2P3cEOyvvebG+Puymqxxzf7DV48zZX7uB/JulXCXH72KFoKfIVM JgcP2IxTovb0HR8duGCLlUgwiIC7veOzckIpRSt/XCY6fukSXgr5EopoBmMThdOG40yE tJ5/3Vy9tYltZrcmcGqWvVFZB71pso4ZBC3Qik3Mu5diSQJsD4bzUamSGJ6TKGbx5XdB K7S8LWPkZLCKLRAizXMF7qFxz9Lq4AnBJaYJhZTuz/KbNh9qheN4GNOZauQSwqBWXTgu CkabnA+rPtabpJ3uqbF8k47uLuyIu/ZniDmJqmQaIUiNdBlh+3j/H5xRvtx3QDVpH6L7 parw== X-Gm-Message-State: AJaThX4GN+OEW7ITSdgo3Y1vuRIwEdlyx4K6OXmLp8QLk8t61FtfPCSw 9gZcr4viFu/gzJAUbIRQ1XQ= X-Google-Smtp-Source: AGs4zMbW5Kxn6GzGVPbgK+Bm5P5DMpEelIl4BM0NYNwdYeTN+t9VEppgnMnoD0CbeMQS+kf7tF3TdA== X-Received: by 10.28.194.11 with SMTP id s11mr161371wmf.143.1512038874118; Thu, 30 Nov 2017 02:47:54 -0800 (PST) Received: from localhost ([213.251.34.151]) by smtp.gmail.com with ESMTPSA id p17sm66043wma.23.2017.11.30.02.47.52 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 30 Nov 2017 02:47:53 -0800 (PST) Message-ID: <1512038872.10466.0.camel@debian.org> From: Luca Boccassi To: alangordondewar@gmail.com, cristian.dumitrescu@intel.com Cc: dev@dpdk.org, Alan Dewar Date: Thu, 30 Nov 2017 10:47:52 +0000 In-Reply-To: <1512032696-30765-1-git-send-email-alan.dewar@att.com> References: <1512032696-30765-1-git-send-email-alan.dewar@att.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.22.6-1+deb9u1 Mime-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] sched: fix overflow errors in WRR weighting code 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: Thu, 30 Nov 2017 10:47:55 -0000 On Thu, 2017-11-30 at 09:04 +0000, alangordondewar@gmail.com wrote: > From: Alan Dewar >=20 > Revised patch - this version fixes an issue when a small wrr_cost is > shifted so far right that its value becomes zero. >=20 > The WRR code calculates the lowest common denominator between the > four > WRR weights as a uint32_t value and divides the LCD by each of the > WRR > weights and casts the results as a uint8_t.=C2=A0=C2=A0This casting can c= ause > the ratios of the computed wrr costs to be wrong.=C2=A0=C2=A0For example = with > WRR weights of 3, 5, 7 and 11, the LCD is computed to be > 1155.=C2=A0=C2=A0The WRR costs get computed as: >=20 > =C2=A0 1155/3 =3D 385, 1155/5 =3D 231, 1155/7 =3D 165, 1155/11 =3D 105. >=20 > When the value 385 is cast into an uint8_t it ends up as 129. > Rather than casting straight into a uint8_t, this patch shifts the > computed WRR costs right so that the largest value is only eight bits > wide. >=20 > In grinder_schedule, the packet length is multiplied by the WRR cost > and added to the grinder's wrr_tokens value.=C2=A0=C2=A0The grinder's > wrr_tokens > field is a uint16_t, so combination of a packet length of 1500 bytes > and a wrr cost of 44 will overflow this field on the first packet. >=20 > This patch increases the width of the grinder's wrr_tokens and > wrr_mask fields from uint16_t to uint32_t. >=20 > In grinder_wrr_store, the remaining tokens in the grinder's > wrr_tokens > array are copied to the appropriate pipe's wrr_tokens array.=C2=A0=C2=A0H= owever > the pipe's wrr_tokens array is only a uint8_t array so unused tokens > were quite frequently lost which upsets the balance of traffic across > the four WRR queues. >=20 > This patch increases the width of the pipe's wrr_tokens array from > a uint8_t to uint32_t. >=20 > Signed-off-by: Alan Dewar > --- > v2 - fixed bug in the wrr_cost calculation code that could result > in a zero wrr_cost >=20 > =C2=A0lib/librte_sched/rte_sched.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0| 59 > +++++++++++++++++++++++++++++-------- > =C2=A0lib/librte_sched/rte_sched_common.h | 15 ++++++++++ > =C2=A02 files changed, 61 insertions(+), 13 deletions(-) >=20 > diff --git a/lib/librte_sched/rte_sched.c > b/lib/librte_sched/rte_sched.c > index 7252f85..324743d 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -130,7 +130,7 @@ struct rte_sched_pipe { > =C2=A0 uint32_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE]; > =C2=A0 > =C2=A0 /* Weighted Round Robin (WRR) */ > - uint8_t wrr_tokens[RTE_SCHED_QUEUES_PER_PIPE]; > + uint32_t wrr_tokens[RTE_SCHED_QUEUES_PER_PIPE]; > =C2=A0 > =C2=A0 /* TC oversubscription */ > =C2=A0 uint32_t tc_ov_credits; > @@ -205,8 +205,8 @@ struct rte_sched_grinder { > =C2=A0 struct rte_mbuf *pkt; > =C2=A0 > =C2=A0 /* WRR */ > - uint16_t wrr_tokens[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > - uint16_t wrr_mask[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > + uint32_t wrr_tokens[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > + uint32_t wrr_mask[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > =C2=A0 uint8_t wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > =C2=A0}; > =C2=A0 > @@ -542,6 +542,17 @@ rte_sched_time_ms_to_bytes(uint32_t time_ms, > uint32_t rate) > =C2=A0 return time; > =C2=A0} > =C2=A0 > +static uint32_t rte_sched_reduce_to_byte(uint32_t value) > +{ > + uint32_t shift =3D 0; > + > + while (value & 0xFFFFFF00) { > + value >>=3D 1; > + shift++; > + } > + return shift; > +} > + > =C2=A0static void > =C2=A0rte_sched_port_config_pipe_profile_table(struct rte_sched_port > *port, struct rte_sched_port_params *params) > =C2=A0{ > @@ -583,6 +594,8 @@ rte_sched_port_config_pipe_profile_table(struct > rte_sched_port *port, struct rte > =C2=A0 uint32_t > wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS]; > =C2=A0 uint32_t lcd, lcd1, lcd2; > =C2=A0 uint32_t qindex; > + uint32_t low_pos; > + uint32_t shift; > =C2=A0 > =C2=A0 qindex =3D j * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; > =C2=A0 > @@ -594,12 +607,28 @@ rte_sched_port_config_pipe_profile_table(struct > rte_sched_port *port, struct rte > =C2=A0 lcd1 =3D rte_get_lcd(wrr_cost[0], > wrr_cost[1]); > =C2=A0 lcd2 =3D rte_get_lcd(wrr_cost[2], > wrr_cost[3]); > =C2=A0 lcd =3D rte_get_lcd(lcd1, lcd2); > + low_pos =3D rte_min_pos_4_u32(wrr_cost); > =C2=A0 > =C2=A0 wrr_cost[0] =3D lcd / wrr_cost[0]; > =C2=A0 wrr_cost[1] =3D lcd / wrr_cost[1]; > =C2=A0 wrr_cost[2] =3D lcd / wrr_cost[2]; > =C2=A0 wrr_cost[3] =3D lcd / wrr_cost[3]; > =C2=A0 > + shift =3D > rte_sched_reduce_to_byte(wrr_cost[low_pos]); > + wrr_cost[0] >>=3D shift; > + wrr_cost[1] >>=3D shift; > + wrr_cost[2] >>=3D shift; > + wrr_cost[3] >>=3D shift; > + > + if (wrr_cost[0] =3D=3D 0) > + wrr_cost[0]++; > + if (wrr_cost[1] =3D=3D 0) > + wrr_cost[1]++; > + if (wrr_cost[2] =3D=3D 0) > + wrr_cost[2]++; > + if (wrr_cost[3] =3D=3D 0) > + wrr_cost[3]++; > + > =C2=A0 dst->wrr_cost[qindex] =3D (uint8_t) > wrr_cost[0]; > =C2=A0 dst->wrr_cost[qindex + 1] =3D (uint8_t) > wrr_cost[1]; > =C2=A0 dst->wrr_cost[qindex + 2] =3D (uint8_t) > wrr_cost[2]; > @@ -1941,15 +1970,19 @@ grinder_wrr_load(struct rte_sched_port *port, > uint32_t pos) > =C2=A0 > =C2=A0 qindex =3D tc_index * 4; > =C2=A0 > - grinder->wrr_tokens[0] =3D ((uint16_t) pipe- > >wrr_tokens[qindex]) << RTE_SCHED_WRR_SHIFT; > - grinder->wrr_tokens[1] =3D ((uint16_t) pipe->wrr_tokens[qindex=20 > + 1]) << RTE_SCHED_WRR_SHIFT; > - grinder->wrr_tokens[2] =3D ((uint16_t) pipe->wrr_tokens[qindex=20 > + 2]) << RTE_SCHED_WRR_SHIFT; > - grinder->wrr_tokens[3] =3D ((uint16_t) pipe->wrr_tokens[qindex=20 > + 3]) << RTE_SCHED_WRR_SHIFT; > + grinder->wrr_tokens[0] =3D pipe->wrr_tokens[qindex] << > + RTE_SCHED_WRR_SHIFT; > + grinder->wrr_tokens[1] =3D pipe->wrr_tokens[qindex + 1] << > + RTE_SCHED_WRR_SHIFT; > + grinder->wrr_tokens[2] =3D pipe->wrr_tokens[qindex + 2] << > + RTE_SCHED_WRR_SHIFT; > + grinder->wrr_tokens[3] =3D pipe->wrr_tokens[qindex + 3] << > + RTE_SCHED_WRR_SHIFT; > =C2=A0 > - grinder->wrr_mask[0] =3D (qmask & 0x1) * 0xFFFF; > - grinder->wrr_mask[1] =3D ((qmask >> 1) & 0x1) * 0xFFFF; > - grinder->wrr_mask[2] =3D ((qmask >> 2) & 0x1) * 0xFFFF; > - grinder->wrr_mask[3] =3D ((qmask >> 3) & 0x1) * 0xFFFF; > + grinder->wrr_mask[0] =3D (qmask & 0x1) * 0xFFFFFFFF; > + grinder->wrr_mask[1] =3D ((qmask >> 1) & 0x1) * 0xFFFFFFFF; > + grinder->wrr_mask[2] =3D ((qmask >> 2) & 0x1) * 0xFFFFFFFF; > + grinder->wrr_mask[3] =3D ((qmask >> 3) & 0x1) * 0xFFFFFFFF; > =C2=A0 > =C2=A0 grinder->wrr_cost[0] =3D pipe_params->wrr_cost[qindex]; > =C2=A0 grinder->wrr_cost[1] =3D pipe_params->wrr_cost[qindex + 1]; > @@ -1981,14 +2014,14 @@ static inline void > =C2=A0grinder_wrr(struct rte_sched_port *port, uint32_t pos) > =C2=A0{ > =C2=A0 struct rte_sched_grinder *grinder =3D port->grinder + pos; > - uint16_t wrr_tokens_min; > + uint32_t wrr_tokens_min; > =C2=A0 > =C2=A0 grinder->wrr_tokens[0] |=3D ~grinder->wrr_mask[0]; > =C2=A0 grinder->wrr_tokens[1] |=3D ~grinder->wrr_mask[1]; > =C2=A0 grinder->wrr_tokens[2] |=3D ~grinder->wrr_mask[2]; > =C2=A0 grinder->wrr_tokens[3] |=3D ~grinder->wrr_mask[3]; > =C2=A0 > - grinder->qpos =3D rte_min_pos_4_u16(grinder->wrr_tokens); > + grinder->qpos =3D rte_min_pos_4_u32(grinder->wrr_tokens); > =C2=A0 wrr_tokens_min =3D grinder->wrr_tokens[grinder->qpos]; > =C2=A0 > =C2=A0 grinder->wrr_tokens[0] -=3D wrr_tokens_min; > diff --git a/lib/librte_sched/rte_sched_common.h > b/lib/librte_sched/rte_sched_common.h > index aed144b..a3c6bc2 100644 > --- a/lib/librte_sched/rte_sched_common.h > +++ b/lib/librte_sched/rte_sched_common.h > @@ -77,6 +77,21 @@ rte_min_pos_4_u16(uint16_t *x) > =C2=A0 return pos0; > =C2=A0} > =C2=A0 > +static inline uint32_t > +rte_min_pos_4_u32(uint32_t *x) > +{ > + uint32_t pos0 =3D 0; > + uint32_t pos1 =3D 2; > + > + if (x[1] <=3D x[0]) > + pos0 =3D 1; > + if (x[3] <=3D x[2]) > + pos1 =3D 3; > + if (x[pos1] <=3D x[pos0]) > + pos0 =3D pos1; > + > + return pos0; > +} > =C2=A0#endif > =C2=A0 > =C2=A0/* Reviewed-by: Luca Boccassi LGTM --=20 Kind regards, Luca Boccassi