From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id 7670F3790 for ; Tue, 14 Nov 2017 18:52:51 +0100 (CET) Received: by mail-wr0-f193.google.com with SMTP id y9so18253179wrb.2 for ; Tue, 14 Nov 2017 09:52:51 -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=dod1dp+wzR7cDsus6k9zLCtitutan6VkdqiEqVI+M20=; b=j8zIOnlGPe4IWoRjwy+mFepA+gKQZYn3wPPTzYC1sS+Z+9ICLqef9JI5E0xL46X1Z7 txHWeD/n7/WirMO9Rcm5Vd7O4s3FwBiRlFlkIbkpWX6mqgP6iAXefjErxt7bJFxaOhZI wgMbqpAEoyMlIWojuoEG0hiphYpFUsYngRHZIYE6RvWnA+maTiiVuiE0ExtFROLrgsTd ZGR9jPmdCgJKtduDbyAy/1L4K0i5RMt9U9MTijRrG9v5CpcFkcSIWH5hlwPZvkPLlnOZ pND5cbeMraHzrJczCDKwugG+VVtggR+7ZD27uj5m7iPkjT0wrKPs0uAKxK5P6o+JeKx8 z1UQ== X-Gm-Message-State: AJaThX5EMsnx39EsQBNJuDRBaOX/TrtMIehmKjsmUlJwEOSK8S/Z48wG ttNTbfd2bwz1l+mBZ9vpbXE= X-Google-Smtp-Source: AGs4zMZuNXX1eF6koLVX1CvdTkXhsdGn99Bjty6SQsLM9rSqtgJxQklwnhe3tCYPFLD5FeKQKJ+4Tg== X-Received: by 10.223.199.138 with SMTP id l10mr10073076wrg.121.1510681971042; Tue, 14 Nov 2017 09:52:51 -0800 (PST) Received: from localhost ([2a00:23c5:bef3:400:4a51:b7ff:fe0b:4749]) by smtp.gmail.com with ESMTPSA id k5sm11100881wmg.21.2017.11.14.09.52.49 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 14 Nov 2017 09:52:50 -0800 (PST) Message-ID: <1510681968.28034.0.camel@debian.org> From: Luca Boccassi To: alangordondewar@gmail.com, cristian.dumitrescu@intel.com Cc: dev@dpdk.org, chas3@att.com, alan.robertson@att.com, Alan Dewar Date: Tue, 14 Nov 2017 17:52:48 +0000 In-Reply-To: <1510675480-14456-1-git-send-email-alan.dewar@att.com> References: <1510675480-14456-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] 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: Tue, 14 Nov 2017 17:52:51 -0000 On Tue, 2017-11-14 at 16:04 +0000, alangordondewar@gmail.com wrote: > From: Alan Dewar >=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 > --- > =C2=A0lib/librte_sched/rte_sched.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0| 57 ++++++++++++++++++++++++++- > ---------- > =C2=A0lib/librte_sched/rte_sched_common.h | 15 ++++++++++ > =C2=A02 files changed, 55 insertions(+), 17 deletions(-) >=20 > diff --git a/lib/librte_sched/rte_sched.c > b/lib/librte_sched/rte_sched.c > index 7252f85..a65c98f 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,16 +607,22 @@ 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 > - dst->wrr_cost[qindex] =3D (uint8_t) > wrr_cost[0]; > - dst->wrr_cost[qindex + 1] =3D (uint8_t) > wrr_cost[1]; > - dst->wrr_cost[qindex + 2] =3D (uint8_t) > wrr_cost[2]; > - dst->wrr_cost[qindex + 3] =3D (uint8_t) > wrr_cost[3]; > + shift =3D > rte_sched_reduce_to_byte(wrr_cost[low_pos]); > + dst->wrr_cost[qindex] =3D (uint8_t) > (wrr_cost[0] >> > + =C2=A0=C2=A0=C2=A0shift); > + dst->wrr_cost[qindex + 1] =3D (uint8_t) > (wrr_cost[1] >> > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0shift > ); > + dst->wrr_cost[qindex + 2] =3D (uint8_t) > (wrr_cost[2] >> > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0shift > ); > + dst->wrr_cost[qindex + 3] =3D (uint8_t) > (wrr_cost[3] >> > + =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0shift > ); > =C2=A0 } > =C2=A0 > =C2=A0 rte_sched_port_log_pipe_profile(port, i); > @@ -1941,15 +1960,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 +2004,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