Re: [dpdk-dev] [PATCH v3 1/2] app/testpmd: update forward engine beginning

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

Hi

> -----Original Message-----
> From: Zhang, AlvinX <alvinx.zhang@intel.com>
> Sent: Saturday, September 18, 2021 11:07
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: [PATCH v3 1/2] app/testpmd: update forward engine beginning
> 
> For each forward engine, there may be some special conditions must be meet

met

> before the forwarding run.

is running / runs

> 
> Adding checks for these conditions in configuring is not suitable, because one
> condition may rely on multiple configurations, and the conditions required by
> each forward engine is not general.
> 
> The best solution is each forward engine has a callback to check whether these
> conditions are meet, and then testpmd can call the callback to determine

met

> whether the forwarding can be started.
> 
> There was a void callback 'port_fwd_begin' in forward engine, it did some
> initialization for forwarding, this patch updates it's return type, then we can add

its return value then we can

> some checks in it to confirm whether the forwarding can be started. In addition,
> this patch puts the calling part of the callback up to before some forwarding
> related status being set.

this patch calls this callback before the forwarding stats is reset and then launches the forwarding engine.

Also the cc stable issue I state in the 2nd patch.
But overall the patch looks good to me.

BRs
Xiaoyun

> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>  app/test-pmd/flowgen.c     |  3 ++-
>  app/test-pmd/ieee1588fwd.c |  3 ++-
>  app/test-pmd/noisy_vnf.c   |  4 +++-
>  app/test-pmd/testpmd.c     | 38 ++++++++++++++++++++++++++------------
>  app/test-pmd/testpmd.h     |  2 +-
>  app/test-pmd/txonly.c      |  3 ++-
>  6 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/app/test-pmd/flowgen.c b/app/test-pmd/flowgen.c index
> 0d3664a..83234fc 100644
> --- a/app/test-pmd/flowgen.c
> +++ b/app/test-pmd/flowgen.c
> @@ -201,10 +201,11 @@
>  	get_end_cycles(fs, start_tsc);
>  }
> 
> -static void
> +static int
>  flowgen_begin(portid_t pi)
>  {
>  	printf("  number of flows for port %u: %d\n", pi, nb_flows_flowgen);
> +	return 0;
>  }
> 
>  struct fwd_engine flow_gen_engine = {
> diff --git a/app/test-pmd/ieee1588fwd.c b/app/test-pmd/ieee1588fwd.c index
> 034f238..81624a7 100644
> --- a/app/test-pmd/ieee1588fwd.c
> +++ b/app/test-pmd/ieee1588fwd.c
> @@ -198,10 +198,11 @@ struct ptpv2_msg {
>  	port_ieee1588_tx_timestamp_check(fs->rx_port);
>  }
> 
> -static void
> +static int
>  port_ieee1588_fwd_begin(portid_t pi)
>  {
>  	rte_eth_timesync_enable(pi);
> +	return 0;
>  }
> 
>  static void
> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c index
> 382a4c2..e4434be 100644
> --- a/app/test-pmd/noisy_vnf.c
> +++ b/app/test-pmd/noisy_vnf.c
> @@ -231,7 +231,7 @@ struct noisy_config {
>  	rte_free(noisy_cfg[pi]);
>  }
> 
> -static void
> +static int
>  noisy_fwd_begin(portid_t pi)
>  {
>  	struct noisy_config *n;
> @@ -273,6 +273,8 @@ struct noisy_config {
>  		rte_exit(EXIT_FAILURE,
>  			 "--noisy-lkup-memory-size must be > 0\n");
>  	}
> +
> +	return 0;
>  }
> 
>  struct fwd_engine noisy_vnf_engine = {
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 97ae52e..0345b2e 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2172,16 +2172,10 @@ struct extmem_param {  static void
> launch_packet_forwarding(lcore_function_t *pkt_fwd_on_lcore)  {
> -	port_fwd_begin_t port_fwd_begin;
>  	unsigned int i;
>  	unsigned int lc_id;
>  	int diag;
> 
> -	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> -	if (port_fwd_begin != NULL) {
> -		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++)
> -			(*port_fwd_begin)(fwd_ports_ids[i]);
> -	}
>  	for (i = 0; i < cur_fwd_config.nb_fwd_lcores; i++) {
>  		lc_id = fwd_lcores_cpuids[i];
>  		if ((interactive == 0) || (lc_id != rte_lcore_id())) { @@ -2227,10
> +2221,35 @@ struct extmem_param {
>  		fprintf(stderr, "Packet forwarding already started\n");
>  		return;
>  	}
> -	test_done = 0;
> 
>  	fwd_config_setup();
> 
> +	port_fwd_begin = cur_fwd_config.fwd_eng->port_fwd_begin;
> +	if (port_fwd_begin != NULL) {
> +		for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
> +			if (port_fwd_begin(fwd_ports_ids[i])) {
> +				fprintf(stderr,
> +					"Packet forwarding not ready\n");
> +				return;
> +			}
> +		}
> +	}
> +
> +	if (with_tx_first) {
> +		port_fwd_begin = tx_only_engine.port_fwd_begin;
> +		if (port_fwd_begin != NULL) {
> +			for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
> +				if (port_fwd_begin(fwd_ports_ids[i])) {
> +					fprintf(stderr,
> +						"Packet forwarding not
> ready\n");
> +					return;
> +				}
> +			}
> +		}
> +	}
> +
> +	test_done = 0;
> +
>  	if(!no_flush_rx)
>  		flush_fwd_rx_queues();
> 
> @@ -2239,11 +2258,6 @@ struct extmem_param {
> 
>  	fwd_stats_reset();
>  	if (with_tx_first) {
> -		port_fwd_begin = tx_only_engine.port_fwd_begin;
> -		if (port_fwd_begin != NULL) {
> -			for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++)
> -				(*port_fwd_begin)(fwd_ports_ids[i]);
> -		}
>  		while (with_tx_first--) {
>  			launch_packet_forwarding(
>  					run_one_txonly_burst_on_core);
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> 5863b2f..e9d9db0 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -268,7 +268,7 @@ struct fwd_lcore {
>   *     Forwards packets unchanged on the same port.
>   *     Check that sent IEEE1588 PTP packets are timestamped by the hardware.
>   */
> -typedef void (*port_fwd_begin_t)(portid_t pi);
> +typedef int (*port_fwd_begin_t)(portid_t pi);
>  typedef void (*port_fwd_end_t)(portid_t pi);  typedef void
> (*packet_fwd_t)(struct fwd_stream *fs);
> 
> diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c index
> aed820f..386a4ff 100644
> --- a/app/test-pmd/txonly.c
> +++ b/app/test-pmd/txonly.c
> @@ -435,7 +435,7 @@
>  	get_end_cycles(fs, start_tsc);
>  }
> 
> -static void
> +static int
>  tx_only_begin(portid_t pi)
>  {
>  	uint16_t pkt_data_len;
> @@ -467,6 +467,7 @@
>  		timestamp_init_req++;
>  	/* Make sure all settings are visible on forwarding cores.*/
>  	rte_wmb();
> +	return 0;
>  }
> 
>  struct fwd_engine tx_only_engine = {
> --
> 1.8.3.1