From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 04CCC1B6B9 for ; Fri, 10 Nov 2017 11:22:18 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id b14so1686976wme.2 for ; Fri, 10 Nov 2017 02:22:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=AA5SzJc/WrVom6zHZTj+OIqiAaTRVcUS8/3VXK2FIg0=; b=UXEZJrvvPtE4tPyZFwVJ3wQF6xGxUw39QFni/6i3mMyefpeEnhk2RPP5DF3w+t8VT/ v9n4lXE7AjrMSkGqNc/kmF4ybM2vZyQqq/bbdLKrH8YJY9IdAWbLFkn2d3otjh/B0wgt 6t6LZouRj2nhFTnDTKB3xd2TgISGejeiCp7kitqNtxRr6zQY07MhTK9JerpbZn/zI6kR 2yVLdHkwJynZfvOpIIwZdBAfUDS7pB/Ec8rpoaNNCe4N8DbKcNemQq07wEQhIC88gcIh at4p7biBnf6PLmUtodx0SFGildFjf1eHssHoipDKcAh6T7CH6sbsNTS8L8fNDcrUyx9q sM/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=AA5SzJc/WrVom6zHZTj+OIqiAaTRVcUS8/3VXK2FIg0=; b=gBrtCK8SmV91DnYB0J3Lmw5gSDad8jfNfPzucrFENNt0Nk6bGPXTuqIzvZRWonxREJ GBOTSrm9Xd0EkxH3xih4KuAwwD7TC+qa0hRWA3v8MJYPKw15IEi8Hs2KWI9CXqlOnlLw 6WF629R/0E08OcFQ3IZhwpF4ViqgTeYRubTF1QYcmExUwQOcmAjsVNRFFfG94fTdTC8K i6HTZwO0hjv5+6BznD9VxXYvVsCm8AFycrT+e6Ppn5RJ2EjNrkhjwvo1QGC9cma9aKas GgAGsnneWMAQuo63MoFWOREyHTnp5+tOneS4JN5bqc2I3Hlv02TeuKdmBzokHG0GL0YE YReQ== X-Gm-Message-State: AJaThX4wxrvPl47vKytNWKREm3YGci8YZoNK3YOQbPl2VpvPQ78DP7Zy m+GPjwSVUGAv3yOdlcq6Vcu4jw== X-Google-Smtp-Source: AGs4zMYlMLKfMaUR3/DgOsrxnomraZ0AKPuC7suTrXOzMW8E25eVOSgGlgrhRotCfJpcxkVoHVvf3Q== X-Received: by 10.80.214.136 with SMTP id r8mr1893914edi.17.1510309338522; Fri, 10 Nov 2017 02:22:18 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id w49sm7998705edb.13.2017.11.10.02.22.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Nov 2017 02:22:17 -0800 (PST) Date: Fri, 10 Nov 2017 11:22:06 +0100 From: Adrien Mazarguil To: Yongseok Koh Cc: Ori Kam , nelio.laranjeiro@6wind.com, dev@dpdk.org, stable@dpdk.org Message-ID: <20171110102206.GG24849@6wind.com> References: <1510243472-24872-1-git-send-email-orika@mellanox.com> <20171109223029.GA3599@yongseok-MBP.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171109223029.GA3599@yongseok-MBP.local> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix number of segment calculation 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, 10 Nov 2017 10:22:19 -0000 Hi Yongseok, On Thu, Nov 09, 2017 at 02:30:30PM -0800, Yongseok Koh wrote: > On Thu, Nov 09, 2017 at 06:04:32PM +0200, Ori Kam wrote: > > The CRC size should be taken into consideration when computing > > the number of mbuf segments for packet on the receive path. > > Large packets can be dropped due to extra CRC length. > > > > Fixes: a1366b1a2be3 ("net/mlx5: add reference counter on DPDK Rx queues") > > Cc: stable@dpdk.org > > Cc: nelio.laranjeiro@6wind.com > > > > Signed-off-by: Ori Kam > > --- > > drivers/net/mlx5/mlx5_rxq.c | 7 +++++-- > > 1 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c > > index 6b29aae..701925b 100644 > > --- a/drivers/net/mlx5/mlx5_rxq.c > > +++ b/drivers/net/mlx5/mlx5_rxq.c > > @@ -887,6 +887,8 @@ struct mlx5_rxq_ctrl* > > const uint16_t desc_n = > > desc + priv->rx_vec_en * MLX5_VPMD_DESCS_PER_LOOP; > > unsigned int mb_len = rte_pktmbuf_data_room_size(mp); > > + uint8_t crc_size = > > + !!(dev->data->dev_conf.rxmode.hw_strip_crc == 0) << 2; > > How about making it more explicit with ETHER_CRC_LEN? E.g. > uint8_t crc_size = ETHER_CRC_LEN * > (dev->data->dev_conf.rxmode.hw_strip_crc == 0); > > > > > tmpl = rte_calloc_socket("RXQ", 1, > > sizeof(*tmpl) + > > @@ -900,12 +902,13 @@ struct mlx5_rxq_ctrl* > > /* Enable scattered packets support for this queue if necessary. */ > > assert(mb_len >= RTE_PKTMBUF_HEADROOM); > > You might want to make the same change for this assert? > > > if (dev->data->dev_conf.rxmode.max_rx_pkt_len <= > > - (mb_len - RTE_PKTMBUF_HEADROOM)) { > > + (mb_len - RTE_PKTMBUF_HEADROOM - crc_size)) { > > tmpl->rxq.sges_n = 0; > > } else if (dev->data->dev_conf.rxmode.enable_scatter) { > > unsigned int size = > > RTE_PKTMBUF_HEADROOM + > > - dev->data->dev_conf.rxmode.max_rx_pkt_len; > > + dev->data->dev_conf.rxmode.max_rx_pkt_len + > > + crc_size; > > I think there's another bugs we didn't know. If scatter is required, > RTE_PKTMBUF_HEADROOM is also reserved per every chained mbufs. So, it looks like > mb_len should be "rte_pktmbuf_data_room_size(mp) - RTE_PKTMBUF_HEADROOM" when it > is declared in the beginning. Make sense? RTE_PKTMBUF_HEADROOM is actually only reserved on the first segment, i.e. once per mbuf chain, it should be fine. > > /* > > * Determine the number of SGEs needed for a full packet > > * and round it to the next power of two. > > */ > > sges_n = log2above((size / mb_len) + !!(size % mb_len)); > > tmpl->rxq.sges_n = sges_n; > > rxq.sges_n is 2bits, which means the max value is 3. So, if sges_n is larger > than 3, it would just take the last 2bits and it will result in false error > below. As we can't use sizeof() for bit-fields, this should be changed like: The name is perhaps confusing, sges_n is documented as a log 2 value, 1 << 3 means 8 segments at most. Assuming default mbuf size, this allows up to 17280 bytes per packet excluding headroom. You're right exceeding 3 will remove the extra bits and since sizeof() can't be used, that's precisely the reason for the subsequent check, which makes sure the stored value is enough for a max_rx_pkt_len-sized packet after converting it back to a number of bytes. > > /* Check the maximum value of the bit-field. */ > tmpl->rxq.sges_n = -1; > tmpl->rxq.sges_n = RTE_MIN(tmpl->rxq.sges_n, sges_n); > > > /* Make sure rxq.sges_n did not overflow. */ > > size = mb_len * (1 << tmpl->rxq.sges_n); > > size -= RTE_PKTMBUF_HEADROOM; > > if (size < dev->data->dev_conf.rxmode.max_rx_pkt_len) { > > ERROR("%p: too many SGEs (%u) needed to handle" > > " requested maximum packet size %u", > > (void *)dev, > > 1 << sges_n, > > dev->data->dev_conf.rxmode.max_rx_pkt_len); > > goto error; > > } > > This may be unnecessary if we make right changes? I think it has to be kept as a safety check even if the max number of SGEs is increased, at least as long as it's stored as a bit-field value. -- Adrien Mazarguil 6WIND