From: "Zhao1, Wei" <wei.zhao1@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command into testpmd
Date: Tue, 14 Mar 2017 07:58:03 +0000 [thread overview]
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA0205817C@PGSMSX103.gar.corp.intel.com> (raw)
In-Reply-To: <3a152048-f129-33fd-d31e-4220c505fe92@intel.com>
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 7:07 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@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@intel.com>
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@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.
>
> <...>
next prev parent reply other threads:[~2017-03-14 7:58 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-03 4:56 [dpdk-dev] [PATCH 0/3] net/i40e: vf port reset Wei Zhao
2017-03-03 4:56 ` [dpdk-dev] [PATCH 1/3] app/testpmd: add port reset command into testpmd Wei Zhao
2017-03-08 11:07 ` Ferruh Yigit
2017-03-14 7:58 ` Zhao1, Wei [this message]
2017-03-27 7:08 ` Zhao1, Wei
2017-03-03 4:56 ` [dpdk-dev] [PATCH 2/3] lib/librte_ether: add support for port reset Wei Zhao
2017-03-03 4:56 ` [dpdk-dev] [PATCH 3/3] net/i40e: implement device reset on port Wei Zhao
2017-03-08 11:20 ` Ferruh Yigit
2017-03-14 6:46 ` Zhao1, Wei
2017-03-24 8:31 ` Wu, Jingjing
2017-03-24 9:07 ` Wu, Jingjing
2017-03-27 6:10 ` Zhao1, Wei
2017-03-24 5:57 ` [dpdk-dev] [PATCH 0/3] net/i40e: vf port reset Wu, Jingjing
2017-03-27 6:17 ` Zhao1, Wei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A2573D2ACFCADC41BB3BE09C6DE313CA0205817C@PGSMSX103.gar.corp.intel.com \
--to=wei.zhao1@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).