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

[Yongseok Koh <yskoh@mellanox.com>]

On Fri, Nov 10, 2017 at 11:06:25AM +0100, Adrien Mazarguil wrote:
> Hi Ori,
> 
> 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>
> 
> I don't think there's an issue to fix, there's actually a reason it's done
> that way, perhaps I'm wrong but let me elaborate.
> 
> When applications request CRC to be written to mbuf (more precisely not to
> be stripped), its extra 4 bytes are neither part of mbuf->pkt_len nor
> mbuf->data_len. It just happens to be written past mbuf data if there's room
> for it, where applications knowingly expect it based on how they configured
> the PMD. That's the API.
>
> This implies applications also size mbufs accordingly; if they don't provide
> room for the CRC, it can't be written. This extra room is assumed to be part
> of max_rx_pkt_len. When CRC stripping is requested, they do not have to
> provide such room (IBV_WQ_FLAGS_SCATTER_FCS is not set on mlx5 Rx queues).

I looked around other driver/example codes as it is not documented (or too
obvious to do?), it looks there's consensus that max_rx_pkt_len includes 4B FCS.
Then, I agree that PMD doesn't need to care about this.

> One problem with your proposal is assuming all segments are consumed
> entirely during Rx and max_rx_pkt_len is reached, another segment with zero
> data length gets appended just to hold the CRC. Applications may interpret
> this as a bug.

I don't think this patch causes the issue. It just unnecessarily reserves extra
4B room if CRC strip is disabled. And even apps should not interpret this as a
bug because apps requested to have CRC.
Currently mlx5_rx_busrt() doesn't allow this situation (putting only 4B CRC in
the last segment) because it subtracts ETHER_CRC_LEN from pkt_len if CRC isn't
stripped. And it is done before looking for the next segment. I think this is a
problem to fix on the contrary - app wanted to see CRC but it's not there.
Right?

Thanks,
Yongseok