diff mbox series

[v5,2/6] app/testpmd: allow detaching a port not closed

Message ID 20181018012402.1240-3-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v5,1/6] app/testpmd: fix ports list after removing several at once |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Oct. 18, 2018, 1:23 a.m. UTC 
  The testpmd application aim is for testing;
so order of operations should not be enforced.

There was a test to forbid detaching before closing a port.
However, it may interesting to test what happens in such case.
It is possible for a PMD to automatically close the port when detaching.

in order to avoid a crash, it is checked that the port must be stopped
before detaching (as for closing).

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Andrew Rybchenko Oct. 18, 2018, 7:45 a.m. UTC | #1 
On 10/18/18 4:23 AM, Thomas Monjalon wrote:
> The testpmd application aim is for testing;
> so order of operations should not be enforced.
>
> There was a test to forbid detaching before closing a port.
> However, it may interesting to test what happens in such case.
> It is possible for a PMD to automatically close the port when detaching.

Yes. In the case of net/sfc it requires a patch to call sfc_dev_close()
from uninit. I think network PMD maintainers should be notified
to double-check drivers.

> in order to avoid a crash, it is checked that the port must be stopped
> before detaching (as for closing).

I thought that it is sufficient to stop traffic and the port may be stopped
automatically by PMD. Not sure about it, just would like to clarify my
previous notes.

> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

[...]
Iremonger, Bernard Oct. 18, 2018, 10:51 a.m. UTC | #2 
Hi Thomas,

> -----Original Message-----
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> Sent: Thursday, October 18, 2018 8:45 AM
> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>; rahul.lakkireddy@chelsio.com
> Subject: Re: [PATCH v5 2/6] app/testpmd: allow detaching a port not closed
> 
> On 10/18/18 4:23 AM, Thomas Monjalon wrote:
> > The testpmd application aim is for testing; so order of operations
> > should not be enforced.
> >
> > There was a test to forbid detaching before closing a port.
> > However, it may interesting to test what happens in such case.
> > It is possible for a PMD to automatically close the port when detaching.
> 
> Yes. In the case of net/sfc it requires a patch to call sfc_dev_close() from uninit.
> I think network PMD maintainers should be notified to double-check drivers.
> 
> > in order to avoid a crash, it is checked that the port must be stopped
> > before detaching (as for closing).
> 
> I thought that it is sufficient to stop traffic and the port may be stopped
> automatically by PMD. Not sure about it, just would like to clarify my previous
> notes.
> 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> [...]

This patch seems too  risky as it may impact some of the PMD's.

Regards,

Bernard.
Thomas Monjalon Oct. 18, 2018, 11:24 a.m. UTC | #3 
18/10/2018 12:51, Iremonger, Bernard:
> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
> > On 10/18/18 4:23 AM, Thomas Monjalon wrote:
> > > The testpmd application aim is for testing; so order of operations
> > > should not be enforced.
> > >
> > > There was a test to forbid detaching before closing a port.
> > > However, it may interesting to test what happens in such case.
> > > It is possible for a PMD to automatically close the port when detaching.
> > 
> > Yes. In the case of net/sfc it requires a patch to call sfc_dev_close() from uninit.
> > I think network PMD maintainers should be notified to double-check drivers.
> > 
> > > in order to avoid a crash, it is checked that the port must be stopped
> > > before detaching (as for closing).
> > 
> > I thought that it is sufficient to stop traffic and the port may be stopped
> > automatically by PMD. Not sure about it, just would like to clarify my previous
> > notes.
> > 
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > 
> > [...]
> 
> This patch seems too  risky as it may impact some of the PMD's.

Yes, it will not work for all PMDs.
If we want to allow this scenario, we'll need to improve some PMDs.
This patch is just allowing to test the scenario.
It will help PMD developers, so I think it is more helpful than risky.
diff mbox series

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c4109417a..31aadec63 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2343,13 +2343,15 @@  detach_port(portid_t port_id)
 	printf("Detaching a port...\n");
 
 	if (!port_is_closed(port_id)) {
-		printf("Please close port first\n");
-		return;
+		if (ports[port_id].port_status != RTE_PORT_STOPPED) {
+			printf("Port not stopped\n");
+			return;
+		}
+		printf("Port was not closed\n");
+		if (ports[port_id].flow_list)
+			port_flow_flush(port_id);
 	}
 
-	if (ports[port_id].flow_list)
-		port_flow_flush(port_id);
-
 	if (rte_eth_dev_detach(port_id, name)) {
 		TESTPMD_LOG(ERR, "Failed to detach port %u\n", port_id);
 		return;