app/testpmd: skip stopped queues when forwarding

  After "port <port_id> rxq|txq <queue_id> stop"
the stopped queue was used in forwarding nonetheless,
which may cause undefined behavior in the PMD.

Record the configured queue state
and account for it when launching forwarding as follows:
+--------+---------+-----------------+---------------+
|RxQ     |TxQ      |Configured mode  |Launch routine |
+--------+---------+-----------------+---------------+
|stopped |stopped  |*                |-              |
|stopped |started  |txonly           |(configured)   |
|stopped |started  |*                |-              |
|started |stopped  |*                |rxonly         |
|started |started  |*                |(configured)   |
+--------+---------+-----------------+---------------+
Display stopped queues on "show port config rxtx".

Fixes: 5f4ec54f1d16 ("testpmd: queue start and stop")
Cc: jing.d.chen@intel.com
Cc: stable@dpdk.org

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Reviewed-by: Raslan Darawsheh <rasland@nvidia.com>
---
 app/test-pmd/cmdline.c |  8 ++++++++
 app/test-pmd/config.c  |  6 ++++++
 app/test-pmd/testpmd.c | 18 ++++++++++++++++--
 app/test-pmd/testpmd.h | 10 ++++++++++
 4 files changed, 40 insertions(+), 2 deletions(-)
  

> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Thursday, January 13, 2022 12:21 PM

Hi Aman, Xiaoyun, Yuying,

Any comments on the proposed behavior or the code?
  

Hi Dmitry,

Thanks for the patch.

