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: 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?
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
