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

[Yongseok Koh <yskoh@mellanox.com>]

On Fri, Nov 10, 2017 at 11:22:06AM +0100, Adrien Mazarguil wrote:
> 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 <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?
> 
> RTE_PKTMBUF_HEADROOM is actually only reserved on the first segment,
> i.e. once per mbuf chain, it should be fine.

Right, I got confused with Tx. mlx5's Rx overwrites m->data_offset when it
injects mbufs for extra segments.

> > > 		/*
> > > 		 * 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.

The name wasn't confusing, I wanted to make it clearer as I thought it could
have some false negatives. But, I misread something. The sanity check can
correctly filter those cases. No bug here!

Thanks,
Yongseok