From: Yongseok Koh <yskoh@mellanox.com>
To: Ori Kam <orika@mellanox.com>
Cc: adrien.mazarguil@6wind.com, nelio.laranjeiro@6wind.com,
dev@dpdk.org, stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix number of segment calculation
Date: Thu, 9 Nov 2017 14:30:30 -0800 [thread overview]
Message-ID: <20171109223029.GA3599@yongseok-MBP.local> (raw)
In-Reply-To: <1510243472-24872-1-git-send-email-orika@mellanox.com>
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 <orika@mellanox.com>
> ---
> 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?
> /*
> * 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:
/* 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?
Thanks,
Yongseok
next prev parent reply other threads:[~2017-11-09 22:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-09 16:04 Ori Kam
2017-11-09 22:30 ` Yongseok Koh [this message]
2017-11-10 10:22 ` Adrien Mazarguil
2017-11-10 21:42 ` Yongseok Koh
2017-11-10 10:06 ` Adrien Mazarguil
2017-11-10 21:22 ` Yongseok Koh
2017-11-12 7:08 ` Ori Kam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171109223029.GA3599@yongseok-MBP.local \
--to=yskoh@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.com \
--cc=orika@mellanox.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).