[v3,1/4] app/testpmd: allow detaching a port not closed

  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.

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

+Cc Bernard

Note that the function port_is_closed is replaced because
a closed port is always seen as invalid after this series:
the state is set to RTE_ETH_DEV_UNUSED by rte_eth_dev_release_port().
I may split this patch and add a release note to make it clear.


17/10/2018 03:54, Thomas Monjalon:
> 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.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test-pmd/testpmd.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 5dbbf783f..f5dee1d71 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1938,18 +1938,6 @@ port_is_started(portid_t port_id)
>  	return 1;
>  }
>  
> -static int
> -port_is_closed(portid_t port_id)
> -{
> -	if (port_id_is_invalid(port_id, ENABLED_WARN))
> -		return 0;
> -
> -	if (ports[port_id].port_status != RTE_PORT_CLOSED)
> -		return 0;
> -
> -	return 1;
> -}
> -
>  int
>  start_port(portid_t pid)
>  {
> @@ -2319,14 +2307,12 @@ 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_CLOSED) {
> +		printf("Port 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;
>
  

On 10/17/18 4:54 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.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

I'm afraid it could be a problem which the patch, since port
close ensures that the port is not used for traffic forwarding.
Right now the check is gone and we can detach port which
is used for traffic forwarding on separate data cores.
So, almost guaranteed crash.
  

17/10/2018 08:26, Andrew Rybchenko:
> On 10/17/18 4:54 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.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> I'm afraid it could be a problem which the patch, since port
> close ensures that the port is not used for traffic forwarding.
> Right now the check is gone and we can detach port which
> is used for traffic forwarding on separate data cores.
> So, almost guaranteed crash.

Yes I can duplicate this check in detach_port().
  

Hi Thomas,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, October 17, 2018 9:21 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org;
> ophirmu@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/4] app/testpmd: allow detaching a port not
> closed
> 
> 17/10/2018 08:26, Andrew Rybchenko:
> > On 10/17/18 4:54 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.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >
> > I'm afraid it could be a problem which the patch, since port close
> > ensures that the port is not used for traffic forwarding.
> > Right now the check is gone and we can detach port which is used for
> > traffic forwarding on separate data cores.
> > So, almost guaranteed crash.
> 
> Yes I can duplicate this check in detach_port().

I agree with Andrew that this will cause a crash.

I don't understand why the sequence is changing here.
The close(), detach()  sequence has been in place since the port hot plug work some years ago, user applications may already be using this sequence.

Regards,

Bernard.
  

17/10/2018 12:30, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon
> > 17/10/2018 08:26, Andrew Rybchenko:
> > > On 10/17/18 4:54 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.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > I'm afraid it could be a problem which the patch, since port close
> > > ensures that the port is not used for traffic forwarding.
> > > Right now the check is gone and we can detach port which is used for
> > > traffic forwarding on separate data cores.
> > > So, almost guaranteed crash.
> > 
> > Yes I can duplicate this check in detach_port().
> 
> I agree with Andrew that this will cause a crash.

As I answered, I will add a check that port is stopped.

> I don't understand why the sequence is changing here.
> The close(), detach()  sequence has been in place since the port hot plug work some years ago, user applications may already be using this sequence.

I explained the reason in the commit log:
Adding too much checks is wrong for a test application.
We must be free to call detach without close.