Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
> Sent: Friday, March 22, 2019 5:43 PM
> To: Lu, Wenzhuo <wenzhuo...@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.
