[dpdk-dev] [PATCH] testpmd: call cleanup on exit

[dpdk-dev] [PATCH] testpmd: call cleanup on exit

[Stephen Hemminger]

The rte_eal_cleanup code is not exercised by testpmd which
is the most used DPDK test tool. Add a call at end of program.

This helps exercise free and close paths which can
be checked with tools like valgrind.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/testpmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index b3746822366f..a94ad19c8f5a 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3570,5 +3570,6 @@ main(int argc, char** argv)
 			return 1;
 	}
 
+	rte_eal_cleanup();
 	return 0;
 }
-- 
2.20.1

Re: [dpdk-dev] [PATCH] testpmd: call cleanup on exit

[Jerin Jacob]

On Tue, Jan 7, 2020 at 11:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The rte_eal_cleanup code is not exercised by testpmd which
> is the most used DPDK test tool. Add a call at end of program.
>
> This helps exercise free and close paths which can
> be checked with tools like valgrind.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  app/test-pmd/testpmd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index b3746822366f..a94ad19c8f5a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3570,5 +3570,6 @@ main(int argc, char** argv)
>                         return 1;
>         }
>
> +       rte_eal_cleanup();
>         return 0;

# How about changing to "return rte_eal_cleanup()" to provide OS the
correct stats?
# IMO, we need the same in other applications main() and signal handler too.




>  }
> --
> 2.20.1
>

[dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Stephen Hemminger]

The rte_eal_cleanup code is not exercised by testpmd which
is the most used DPDK test tool. Add a call at end of program.

This helps exercise free and close paths which can
be checked with tools like valgrind.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - report errors

 app/test-pmd/testpmd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index b3746822366f..2eec8afda1ec 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3570,5 +3570,10 @@ main(int argc, char** argv)
 			return 1;
 	}
 
-	return 0;
+	ret = rte_eal_cleanup();
+	if (ret != 0)
+		rte_exit(EXIT_FAILURE,
+			 "EAL cleanup failed: %s\n", strerror(-ret));
+
+	return EXIT_SUCCESS;
 }
-- 
2.20.1

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Iremonger, Bernard]

Hi Stephen,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Tuesday, January 7, 2020 7:00 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
> 
> The rte_eal_cleanup code is not exercised by testpmd which is the most
> used DPDK test tool. Add a call at end of program.
> 
> This helps exercise free and close paths which can be checked with tools like
> valgrind.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - report errors
> 
>  app/test-pmd/testpmd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> b3746822366f..2eec8afda1ec 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
>  			return 1;
>  	}
> 
> -	return 0;
> +	ret = rte_eal_cleanup();
> +	if (ret != 0)
> +		rte_exit(EXIT_FAILURE,
> +			 "EAL cleanup failed: %s\n", strerror(-ret));
> +
> +	return EXIT_SUCCESS;
>  }
> --
> 2.20.1

This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.

Regards,

Bernard.

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Stephen Hemminger]

On Wed, 8 Jan 2020 09:45:54 +0000
"Iremonger, Bernard" <bernard.iremonger@intel.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > Sent: Tuesday, January 7, 2020 7:00 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
> > 
> > The rte_eal_cleanup code is not exercised by testpmd which is the most
> > used DPDK test tool. Add a call at end of program.
> > 
> > This helps exercise free and close paths which can be checked with tools like
> > valgrind.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > v2 - report errors
> > 
> >  app/test-pmd/testpmd.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > b3746822366f..2eec8afda1ec 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
> >  			return 1;
> >  	}
> > 
> > -	return 0;
> > +	ret = rte_eal_cleanup();
> > +	if (ret != 0)
> > +		rte_exit(EXIT_FAILURE,
> > +			 "EAL cleanup failed: %s\n", strerror(-ret));
> > +
> > +	return EXIT_SUCCESS;
> >  }
> > --
> > 2.20.1  
> 
> This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.
> 
> Regards,
> 
> Bernard.
> 

Not sure if it needs to be backported. It depends on the definition
of a fix.  The original definition of stable was fixes only.
Linux has moved on to fixes and backports of missing features
like this; but DPDK has mostly stuck to the original definition
of stable.

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Ferruh Yigit]

