Re: [dpdk-dev] [PATCH v4 4/5] net/ice: fix vector rx burst for ice

["Guo, Jia" <jia.guo@intel.com>]

Hi, qi

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Thursday, September 17, 2020 7:03 PM
> To: Guo, Jia <jia.guo@intel.com>; Yang, Qiming <qiming.yang@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Wang, Haiyue <haiyue.wang@intel.com>
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; dev@dpdk.org; Zhang, Helin
> <helin.zhang@intel.com>; mb@smartsharesystems.com; Yigit, Ferruh
> <ferruh.yigit@intel.com>; stephen@networkplumber.org; barbette@kth.se;
> Han, YingyaX <yingyax.han@intel.com>
> Subject: RE: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
> 
> 
> 
> > -----Original Message-----
> > From: Guo, Jia <jia.guo@intel.com>
> > Sent: Thursday, September 17, 2020 3:59 PM
> > To: Yang, Qiming <qiming.yang@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; dev@dpdk.org; Guo, Jia
> > <jia.guo@intel.com>; Zhang, Helin <helin.zhang@intel.com>;
> > mb@smartsharesystems.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > stephen@networkplumber.org; barbette@kth.se; Han, YingyaX
> > <yingyax.han@intel.com>
> > Subject: [PATCH v4 4/5] net/ice: fix vector rx burst for ice
> >
> > The limitation of burst size in vector rx was removed, since it should
> > retrieve as much received packets as possible. And also the scattered
> > receive path should use a wrapper function to achieve the goal of
> > burst maximizing. And do some code cleaning for vector rx path.
> >
> > Bugzilla ID: 516
> > Fixes: c68a52b8b38c ("net/ice: support vector SSE in Rx")
> > Fixes: ae60d3c9b227 ("net/ice: support Rx AVX2 vector")
> >
> > Signed-off-by: Jeff Guo <jia.guo@intel.com>
> > Tested-by: Yingya Han <yingyax.han@intel.com>
> > ---
> >  drivers/net/ice/ice_rxtx.h          |  1 +
> >  drivers/net/ice/ice_rxtx_vec_avx2.c | 23 ++++++------
> > drivers/net/ice/ice_rxtx_vec_sse.c  | 56 +++++++++++++++++++----------
> >  3 files changed, 49 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
> > index 2fdcfb7d0..3ef5f300d 100644
> > --- a/drivers/net/ice/ice_rxtx.h
> > +++ b/drivers/net/ice/ice_rxtx.h
> > @@ -35,6 +35,7 @@
> >  #define ICE_MAX_RX_BURST            ICE_RXQ_REARM_THRESH
> >  #define ICE_TX_MAX_FREE_BUF_SZ      64
> >  #define ICE_DESCS_PER_LOOP          4
> > +#define ICE_DESCS_PER_LOOP_AVX	    8
> 
> No need to expose this if no external link, better to keep all avx stuff inside
> avx.c
> 

Ok, so define it in avx.c is the best choice if avx should not in rxtx.h.

