[dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command into testpmd

[Zhao1, Wei]

Hi, Ferruh 

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 7:07 PM
> To: Zhao1, Wei <wei.zhao1 at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command
> into testpmd
> 
> On 3/3/2017 4:56 AM, Wei Zhao wrote:
> > Add vf port reset command into testpmd project, it is the interface
> > for user to reset vf port.
> 
> I think it is better to change the order of this patch, first implement new API
> in ethdev, later this patch implement new API in testpmd.
> 

You means change the order of patch 0001 and 0002 ?0003 remain the same?
Ok，I will do that in v2 patch
 
> >
> > Signed-off-by: Wei Zhao <wei.zhao1 at intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu at intel.com>
> > ---
> >  app/test-pmd/cmdline.c | 17 ++++++++++---  app/test-pmd/testpmd.c |
> > 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  app/test-pmd/testpmd.h |  1 +
> >  3 files changed, 81 insertions(+), 4 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 43fc636..59db672 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -596,6 +596,9 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> >  			"port close (port_id|all)\n"
> >  			"    Close all ports or port_id.\n\n"
> >
> > +			"port reset (port_id|all)\n"
> > +			"    Reset all ports or port_id.\n\n"
> 
> It is not clear what reset does to the port. This is only for VF right?
> Adding reset here hides that it is for VF.

Yes, it is only for VF port reset, maybe I should change cmd line command to "port vf_reset  index"
that mode to avoid ambiguous for user in v2

> 
> <...>
> 
> > @@ -601,6 +602,7 @@ init_config(void)
> >  	if (init_fwd_streams() < 0)
> >  		rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
> >
> > +
> 
> This may be unintentional.

Yes, I will fix it in v2 patch. 

> 
> <...>
> 
> > @@ -1350,6 +1363,10 @@ start_port(portid_t pid)
> >  				return -1;
> >  			}
> >  		}
> > +
> > +		/* register reset interrupt callback */
> > +		rte_eth_dev_callback_register(pi,
> RTE_ETH_EVENT_INTR_RESET,
> > +			reset_event_callback, NULL);
> 
> So each port started will register a callback to handle reset events,
> 
> 1- isn't this overkill for the usecases that does not need this reset?
> 2- should there be an unregister event?
> 3- This issue can be fixed in testpmd, but for user application, is this the
> suggested way?

1. it is hard to know which port may be need to reset in the feature at the beginning stage, so there is  register event for all ports.
2. this is only a demo for user application. I think in user application, there should be a specific thread to check whether reset_ports is set by 
 I40e function _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RESET, NULL),  so  RTE_ETH_EVENT_INTR_RESET must be registered.
 IF true, then user application should call function rte_eth_dev_reset(pi)
3. . this is only a demo for user application, user application can create a new specific thread to do this work , but testpmd app can not because that may be introduce 
   A lot of unknow problem for existing feature.

> 
> >  		if (port->need_reconfig_queues > 0) {
> >  			port->need_reconfig_queues = 0;
> >  			/* setup tx queues */
> > @@ -1559,6 +1576,56 @@ close_port(portid_t pid)  }
> >
> >  void
> > +reset_port(portid_t pid)
> > +{
> > +	portid_t pi;
> > +	struct rte_port *port;
> > +
> > +	if (port_id_is_invalid(pid, ENABLED_WARN))
> > +		return;
> > +
> > +	printf("Closing ports...\n");
> > +
> > +	FOREACH_PORT(pi, ports) {
> 
> Since we already know the port_id (pid), why iterating through all ports?

This is usefully, because when user command is “port reset all”, the port_id is 0xffffffff
If all of these ports are vf, then it will need to retrieve all port.

> 
> > +		if (pid != pi && pid != (portid_t)RTE_PORT_ALL)
> > +			continue;
> > +
> > +		if (port_is_forwarding(pi) != 0 && test_done == 0) {
> > +			printf("Please remove port %d from forwarding "
> > +					"configuration.\n", pi);
> > +			continue;
> > +		}
> > +
> > +		if (port_is_bonding_slave(pi)) {
> > +			printf("Please remove port %d from "
> > +					"bonded device.\n", pi);
> > +			continue;
> > +		}
> > +
> > +		if (!reset_ports[pi]) {
> > +			printf("vf must get reset port %d info from "
> > +					"pf before reset.\n", pi);
> > +			continue;
> > +		}
> 
> Can there be a timing issue here? Is it possible that reset occurred already
> and we are in the middle of the callback function when this check done?
Process:
1. pf reset, and pass message to vf.
2. registered callback function of RTE_ETH_EVENT_INTR_RESET event set flag of  reset_ports[pi], and give a printf message of port index
3. then user use command of testpmd cli " port reset index" to reset vf, and reset port and set flag to 0.

So, I do not find any timing issue by now.

> 
> <...>