Re: [dpdk-dev] [PATCH] net/mlx5: fix number of segment calculation

[Yongseok Koh]

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: sta...@dpdk.org
> Cc: nelio.laranje...@6wind.com
> 
> Signed-off-by: Ori Kam <or...@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