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

["Zhang, AlvinX" <alvinx.zhang@intel.com>]

> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> Sent: Saturday, September 18, 2021 4:31 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v3 1/2] app/testpmd: update forward engine beginning
> 
> 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

Hi Xiaoyun,
Thanks for your attentively reviewing. I will update them in V4.

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

BRs
Alvin