On 1/8/2020 3:28 PM, Stephen Hemminger wrote:
> On Wed, 8 Jan 2020 09:45:54 +0000
> "Iremonger, Bernard" <bernard.iremonger@intel.com> wrote:
> 
>> Hi Stephen,
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
>>> Sent: Tuesday, January 7, 2020 7:00 PM
>>> To: dev@dpdk.org
>>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>>> Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
>>>
>>> The rte_eal_cleanup code is not exercised by testpmd which is the most
>>> used DPDK test tool. Add a call at end of program.
>>>
>>> This helps exercise free and close paths which can be checked with tools like
>>> valgrind.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> ---
>>> v2 - report errors
>>>
>>>  app/test-pmd/testpmd.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> b3746822366f..2eec8afda1ec 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
>>>  			return 1;
>>>  	}
>>>
>>> -	return 0;
>>> +	ret = rte_eal_cleanup();
>>> +	if (ret != 0)
>>> +		rte_exit(EXIT_FAILURE,
>>> +			 "EAL cleanup failed: %s\n", strerror(-ret));
>>> +
>>> +	return EXIT_SUCCESS;
>>>  }
>>> --
>>> 2.20.1  
>>
>> This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.
>>
>> Regards,
>>
>> Bernard.
>>
> 
> Not sure if it needs to be backported. It depends on the definition
> of a fix.  The original definition of stable was fixes only.
> Linux has moved on to fixes and backports of missing features
> like this; but DPDK has mostly stuck to the original definition
> of stable.
> 

I also tend to think this a fix more then a feature, and although it is a minor
fix it is good to get it to reduce the change of the conflict on other stuff in
LTS. I will add fix/stable tag while merging.

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Ferruh Yigit]

On 1/9/2020 11:16 AM, Ferruh Yigit wrote:
> On 1/8/2020 3:28 PM, Stephen Hemminger wrote:
>> On Wed, 8 Jan 2020 09:45:54 +0000
>> "Iremonger, Bernard" <bernard.iremonger@intel.com> wrote:
>>
>>> Hi Stephen,
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
>>>> Sent: Tuesday, January 7, 2020 7:00 PM
>>>> To: dev@dpdk.org
>>>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>>>> Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
>>>>
>>>> The rte_eal_cleanup code is not exercised by testpmd which is the most
>>>> used DPDK test tool. Add a call at end of program.
>>>>
>>>> This helps exercise free and close paths which can be checked with tools like
>>>> valgrind.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>> v2 - report errors
>>>>
>>>>  app/test-pmd/testpmd.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>> b3746822366f..2eec8afda1ec 100644
>>>> --- a/app/test-pmd/testpmd.c
>>>> +++ b/app/test-pmd/testpmd.c
>>>> @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
>>>>  			return 1;
>>>>  	}
>>>>
>>>> -	return 0;
>>>> +	ret = rte_eal_cleanup();
>>>> +	if (ret != 0)
>>>> +		rte_exit(EXIT_FAILURE,
>>>> +			 "EAL cleanup failed: %s\n", strerror(-ret));
>>>> +
>>>> +	return EXIT_SUCCESS;
>>>>  }
>>>> --
>>>> 2.20.1  
>>>
>>> This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.
>>>
>>> Regards,
>>>
>>> Bernard.
>>>
>>
>> Not sure if it needs to be backported. It depends on the definition
>> of a fix.  The original definition of stable was fixes only.
>> Linux has moved on to fixes and backports of missing features
>> like this; but DPDK has mostly stuck to the original definition
>> of stable.
>>
> 
> I also tend to think this a fix more then a feature, and although it is a minor
> fix it is good to get it to reduce the change of the conflict on other stuff in
> LTS. I will add fix/stable tag while merging.
> 

    Fixes: af75078fece3 ("first public release")
    Cc: stable@dpdk.org

    Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Stephen Hemminger]

