[dpdk-dev,1/3] app/testpmd: add port reset command into testpmd
Checks
Commit Message
Add vf port reset command into testpmd project, it is the interface for
user to reset vf port.
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(-)
Comments
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.
>
> 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.
<...>
> @@ -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.
<...>
> @@ -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?
> 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?
> + 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?
<...>
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.
>
> <...>
Hi, Ferruh
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, March 14, 2017 3:58 PM
> To: Yigit, Ferruh <ferruh.yigit@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
>
> 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
I should apologize about this. This rte layer of reset command is used for both vf and pf port reset.
This feature is related to vf, but pf port reset cmd line can also use it. There is mistake when I explain to you last time.
>
> >
> > <...>
> >
> > > @@ -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.
>
> >
> > <...>
@@ -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"
+
"port attach (ident)\n"
" Attach physical or virtual dev by pci address or virtual device name\n\n"
@@ -906,6 +909,8 @@ static void cmd_operate_port_parsed(void *parsed_result,
stop_port(RTE_PORT_ALL);
else if (!strcmp(res->name, "close"))
close_port(RTE_PORT_ALL);
+ else if (!strcmp(res->name, "reset"))
+ reset_port(RTE_PORT_ALL);
else
printf("Unknown parameter\n");
}
@@ -915,14 +920,15 @@ cmdline_parse_token_string_t cmd_operate_port_all_cmd =
"port");
cmdline_parse_token_string_t cmd_operate_port_all_port =
TOKEN_STRING_INITIALIZER(struct cmd_operate_port_result, name,
- "start#stop#close");
+ "start#stop#close#reset");
cmdline_parse_token_string_t cmd_operate_port_all_all =
TOKEN_STRING_INITIALIZER(struct cmd_operate_port_result, value, "all");
cmdline_parse_inst_t cmd_operate_port = {
.f = cmd_operate_port_parsed,
.data = NULL,
- .help_str = "port start|stop|close all: Start/Stop/Close all ports",
+ .help_str = "port start|stop|close|reset all: Start/Stop/Close/"
+ "Reset all ports",
.tokens = {
(void *)&cmd_operate_port_all_cmd,
(void *)&cmd_operate_port_all_port,
@@ -950,6 +956,8 @@ static void cmd_operate_specific_port_parsed(void *parsed_result,
stop_port(res->value);
else if (!strcmp(res->name, "close"))
close_port(res->value);
+ else if (!strcmp(res->name, "reset"))
+ reset_port(res->value);
else
printf("Unknown parameter\n");
}
@@ -959,7 +967,7 @@ cmdline_parse_token_string_t cmd_operate_specific_port_cmd =
keyword, "port");
cmdline_parse_token_string_t cmd_operate_specific_port_port =
TOKEN_STRING_INITIALIZER(struct cmd_operate_specific_port_result,
- name, "start#stop#close");
+ name, "start#stop#close#reset");
cmdline_parse_token_num_t cmd_operate_specific_port_id =
TOKEN_NUM_INITIALIZER(struct cmd_operate_specific_port_result,
value, UINT8);
@@ -967,7 +975,8 @@ cmdline_parse_token_num_t cmd_operate_specific_port_id =
cmdline_parse_inst_t cmd_operate_specific_port = {
.f = cmd_operate_specific_port_parsed,
.data = NULL,
- .help_str = "port start|stop|close <port_id>: Start/Stop/Close port_id",
+ .help_str = "port start|stop|close|reset <port_id>: Start/Stop/Close/"
+ "Reset port_id",
.tokens = {
(void *)&cmd_operate_specific_port_cmd,
(void *)&cmd_operate_specific_port_port,
@@ -137,6 +137,7 @@ portid_t nb_fwd_ports; /**< Number of forwarding ports. */
unsigned int fwd_lcores_cpuids[RTE_MAX_LCORE]; /**< CPU ids configuration. */
portid_t fwd_ports_ids[RTE_MAX_ETHPORTS]; /**< Port ids configuration. */
+volatile char reset_ports[RTE_MAX_ETHPORTS] = {0}; /**< Portr reset falg. */
struct fwd_stream **fwd_streams; /**< For each RX queue of each port. */
streamid_t nb_fwd_streams; /**< Is equal to (nb_ports * nb_rxq). */
@@ -601,6 +602,7 @@ init_config(void)
if (init_fwd_streams() < 0)
rte_exit(EXIT_FAILURE, "FAIL from init_fwd_streams()\n");
+
fwd_config_setup();
}
@@ -1305,6 +1307,17 @@ port_is_closed(portid_t port_id)
return 1;
}
+static void
+reset_event_callback(uint8_t port_id, enum rte_eth_event_type type, void *param)
+{
+ RTE_SET_USED(param);
+
+ printf("Event type: %s on port %d\n",
+ type == RTE_ETH_EVENT_INTR_RESET ? "RESET interrupt" :
+ "unknown event", port_id);
+ reset_ports[port_id] = 1;
+}
+
int
start_port(portid_t pid)
{
@@ -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);
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) {
+ 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;
+ }
+
+ port = &ports[pi];
+ if (rte_atomic16_cmpset(&(port->port_status),
+ RTE_PORT_STARTED, RTE_PORT_HANDLING) == 1) {
+ printf("Port %d is not started\n", pi);
+ continue;
+ }
+
+ reset_ports[pi] = 0;
+
+ if (rte_atomic16_cmpset(&(port->port_status),
+ RTE_PORT_HANDLING, RTE_PORT_STARTED) == 0)
+ printf("Port %d cannot be set to started\n", pi);
+ }
+
+ printf("Done\n");
+}
+
+void
attach_port(char *identifier)
{
portid_t pi = 0;
@@ -586,6 +586,7 @@ int init_port_dcb_config(portid_t pid, enum dcb_mode_enable dcb_mode,
int start_port(portid_t pid);
void stop_port(portid_t pid);
void close_port(portid_t pid);
+void reset_port(portid_t pid);
void attach_port(char *identifier);
void detach_port(uint8_t port_id);
int all_ports_stopped(void);