> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Wednesday, October 19, 2022 21:47
> To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z
> <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> Cc: ferruh.yi...@xilinx.com; dev@dpdk.org; Li, Xiaoyun
> <xiaoyun...@intel.com>; awogbem...@google.com; Richardson, Bruce
> <bruce.richard...@intel.com>; Lin, Xueqin <xueqin....@intel.com>
> Subject: Re: [PATCH v5 8/8] net/gve: add support for Rx/Tx
>
> On 10/10/2022 11:17 AM, Junfeng Guo wrote:
>
> >
> > Add Rx/Tx of GQI_QPL queue format and GQI_RDA queue format.
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng....@intel.com>
>
> <...>
>
> > +uint16_t
> > +gve_rx_burst(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t
> nb_pkts)
> > +{
> > + volatile struct gve_rx_desc *rxr, *rxd;
> > + struct gve_rx_queue *rxq = rx_queue;
> > + uint16_t rx_id = rxq->rx_tail;
> > + struct rte_mbuf *rxe;
> > + uint16_t nb_rx, len;
> > + uint64_t addr;
> > +
> > + rxr = rxq->rx_desc_ring;
> > +
> > + for (nb_rx = 0; nb_rx < nb_pkts; nb_rx++) {
> > + rxd = &rxr[rx_id];
> > + if (GVE_SEQNO(rxd->flags_seq) != rxq->expected_seqno)
> > + break;
> > +
> > + if (rxd->flags_seq & GVE_RXF_ERR)
> > + continue;
> > +
>
> I think above code wrong.
> Function returns 'nb_rx', if you continue on error 'nb_rx' kept
> increased, so application will receive wrong number of packets.
>
> Also packets assigned as "rx_pkts[nb_rx] = rxe;", this will cause gaps
> in the arrays.
>
> You can either break on the error, or keep another variable to store
> number of received packets.
Thanks for pointing out this!
Yes, we will use another variable as index to go through all the nb_pkts
and use nb_rx to store all valid pkts. In this way, there would be no gap
in the rx_pkts array and the size can align with the final returned nb_rx.
Thanks!
>
> > + len = rte_be_to_cpu_16(rxd->len) - GVE_RX_PAD;
> > + rxe = rxq->sw_ring[rx_id];
> > + rxe->data_off = RTE_PKTMBUF_HEADROOM;
>
> As far as I can see mbufs allocated using 'rte_pktmbuf_alloc()', if so
> no need to explicitly set the 'm->data_off'.
Will update this, thanks!
