RE: [PATCH] app/testpmd: skip stopped queues when forwarding

[Dmitry Kozlyuk <dkozlyuk@nvidia.com>]

Hi Aman, Yuying,

You share some concerns, so I'm answering in one thread.

> From: Zhang, Yuying <yuying.zhang@intel.com>
> Sent: Wednesday, February 9, 2022 12:00 PM
[...]
> > > +2225,20 @@ run_pkt_fwd_on_lcore(struct fwd_lcore *fc,
> packet_fwd_t
> > pkt_fwd)
> > >   static int
> > >   start_pkt_forward_on_core(void *fwd_arg)
> > >   {
> > > -   run_pkt_fwd_on_lcore((struct fwd_lcore *) fwd_arg,
> > > -                        cur_fwd_config.fwd_eng->packet_fwd);
> > > +   struct fwd_lcore *fc = fwd_arg;
> > > +   struct fwd_stream *fsm = fwd_streams[fc->stream_idx];
> > > +   struct queue_state *rxq = &ports[fsm->rx_port].rxq_state[fsm-
> > >rx_queue];
> > > +   struct queue_state *txq = &ports[fsm->tx_port].txq_state[fsm-
> > >tx_queue];
> > > +   struct fwd_engine *fwd_engine = cur_fwd_config.fwd_eng;
> > > +   packet_fwd_t packet_fwd;
> > > +
> > > +   /* Check if there will ever be any packets to send. */
> > > +   if (rxq->stopped && (txq->stopped || fwd_engine !=
> > &tx_only_engine))
> > > +           return 0;
> Have you considered other fwd_engines such as io_fwd_engine and
> mac_fwd_engine?

The only engine that can send packets without receiving them is "txonly".
All other engines call rte_eth_rx_burst(),
which is illegal for a stopped RxQ even if the packets are discarded.

> > > +   /* Force rxonly mode if RxQ is started, but TxQ is stopped. */
> > > +   packet_fwd = !rxq->stopped && txq->stopped ?
> > rx_only_engine.packet_fwd
> > > +                                              : fwd_engine-
> >packet_fwd;
> > Should we have a print here for user info, that mode has been
> > changed or ignored.

Good idea.

> Why need to force rxonly mode for this situation? BTW, the value of
> cur_fwd_eng hasn't been updated after you changed forward mode.

It is logical to preserve as much workload as possible
so that stopping a TxQ does not reduce the Rx from NIC's perspective.

> > > +   run_pkt_fwd_on_lcore(fc, packet_fwd);
> > >     return 0;
> > >   }
> > >
> > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > > 2149ecd93a..2744fa4d76 100644
> > > --- a/app/test-pmd/testpmd.h
> > > +++ b/app/test-pmd/testpmd.h
> > > @@ -216,6 +216,12 @@ struct xstat_display_info {
> > >     bool     allocated;
> > >   };
> > >
> > > +/** Application state of a queue. */
> > > +struct queue_state {
> > > +   /** The queue is stopped and should not be used. */
> > > +   bool stopped;
> > > +};
> > > +
> > >   /**
> > >    * The data structure associated with each port.
> > >    */
> > > @@ -256,6 +262,10 @@ struct rte_port {
> > >     uint64_t                mbuf_dynf;
> > >     const struct rte_eth_rxtx_callback
> > *tx_set_dynf_cb[RTE_MAX_QUEUES_PER_PORT+1];
> > >     struct xstat_display_info xstats_info;
> > > +   /** Per-Rx-queue state. */
> > > +   struct queue_state rxq_state[RTE_MAX_QUEUES_PER_PORT];
> > > +   /** Per-Tx-queue state. */
> > > +   struct queue_state txq_state[RTE_MAX_QUEUES_PER_PORT];
> > Can we think of adding rxq_state/txq_state as part of existing
> structures
> > under rte_port->rte_eth_rxconf/rte_eth_txconf.
> > And if it helps, rather than bool can we use u8 with eth_dev
> defines- #define
> > RTE_ETH_QUEUE_STATE_STOPPED 0 /**< Queue stopped. */ #define
> > RTE_ETH_QUEUE_STATE_STARTED 1 /**< Queue started. */
> The same.

Will change to constants (even enum maybe?).

Struct rte_eth_{rx,tx}conf cannot be changed, because it's ethdev API,
and should not be changed, because it reflects queue configuration,
while the new arrays reflect the queue state.
What do you think of the following?

struct port_rxqueue {
	struct rte_eth_rxconf conf;
	uint8_t state; 
};

struct port_txqueue {
	struct rte_eth_txconf conf;
	uint8_t state;
};

struct rte_port {
	/* ... */
	struct port_rxqueue rxq[RTE_MAX_QUEUES_PER_PORT];
	struct port_txqueue txq[RTE_MAX_QUEUES_PER_PORT];
};