> >
> >  #define ICE_FDIR_PKT_LEN	512
> >
> > diff --git a/drivers/net/ice/ice_rxtx_vec_avx2.c
> > b/drivers/net/ice/ice_rxtx_vec_avx2.c
> > index be50677c2..843e4f32a 100644
> > --- a/drivers/net/ice/ice_rxtx_vec_avx2.c
> > +++ b/drivers/net/ice/ice_rxtx_vec_avx2.c
> > @@ -29,7 +29,7 @@ ice_rxq_rearm(struct ice_rx_queue *rxq)
> >  			__m128i dma_addr0;
> >
> >  			dma_addr0 = _mm_setzero_si128();
> > -			for (i = 0; i < ICE_DESCS_PER_LOOP; i++) {
> > +			for (i = 0; i < ICE_DESCS_PER_LOOP_AVX; i++) {
> >  				rxep[i].mbuf = &rxq->fake_mbuf;
> >  				_mm_store_si128((__m128i *)&rxdp[i].read,
> >  						dma_addr0);
> > @@ -132,12 +132,17 @@ ice_rxq_rearm(struct ice_rx_queue *rxq)
> >  	ICE_PCI_REG_WRITE(rxq->qrx_tail, rx_id);  }
> >
> > +/**
> > + * vPMD raw receive routine, only accept(nb_pkts >=
> > +ICE_DESCS_PER_LOOP_AVX)
> > + *
> > + * Notice:
> > + * - nb_pkts < ICE_DESCS_PER_LOOP_AVX, just return no packet
> > + * - floor align nb_pkts to a ICE_DESCS_PER_LOOP_AVX power-of-two  */
> 
> The comment is misleading, it looks like we are going to floor align nb_pkts to
> 2^8, better to reword .
> 

It should be, agree.

> >  static inline uint16_t
> >  _ice_recv_raw_pkts_vec_avx2(struct ice_rx_queue *rxq, struct rte_mbuf
> > **rx_pkts,
> >  			    uint16_t nb_pkts, uint8_t *split_packet)  { -#define
> > ICE_DESCS_PER_LOOP_AVX 8
> > -
> >  	const uint32_t *ptype_tbl = rxq->vsi->adapter->ptype_tbl;
> >  	const __m256i mbuf_init = _mm256_set_epi64x(0, 0,
> >  			0, rxq->mbuf_initializer);
> > @@ -603,10 +608,6 @@ _ice_recv_raw_pkts_vec_avx2(struct
> ice_rx_queue
> > *rxq, struct rte_mbuf **rx_pkts,
> >  	return received;
> >  }
> >
> > -/*
> > - * Notice:
> > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> > - */
> >  uint16_t
> >  ice_recv_pkts_vec_avx2(void *rx_queue, struct rte_mbuf **rx_pkts,
> >  		       uint16_t nb_pkts)
> > @@ -616,8 +617,6 @@ ice_recv_pkts_vec_avx2(void *rx_queue, struct
> > rte_mbuf **rx_pkts,
> >
> >  /**
> >   * vPMD receive routine that reassembles single burst of 32 scattered
> > packets
> > - * Notice:
> > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> >   */
> 
> Why we need to remove this? is it still true for this function?
> 

The reason is that this comment is in the calling function " _ice_recv_raw_pkts_vec_avx2" which process the related thing, no need to add it more and more in the caller function. 

> >  static uint16_t
> >  ice_recv_scattered_burst_vec_avx2(void *rx_queue, struct rte_mbuf
> > **rx_pkts, @@ -626,6 +625,9 @@
> ice_recv_scattered_burst_vec_avx2(void
> > *rx_queue, struct rte_mbuf **rx_pkts,
> >  	struct ice_rx_queue *rxq = rx_queue;
> >  	uint8_t split_flags[ICE_VPMD_RX_BURST] = {0};
> >
> > +	/* split_flags only can support max of ICE_VPMD_RX_BURST */
> > +	nb_pkts = RTE_MIN(nb_pkts, ICE_VPMD_RX_BURST);
> 
> Is this necessary?  the only consumer of this function is
> ice_recv_scattered_pkts_vec_avx2, I think nb_pkts <=
> ICE_VPMD_RX_BURST it already be guaranteed.

The reason is that we remove "nb_pkts <= ICE_VPMD_RX_BURST" and in this function split_flags have a limit for ICE_VPMD_RX_BURST, so a checking is need in the function.

> > +
> >  	/* get some new buffers */
> >  	uint16_t nb_bufs = _ice_recv_raw_pkts_vec_avx2(rxq, rx_pkts,
> nb_pkts,
> >  						       split_flags);
> > @@ -657,9 +659,6 @@ ice_recv_scattered_burst_vec_avx2(void
> *rx_queue,
> > struct rte_mbuf **rx_pkts,
> >
> >  /**
> >   * vPMD receive routine that reassembles scattered packets.
> > - * Main receive routine that can handle arbitrary burst sizes
> > - * Notice:
> > - * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> >   */
> 
> Why we need to remove this? isn't it the main routine that be able to handle
> arbitrary burst size?
> 

The question is why we need to said the arbitrary sizes if we process and return what we could receive packet for maximum? It is not only useless comment but also maybe bring some confuse I think. 

> Btw, I will suggest all AVX2 changes can be in a separate patch, because this
> looks like some code clean and fix.
> its not related with the main purpose of the patch set.

I consider it and ask any objection before, so totally I am not disagree on separate it, but I think if  the purpose of the patch set is to clean some misleading for vec(sse/avx) burst, it could still be on a set even separate it to patch.