From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH] net/mlx5: fix number of segment calculation Date: Fri, 10 Nov 2017 13:22:58 -0800 Message-ID: <20171110212257.GA4189@yongseok-MBP.local> References: <1510243472-24872-1-git-send-email-orika@mellanox.com> <20171110100625.GF24849@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Ori Kam , nelio.laranjeiro@6wind.com, dev@dpdk.org, stable@dpdk.org To: Adrien Mazarguil Return-path: Content-Disposition: inline In-Reply-To: <20171110100625.GF24849@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > 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