On 1/13/2022 2:51 PM, Dmitry Kozlyuk wrote:
> After "port <port_id> rxq|txq <queue_id> stop"
> the stopped queue was used in forwarding nonetheless,
> which may cause undefined behavior in the PMD.
>
> Record the configured queue state
> and account for it when launching forwarding as follows:
> +--------+---------+-----------------+---------------+
> |RxQ     |TxQ      |Configured mode  |Launch routine |
> +--------+---------+-----------------+---------------+
> |stopped |stopped  |*                |-              |
> |stopped |started  |txonly           |(configured)   |
> |stopped |started  |*                |-              |
> |started |stopped  |*                |rxonly         |
> |started |started  |*                |(configured)   |
> +--------+---------+-----------------+---------------+
> Display stopped queues on "show port config rxtx".
>
> Fixes: 5f4ec54f1d16 ("testpmd: queue start and stop")
> Cc: jing.d.chen@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Reviewed-by: Raslan Darawsheh <rasland@nvidia.com>
> ---
>   app/test-pmd/cmdline.c |  8 ++++++++
>   app/test-pmd/config.c  |  6 ++++++
>   app/test-pmd/testpmd.c | 18 ++++++++++++++++--
>   app/test-pmd/testpmd.h | 10 ++++++++++
>   4 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index e626b1c7d9..8b0920e23d 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2702,6 +2702,14 @@ cmd_config_rxtx_queue_parsed(void *parsed_result,
>   
>   	if (ret == -ENOTSUP)
>   		fprintf(stderr, "Function not supported in PMD\n");
> +	if (ret == 0) {
> +		struct rte_port *port;
> +		struct queue_state *states;
> +
> +		port = &ports[res->portid];
> +		states = isrx ? port->rxq_state : port->txq_state;
> +		states[res->qid].stopped = !isstart;
> +	}
>   }
>   
>   cmdline_parse_token_string_t cmd_config_rxtx_queue_port =
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 1722d6c8f8..7ce9cb483a 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2817,6 +2817,9 @@ rxtx_config_display(void)
>   				       rx_conf->share_qid);
>   			printf("\n");
>   		}
> +		for (qid = 0; qid < nb_rxq; qid++)
> +			if (ports[pid].rxq_state[qid].stopped)
> +				printf("    RX queue %d is stopped\n", qid);
>   
>   		/* per tx queue config only for first queue to be less verbose */
>   		for (qid = 0; qid < 1; qid++) {
> @@ -2850,6 +2853,9 @@ rxtx_config_display(void)
>   			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
>   				offloads_tmp, tx_rs_thresh_tmp);
>   		}
> +		for (qid = 0; qid < nb_txq; qid++)
> +			if (ports[pid].txq_state[qid].stopped)
> +				printf("    TX queue %d is stopped\n", qid);
>   	}
>   }
>   
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 6c387bde84..36ff845181 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2152,6 +2152,8 @@ flush_fwd_rx_queues(void)
>   		for (rxp = 0; rxp < cur_fwd_config.nb_fwd_ports; rxp++) {
>   			for (rxq = 0; rxq < nb_rxq; rxq++) {
>   				port_id = fwd_ports_ids[rxp];
> +				if (ports[port_id].rxq_state[rxq].stopped)
> +					continue;
>   				/**
>   				* testpmd can stuck in the below do while loop
>   				* if rte_eth_rx_burst() always returns nonzero
> @@ -2223,8 +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;
> +	/* 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.
> +	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. */
>   };
>   
>   /**
  

Hi Dmitry,

> -----Original Message-----
> From: Singh, Aman Deep <aman.deep.singh@intel.com>
> Sent: Thursday, February 3, 2022 9:52 PM
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; jing.d.chen@intel.com; stable@dpdk.org; Raslan
> Darawsheh <rasland@nvidia.com>
> Subject: Re: [PATCH] app/testpmd: skip stopped queues when forwarding
> 
> Hi Dmitry,
> 
> Thanks for the patch.
> 
> On 1/13/2022 2:51 PM, Dmitry Kozlyuk wrote:
> > After "port <port_id> rxq|txq <queue_id> stop"
> > the stopped queue was used in forwarding nonetheless, which may cause
> > undefined behavior in the PMD.
> >
> > Record the configured queue state
> > and account for it when launching forwarding as follows:
> > +--------+---------+-----------------+---------------+
> > |RxQ     |TxQ      |Configured mode  |Launch routine |
> > +--------+---------+-----------------+---------------+
> > |stopped |stopped  |*                |-              |
> > |stopped |started  |txonly           |(configured)   |
> > |stopped |started  |*                |-              |
> > |started |stopped  |*                |rxonly         |
> > |started |started  |*                |(configured)   |
> > +--------+---------+-----------------+---------------+
> > Display stopped queues on "show port config rxtx".
> >
> > Fixes: 5f4ec54f1d16 ("testpmd: queue start and stop")
> > Cc: jing.d.chen@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Reviewed-by: Raslan Darawsheh <rasland@nvidia.com>
> > ---
> >   app/test-pmd/cmdline.c |  8 ++++++++
> >   app/test-pmd/config.c  |  6 ++++++
> >   app/test-pmd/testpmd.c | 18 ++++++++++++++++--
> >   app/test-pmd/testpmd.h | 10 ++++++++++
> >   4 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > e626b1c7d9..8b0920e23d 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -2702,6 +2702,14 @@ cmd_config_rxtx_queue_parsed(void
> > *parsed_result,
> >
> >   	if (ret == -ENOTSUP)
> >   		fprintf(stderr, "Function not supported in PMD\n");
> > +	if (ret == 0) {
> > +		struct rte_port *port;
> > +		struct queue_state *states;
> > +
> > +		port = &ports[res->portid];
> > +		states = isrx ? port->rxq_state : port->txq_state;
> > +		states[res->qid].stopped = !isstart;
> > +	}
> >   }
> >
> >   cmdline_parse_token_string_t cmd_config_rxtx_queue_port = diff --git
> > a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 1722d6c8f8..7ce9cb483a 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -2817,6 +2817,9 @@ rxtx_config_display(void)
> >   				       rx_conf->share_qid);
> >   			printf("\n");
> >   		}
> > +		for (qid = 0; qid < nb_rxq; qid++)
> > +			if (ports[pid].rxq_state[qid].stopped)
> > +				printf("    RX queue %d is stopped\n", qid);
> >
> >   		/* per tx queue config only for first queue to be less verbose
> */
> >   		for (qid = 0; qid < 1; qid++) {
> > @@ -2850,6 +2853,9 @@ rxtx_config_display(void)
> >   			printf("      TX offloads=0x%"PRIx64" - TX RS bit
> threshold=%d\n",
> >   				offloads_tmp, tx_rs_thresh_tmp);
> >   		}
> > +		for (qid = 0; qid < nb_txq; qid++)
> > +			if (ports[pid].txq_state[qid].stopped)
> > +				printf("    TX queue %d is stopped\n", qid);
> >   	}
> >   }
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 6c387bde84..36ff845181 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2152,6 +2152,8 @@ flush_fwd_rx_queues(void)
> >   		for (rxp = 0; rxp < cur_fwd_config.nb_fwd_ports; rxp++) {
> >   			for (rxq = 0; rxq < nb_rxq; rxq++) {
> >   				port_id = fwd_ports_ids[rxp];
> > +				if (ports[port_id].rxq_state[rxq].stopped)
> > +					continue;
> >   				/**
> >   				* testpmd can stuck in the below do while
> loop
> >   				* if rte_eth_rx_burst() always returns
> nonzero @@ -2223,8
> > +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?
> > +	/* 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.
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.
> > +	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.
> >   };
> >
> >   /**
  

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];
};
  

Hi Dmitry

Sorry to be direct, but I don't think this patch makes sense.
I need the code to clarify the problems so I'll use this thread not the one you're answering.

> -----Original Message-----
> From: Zhang, Yuying <yuying.zhang@intel.com>
> Sent: Wednesday, February 9, 2022 09:00
> To: Singh, Aman Deep <aman.deep.singh@intel.com>; Dmitry Kozlyuk
> <dkozlyuk@nvidia.com>; dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; jing.d.chen@intel.com;
> stable@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: RE: [PATCH] app/testpmd: skip stopped queues when forwarding
> 
> Hi Dmitry,
> 
> > -----Original Message-----
> > From: Singh, Aman Deep <aman.deep.singh@intel.com>
> > Sent: Thursday, February 3, 2022 9:52 PM
> > To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; Zhang, Yuying
> > <yuying.zhang@intel.com>; jing.d.chen@intel.com; stable@dpdk.org;
> > Raslan Darawsheh <rasland@nvidia.com>
> > Subject: Re: [PATCH] app/testpmd: skip stopped queues when forwarding
> >
> > Hi Dmitry,
> >
> > Thanks for the patch.
> >
> > On 1/13/2022 2:51 PM, Dmitry Kozlyuk wrote:
> > > After "port <port_id> rxq|txq <queue_id> stop"
> > > the stopped queue was used in forwarding nonetheless, which may
> > > cause undefined behavior in the PMD.
> > >
> > > Record the configured queue state
> > > and account for it when launching forwarding as follows:
> > > +--------+---------+-----------------+---------------+
> > > |RxQ     |TxQ      |Configured mode  |Launch routine |
> > > +--------+---------+-----------------+---------------+
> > > |stopped |stopped  |*                |-              |
> > > |stopped |started  |txonly           |(configured)   |
> > > |stopped |started  |*                |-              |
> > > |started |stopped  |*                |rxonly         |
> > > |started |started  |*                |(configured)   |
> > > +--------+---------+-----------------+---------------+
> > > Display stopped queues on "show port config rxtx".
> > >
> > > Fixes: 5f4ec54f1d16 ("testpmd: queue start and stop")
> > > Cc: jing.d.chen@intel.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > > Reviewed-by: Raslan Darawsheh <rasland@nvidia.com>
> > > ---
> > >   app/test-pmd/cmdline.c |  8 ++++++++
> > >   app/test-pmd/config.c  |  6 ++++++
> > >   app/test-pmd/testpmd.c | 18 ++++++++++++++++--
> > >   app/test-pmd/testpmd.h | 10 ++++++++++
> > >   4 files changed, 40 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > e626b1c7d9..8b0920e23d 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c

<snip>

> > > +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;

1. For each lcore, there may be many fwd streams which are different queue pair even different port pair.
Just imagine the simplest case, you run iofwd with 4 ports single queue with single forwarding core.
But you're only dealing with the first fwd stream here. Each fc actually includes stream from fc->stream_idx to fc->stream_idx+fc->stream_nb-1.

What are you going to do with this?
If you stop port 0 rxq 0, you shouldn't change the whole lcore behavior to rxonly. Because the pair of port 2 rxq 0-> port 3 txq 0 and port 3 rxq0 -> port 2 txq0 should be normal iofwd.
And this is only the basic case.
And I don't think it's that harmful to just let those fwd running even if you stop some rxq. Because their pair won't have pkts to do tx if there's no rx.

2. Even if you're going to narrow down for the case which fc->nb_stream=1.
You keep the value of cur_fwd_config. There's a potential issue here since cur_fwd_config is not really cur_fwd_config anymore.
In stop_packet_forwarding(), It sets fc->stopped=1 and uses cur_fwd_config.fwd_eng->port_fwd_end to stop packeting forward.
Right now, in most of the fwd mode, port_fwd_end=NULL. But txonly and noisy_vnf and icmp_echo are not.
What if someday rxonly engine needs to have a port_fwd_end? Should you call port_fwd_end for your lcore (rxonly) too while others are still cur_fwd_engine?

Also, in this case, shouldn't you init "stopped" (or "state" following other comments) to be 1 for txonly? Because the default value will be 0, following your code, txonly engine will change to rxonly if the txq is stopped.

Also, even if you init "stopped" for txonly, have you considered flow_gen engine? Flow_gen is also kind of a txonly engine (all received pkts will be dropped) to generate multi-flow packets.


Anyway, I still think it's not worth it. And there may be other issues too. The patch looks dangerous to me.

BRs
Xiaoyun

> > > +
> > > +	/* 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?
> > > +	/* 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.
> 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.
> > > +	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.
> > >   };
> > >
> > >   /**
  

- jing.d.chen@intel.com (invalid address?), +ethdev maintainers

> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> [...] 
> Sorry to be direct, but I don't think this patch makes sense.
> I need the code to clarify the problems so I'll use this thread not
> the one you're answering.

Thank you for explaining the issues in great detail!
I'd like to understand one crucial point below
to decide whether testpmd needs any changes at all.

> [...]
> And I don't think it's that harmful to just let those fwd running even
> if you stop some rxq. Because their pair won't have pkts to do tx if
> there's no rx.

This patch was created with the assumption
that stopped queues may not be used for Rx/Tx.
No-op behavior of rte_eth_rx/tx_burst()
for a stopped queue is not documented.
Yes, at least some PMDs implement it this way.
But is this behavior intended?

It is the application that stops the queue or starts it deferred.
Stopping is non-atomic, so polling the queue is not allowed during it.
Hence, if the application intends to stop queues, it must either:

a) Know the queue is not polled before stopping it.
   Use case: a secondary that was polling the queue has crashed,
   the primary is doing a recovery to free all mbufs.
   There is no issue since there is no poller to touch the queue.

b) Tell the poller to skip the queue for the time of stopping it.
   Use case: deferred queue start or queue reconfiguration.
   But if the poller is cooperating anyway,
   what prevents it from not touching the queue for longer?

The same considerations apply to starting a queue.

No-op behavior is convenient from the application point of view.
But it also means that pollers of stopped queues
will go all the way down to PMD Rx/Tx routines, wasting cycles,
and some PMDs will do a check for the queue state,
even though it may never be needed for a particular application.
  

Andrew, Ferruh, Thomas, which behavior does ethdev assume (see below)?

> This patch was created with the assumption
> that stopped queues may not be used for Rx/Tx.
> No-op behavior of rte_eth_rx/tx_burst()
> for a stopped queue is not documented.
> Yes, at least some PMDs implement it this way.
> But is this behavior intended?
> 
> It is the application that stops the queue or starts it deferred.
> Stopping is non-atomic, so polling the queue is not allowed during it.
> Hence, if the application intends to stop queues, it must either:
> 
> a) Know the queue is not polled before stopping it.
>    Use case: a secondary that was polling the queue has crashed,
>    the primary is doing a recovery to free all mbufs.
>    There is no issue since there is no poller to touch the queue.
> 
> b) Tell the poller to skip the queue for the time of stopping it.
>    Use case: deferred queue start or queue reconfiguration.
>    But if the poller is cooperating anyway,
>    what prevents it from not touching the queue for longer?
> 
> The same considerations apply to starting a queue.
> 
> No-op behavior is convenient from the application point of view.
> But it also means that pollers of stopped queues
> will go all the way down to PMD Rx/Tx routines, wasting cycles,
> and some PMDs will do a check for the queue state,
> even though it may never be needed for a particular application.
  

21/02/2022 09:58, Dmitry Kozlyuk:
> Andrew, Ferruh, Thomas, which behavior does ethdev assume (see below)?

For the whole device stop, this is the documentation:
"
  The transmit and receive functions should not be invoked
  when the device is stopped.
"

There is also this comment on rte_eth_dev_reset:
"
  Note: To avoid unexpected behavior, the application should stop calling
  Tx and Rx functions before calling rte_eth_dev_reset( ). For thread
  safety, all these controlling functions should be called from the same
  thread.
"

For queue stop, there is no documented expectation.

There is this comment for queue callback removal:
"
  The memory for the callback can be
  subsequently freed back by the application by calling rte_free():
 
  - Immediately - if the port is stopped, or the user knows that no
    callbacks are in flight e.g. if called from the thread doing Rx/Tx
    on that queue.
 
  - After a short delay - where the delay is sufficient to allow any
    in-flight callbacks to complete. Alternately, the RCU mechanism can be
    used to detect when data plane threads have ceased referencing the
    callback memory.
"

> > This patch was created with the assumption
> > that stopped queues may not be used for Rx/Tx.
> > No-op behavior of rte_eth_rx/tx_burst()
> > for a stopped queue is not documented.

Indeed, it is not documented.
I suggest working on this documentation first.
testpmd could be adjusted later if needed.

> > Yes, at least some PMDs implement it this way.
> > But is this behavior intended?
> > 
> > It is the application that stops the queue or starts it deferred.
> > Stopping is non-atomic, so polling the queue is not allowed during it.
> > Hence, if the application intends to stop queues, it must either:
> > 
> > a) Know the queue is not polled before stopping it.
> >    Use case: a secondary that was polling the queue has crashed,
> >    the primary is doing a recovery to free all mbufs.
> >    There is no issue since there is no poller to touch the queue.
> > 
> > b) Tell the poller to skip the queue for the time of stopping it.
> >    Use case: deferred queue start or queue reconfiguration.
> >    But if the poller is cooperating anyway,
> >    what prevents it from not touching the queue for longer?
> > 
> > The same considerations apply to starting a queue.
> > 
> > No-op behavior is convenient from the application point of view.
> > But it also means that pollers of stopped queues
> > will go all the way down to PMD Rx/Tx routines, wasting cycles,
> > and some PMDs will do a check for the queue state,
> > even though it may never be needed for a particular application.

Yes that's the question: Do we want
1/ to allow apps to poll stopped queues, adding checks in PMDs,
or do we prefer
2/ saving check cycles and expect apps to not poll?