[dpdk-dev] [PATCH] app/test-pmd: stop all ports before any close

[dpdk-dev] [PATCH] app/test-pmd: stop all ports before any close

[Cristian Dumitrescu]

This patch proposes a slightly different test-pmd quit operation: stop
all devices before starting to close any device. Basically, stop all
moving parts before beginning to remove them. The current test-pmd quit
is stoping and closing each device before moving to the next device.

If all devices in the system are independent of each other, this
difference is usually not important. In case of Soft NIC devices, any
such virtual device typically depends on one or more physical devices
being alive, as it accesses their queues, so this difference becomes
important.

Without this straightforward fix, all the Soft NIC devices need to be
manually stopped before the quit command is issued, otherwise the quit
command can sometimes crash the test-pmd application.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 8d584b008..15a948828 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2391,9 +2391,13 @@ pmd_test_exit(void)
 	if (ports != NULL) {
 		no_link_check = 1;
 		RTE_ETH_FOREACH_DEV(pt_id) {
-			printf("\nShutting down port %d...\n", pt_id);
+			printf("\nStopping port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
+		}
+		RTE_ETH_FOREACH_DEV(pt_id) {
+			printf("\nShutting down port %d...\n", pt_id);
+			fflush(stdout);
 			close_port(pt_id);
 
 			/*
-- 
2.17.1

Re: [dpdk-dev] [PATCH] app/test-pmd: stop all ports before any close

[Iremonger, Bernard]

Hi Cristian,

> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Thursday, January 3, 2019 4:37 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>
> Subject: [PATCH] app/test-pmd: stop all ports before any close

./devtools/check-git-log.sh -1
Wrong headline label:
        app/test-pmd: stop all ports before any close

Should be app/testpmd

The commit message should also have the "fix" keyword

> 
> This patch proposes a slightly different test-pmd quit operation: stop all devices
> before starting to close any device. Basically, stop all moving parts before
> beginning to remove them. The current test-pmd quit is stoping and closing each
> device before moving to the next device.
> 
> If all devices in the system are independent of each other, this difference is
> usually not important. In case of Soft NIC devices, any such virtual device
> typically depends on one or more physical devices being alive, as it accesses
> their queues, so this difference becomes important.
> 
> Without this straightforward fix, all the Soft NIC devices need to be manually
> stopped before the quit command is issued, otherwise the quit command can
> sometimes crash the test-pmd application.

There should be a fixes line before the sign-off, and maybe a " Cc: stable@dpdk.org" line
 
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> ---
>  app/test-pmd/testpmd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 8d584b008..15a948828 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2391,9 +2391,13 @@ pmd_test_exit(void)
>  	if (ports != NULL) {
>  		no_link_check = 1;
>  		RTE_ETH_FOREACH_DEV(pt_id) {
> -			printf("\nShutting down port %d...\n", pt_id);
> +			printf("\nStopping port %d...\n", pt_id);
>  			fflush(stdout);
>  			stop_port(pt_id);
> +		}
> +		RTE_ETH_FOREACH_DEV(pt_id) {
> +			printf("\nShutting down port %d...\n", pt_id);
> +			fflush(stdout);
>  			close_port(pt_id);
> 
>  			/*
> --
> 2.17.1

Regards,

Bernard.

Re: [dpdk-dev] [PATCH] app/test-pmd: stop all ports before any close

[Dumitrescu, Cristian]

Hi Bernard,

Thanks for reviewing this patch.

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, January 4, 2019 10:16 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: RE: [PATCH] app/test-pmd: stop all ports before any close
> 
> Hi Cristian,
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian
> > Sent: Thursday, January 3, 2019 4:37 PM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> > Subject: [PATCH] app/test-pmd: stop all ports before any close
> 
> ./devtools/check-git-log.sh -1
> Wrong headline label:
>         app/test-pmd: stop all ports before any close
> 
> Should be app/testpmd

Will fix.

> 
> The commit message should also have the "fix" keyword
> 

We can treat this as a fix or as an improvement, I was not sure about what people think. I don't mind either way. Will treat this as a fix then in v2.

> >
> > This patch proposes a slightly different test-pmd quit operation: stop all
> devices
> > before starting to close any device. Basically, stop all moving parts before
> > beginning to remove them. The current test-pmd quit is stoping and closing
> each
> > device before moving to the next device.
> >
> > If all devices in the system are independent of each other, this difference is
> > usually not important. In case of Soft NIC devices, any such virtual device
> > typically depends on one or more physical devices being alive, as it accesses
> > their queues, so this difference becomes important.
> >
> > Without this straightforward fix, all the Soft NIC devices need to be
> manually
> > stopped before the quit command is issued, otherwise the quit command
> can
> > sometimes crash the test-pmd application.
> 
> There should be a fixes line before the sign-off, and maybe a " Cc:
> stable@dpdk.org" line
> 
> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 8d584b008..15a948828 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2391,9 +2391,13 @@ pmd_test_exit(void)
> >  	if (ports != NULL) {
> >  		no_link_check = 1;
> >  		RTE_ETH_FOREACH_DEV(pt_id) {
> > -			printf("\nShutting down port %d...\n", pt_id);
> > +			printf("\nStopping port %d...\n", pt_id);
> >  			fflush(stdout);
> >  			stop_port(pt_id);
> > +		}
> > +		RTE_ETH_FOREACH_DEV(pt_id) {
> > +			printf("\nShutting down port %d...\n", pt_id);
> > +			fflush(stdout);
> >  			close_port(pt_id);
> >
> >  			/*
> > --
> > 2.17.1
> 
> Regards,
> 
> Bernard.

Regards,
Cristian