Re: [dpdk-dev] [PATCH v5 3/8] net/ice: support vector SSE in RX

["Lu, Wenzhuo" <wenzhuo.lu@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Friday, March 22, 2019 5:43 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 3/8] net/ice: support vector SSE in RX
> 
> > +
> > +static inline uint16_t
> > +reassemble_packets(struct ice_rx_queue *rxq, struct rte_mbuf
> > +**rx_bufs,
> As this is in the header file, I think it could be better to prefix it with 'ice_'. Or
> maybe with 'ice_rx_' as it seems to be rx-only.
Thanks for the comment. I'll add the prefix.

> > +static inline void
> > +_ice_rx_queue_release_mbufs_vec(struct ice_rx_queue *rxq) {
> > +	const unsigned int mask = rxq->nb_rx_desc - 1;
> > +	unsigned int i;
> > +
> > +	if (!rxq->sw_ring || rxq->rxrearm_nb >= rxq->nb_rx_desc)
> > +		return;
> 
> Maybe not a big deal, but I understand that !rxq->sw_ring is not the
> common case, more an error. If so, the if condition could be split in two, and
> having the first one tagged with unlikely.
> 
> Looking at Tx patch, you should also ensure that rxq != NULL and also print a
> debug/error message to be consistent.
Thanks for the suggestion. I'll change it.

> > +/**
> > + * Notice:
> > + * - nb_pkts < ICE_DESCS_PER_LOOP, just return no packet
> > + * - nb_pkts > ICE_VPMD_RX_BURST, only scan ICE_VPMD_RX_BURST
> > + *   numbers of DD bits
> > + */
> > +uint16_t
> > +ice_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
> > +		  uint16_t nb_pkts)
> > +{
> > +	return _recv_raw_pkts_vec(rx_queue, rx_pkts, nb_pkts, NULL);
> 
> Same as below comment.
_recv_raw_pkts_vec is used by the normal RX and scatter RX. It will be called again later in the patch 4. So, we make it an inline function.

> 
> > +}
> > +
> > +static void __attribute__((cold))
> > +ice_rx_queue_release_mbufs_vec(struct ice_rx_queue *rxq) {
> > +	_ice_rx_queue_release_mbufs_vec(rxq);
> 
> What is the point of having _ice_rx_queue_release_mbufs_vec as it is only
> called once here?
To our experience, it can be reused when the vector is implemented on other platform. So we put it in the common.h.