On Thu, 9 Jan 2020 11:31:30 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/9/2020 11:16 AM, Ferruh Yigit wrote:
> > On 1/8/2020 3:28 PM, Stephen Hemminger wrote:  
> >> On Wed, 8 Jan 2020 09:45:54 +0000
> >> "Iremonger, Bernard" <bernard.iremonger@intel.com> wrote:
> >>  
> >>> Hi Stephen,
> >>>  
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> >>>> Sent: Tuesday, January 7, 2020 7:00 PM
> >>>> To: dev@dpdk.org
> >>>> Cc: Stephen Hemminger <stephen@networkplumber.org>
> >>>> Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
> >>>>
> >>>> The rte_eal_cleanup code is not exercised by testpmd which is the most
> >>>> used DPDK test tool. Add a call at end of program.
> >>>>
> >>>> This helps exercise free and close paths which can be checked with tools like
> >>>> valgrind.
> >>>>
> >>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>> ---
> >>>> v2 - report errors
> >>>>
> >>>>  app/test-pmd/testpmd.c | 7 ++++++-
> >>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>>> b3746822366f..2eec8afda1ec 100644
> >>>> --- a/app/test-pmd/testpmd.c
> >>>> +++ b/app/test-pmd/testpmd.c
> >>>> @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
> >>>>  			return 1;
> >>>>  	}
> >>>>
> >>>> -	return 0;
> >>>> +	ret = rte_eal_cleanup();
> >>>> +	if (ret != 0)
> >>>> +		rte_exit(EXIT_FAILURE,
> >>>> +			 "EAL cleanup failed: %s\n", strerror(-ret));
> >>>> +
> >>>> +	return EXIT_SUCCESS;
> >>>>  }
> >>>> --
> >>>> 2.20.1    
> >>>
> >>> This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.
> >>>
> >>> Regards,
> >>>
> >>> Bernard.
> >>>  
> >>
> >> Not sure if it needs to be backported. It depends on the definition
> >> of a fix.  The original definition of stable was fixes only.
> >> Linux has moved on to fixes and backports of missing features
> >> like this; but DPDK has mostly stuck to the original definition
> >> of stable.
> >>  
> > 
> > I also tend to think this a fix more then a feature, and although it is a minor
> > fix it is good to get it to reduce the change of the conflict on other stuff in
> > LTS. I will add fix/stable tag while merging.
> >   
> 
>     Fixes: af75078fece3 ("first public release")
>     Cc: stable@dpdk.org
> 
>     Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 
> Applied to dpdk-next-net/master, thanks.

Thanks,  my concern about adding it to stable is that it might expose
bugs in other places (like drivers).  That is good for current release
but not something stable users want to see.

Re: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit

[Ferruh Yigit]

On 1/9/2020 4:13 PM, Stephen Hemminger wrote:
> On Thu, 9 Jan 2020 11:31:30 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 1/9/2020 11:16 AM, Ferruh Yigit wrote:
>>> On 1/8/2020 3:28 PM, Stephen Hemminger wrote:  
>>>> On Wed, 8 Jan 2020 09:45:54 +0000
>>>> "Iremonger, Bernard" <bernard.iremonger@intel.com> wrote:
>>>>  
>>>>> Hi Stephen,
>>>>>  
>>>>>> -----Original Message-----
>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
>>>>>> Sent: Tuesday, January 7, 2020 7:00 PM
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>>>>>> Subject: [dpdk-dev] [PATCH v2] testpmd: call cleanup on exit
>>>>>>
>>>>>> The rte_eal_cleanup code is not exercised by testpmd which is the most
>>>>>> used DPDK test tool. Add a call at end of program.
>>>>>>
>>>>>> This helps exercise free and close paths which can be checked with tools like
>>>>>> valgrind.
>>>>>>
>>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>>>> ---
>>>>>> v2 - report errors
>>>>>>
>>>>>>  app/test-pmd/testpmd.c | 7 ++++++-
>>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>> b3746822366f..2eec8afda1ec 100644
>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>> @@ -3570,5 +3570,10 @@ main(int argc, char** argv)
>>>>>>  			return 1;
>>>>>>  	}
>>>>>>
>>>>>> -	return 0;
>>>>>> +	ret = rte_eal_cleanup();
>>>>>> +	if (ret != 0)
>>>>>> +		rte_exit(EXIT_FAILURE,
>>>>>> +			 "EAL cleanup failed: %s\n", strerror(-ret));
>>>>>> +
>>>>>> +	return EXIT_SUCCESS;
>>>>>>  }
>>>>>> --
>>>>>> 2.20.1    
>>>>>
>>>>> This looks like a fix to me and is probably worth backporting, it should probably have a fixes line.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Bernard.
>>>>>  
>>>>
>>>> Not sure if it needs to be backported. It depends on the definition
>>>> of a fix.  The original definition of stable was fixes only.
>>>> Linux has moved on to fixes and backports of missing features
>>>> like this; but DPDK has mostly stuck to the original definition
>>>> of stable.
>>>>  
>>>
>>> I also tend to think this a fix more then a feature, and although it is a minor
>>> fix it is good to get it to reduce the change of the conflict on other stuff in
>>> LTS. I will add fix/stable tag while merging.
>>>   
>>
>>     Fixes: af75078fece3 ("first public release")
>>     Cc: stable@dpdk.org
>>
>>     Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>
>> Applied to dpdk-next-net/master, thanks.
> 
> Thanks,  my concern about adding it to stable is that it might expose
> bugs in other places (like drivers).  That is good for current release
> but not something stable users want to see.
> 

Overall I agree, but the 'rte_eal_cleanup()' is pretty simple as of now, only
doing a few simple cleanups not full cleanup, that is why I believe it can be OK
to add it.