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