> -----Original Message----- > From: Ferruh Yigit <[email protected]> > Sent: Wednesday, October 19, 2022 21:47 > To: Guo, Junfeng <[email protected]>; Zhang, Qi Z > <[email protected]>; Wu, Jingjing <[email protected]> > Cc: [email protected]; [email protected]; Li, Xiaoyun > <[email protected]>; [email protected]; Richardson, Bruce > <[email protected]>; Lin, Xueqin <[email protected]> > 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 <[email protected]> > > Signed-off-by: Junfeng Guo <[email protected]> > > <...> > > > +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!
