RE: [PATCH v6 8/8] net/gve: add support for Rx/Tx

["Guo, Junfeng" <junfeng.guo@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, October 20, 2022 22:47
> To: Guo, Junfeng <junfeng.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>;
> awogbemila@google.com; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
> stephen@networkplumber.org; Xia, Chenbo <chenbo.xia@intel.com>;
> Zhang, Helin <helin.zhang@intel.com>
> Subject: Re: [PATCH v6 8/8] net/gve: add support for Rx/Tx
> 
> On 10/20/2022 11:36 AM, Junfeng Guo wrote:
> 
> >
> > Add Rx/Tx of GQI_QPL queue format and GQI_RDA queue format.
> >
> > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng.guo@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;
> > +       uint16_t i;
> > +
> > +       rxr = rxq->rx_desc_ring;
> > +       nb_rx = 0;
> > +
> > +       for (i = 0; i < nb_pkts; i++) {
> > +               rxd = &rxr[rx_id];
> > +               if (GVE_SEQNO(rxd->flags_seq) != rxq->expected_seqno)
> > +                       break;
> > +
> > +               if (rxd->flags_seq & GVE_RXF_ERR)
> > +                       continue;
> > +
> > +               len = rte_be_to_cpu_16(rxd->len) - GVE_RX_PAD;
> > +               rxe = rxq->sw_ring[rx_id];
> > +               if (rxq->is_gqi_qpl) {
> > +                       addr = (uint64_t)(rxq->qpl->mz->addr) + rx_id * PAGE_SIZE
> + GVE_RX_PAD;
> > +                       rte_memcpy((void *)((size_t)rxe->buf_addr + rxe-
> >data_off),
> > +                                  (void *)(size_t)addr, len);
> > +               }
> > +               rxe->pkt_len = len;
> > +               rxe->data_len = len;
> > +               rxe->port = rxq->port_id;
> > +               rxe->ol_flags = 0;
> > +
> > +               if (rxd->flags_seq & GVE_RXF_TCP)
> > +                       rxe->packet_type |= RTE_PTYPE_L4_TCP;
> > +               if (rxd->flags_seq & GVE_RXF_UDP)
> > +                       rxe->packet_type |= RTE_PTYPE_L4_UDP;
> > +               if (rxd->flags_seq & GVE_RXF_IPV4)
> > +                       rxe->packet_type |= RTE_PTYPE_L3_IPV4;
> > +               if (rxd->flags_seq & GVE_RXF_IPV6)
> > +                       rxe->packet_type |= RTE_PTYPE_L3_IPV6;
> > +
> > +               if (gve_needs_rss(rxd->flags_seq)) {
> > +                       rxe->ol_flags |= RTE_MBUF_F_RX_RSS_HASH;
> > +                       rxe->hash.rss = rte_be_to_cpu_32(rxd->rss_hash);
> 
> You are updating "m->hash.rss" anyway, and if this is without and cost
> you can force enable as done in previous version:
> 'dev->data->dev_conf.rxmode.offloads |=
> RTE_ETH_RX_OFFLOAD_RSS_HASH;'

Yes, it seems the RSS is enabled by default with no obvious perf loss.
There is no RSS init stage. We will also force enable this in the dev config
stage. Thanks!

> 
> <...>
> 
> > +static inline void
> > +gve_free_bulk_mbuf(struct rte_mbuf **txep, int num)
> > +{
> > +       struct rte_mbuf *m, *free[GVE_TX_MAX_FREE_SZ];
> > +       int nb_free = 0;
> > +       int i, s;
> > +
> > +       if (unlikely(num == 0))
> > +               return;
> > +
> > +       /* Find the 1st mbuf which needs to be free */
> > +       for (s = 0; s < num; s++) {
> > +               if (txep[s] != NULL) {
> > +                       m = rte_pktmbuf_prefree_seg(txep[s]);
> > +                       if (m != NULL)
> > +                               break;
> > +                       }
> 
> '}' indentation is wrong.

Thanks for the catch! Will update in the coming version.

> 
> <...>
> 
> > +static inline void
> > +gve_tx_clean_swr_qpl(struct gve_tx_queue *txq)
> > +{
> > +       uint32_t start = txq->sw_ntc;
> > +       uint32_t ntc, nb_clean;
> > +
> > +       ntc = txq->sw_tail;
> > +
> > +       if (ntc == start)
> > +               return;
> > +
> > +       /* if wrap around, free twice. */
> > +       if (ntc < start) {
> > +               nb_clean = txq->nb_tx_desc - start;
> > +               if (nb_clean > GVE_TX_MAX_FREE_SZ)
> > +                       nb_clean = GVE_TX_MAX_FREE_SZ;
> > +               gve_free_bulk_mbuf(&txq->sw_ring[start], nb_clean);
> > +
> > +               txq->sw_nb_free += nb_clean;
> > +               start += nb_clean;
> > +               if (start == txq->nb_tx_desc)
> > +                       start = 0;
> > +               txq->sw_ntc = start;
> > +       }
> > +
> > +       if (ntc > start) {
> 
> may be can drop the 'if' block, since "ntc == start" and "ntc < start"
> cases already covered.

Sure, will drop this 'if' in the coming version. Thanks!

> 
> <...>
> 
> > +uint16_t
> > +gve_tx_burst(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t
> nb_pkts)
> > +{
> > +       struct gve_tx_queue *txq = tx_queue;
> > +
> > +       if (txq->is_gqi_qpl)
> > +               return gve_tx_burst_qpl(tx_queue, tx_pkts, nb_pkts);
> > +
> > +       return gve_tx_burst_ra(tx_queue, tx_pkts, nb_pkts);
> > +}
> > +
> 
> Can there be mix of queue types?
> If only one queue type is supported in specific config, perhaps burst
> function can be set during configuration, to prevent if check on datapath.
> 
> This is optimization and can be done later, it doesn't have to be in the
> set.

Maybe not. There are three queue format types, and we can get the real 
used one via adminq with 'priv->queue_format'. So there may not have 
mix of the queue types. And currently only GQI_QPL and GQI_RDA queue 
format are supported in PMD. Also, only GQI_QPL queue format is in use 
on GCP since GQI_RDA hasn't been released in production.

Will do some refactors for the queue types later. Thanks for the advice!