Hi,
Thanks Adrien and Yongseok for your review.
Due to Adrien comment I'm withdrawing the patch.
Thanks again,
Ori Kam
> -----Original Message-----
> From: Yongseok Koh
> Sent: Friday, November 10, 2017 11:23 PM
> To: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> Cc: Ori Kam <or...@mellanox.com>; Nélio Laranjeiro
> <nelio.laranje...@6wind.com>; dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [PATCH] net/mlx5: fix number of segment calculation
>
> 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: sta...@dpdk.org
> > > Cc: nelio.laranje...@6wind.com
> > >
> > > Signed-off-by: Ori Kam <or...@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
> > mbuf->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
