[v6] app/pdump: add pudmp exits with primary support

  When primary app exits, the residual running pdump will stop the
primary app to restart. Add pdump exits with primary support.

Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---

Change in V6:
  - remove "Suggested-by" tags and head line '.' in git log.
  - adjust the rte_alarm.h head file position.
  - add comment for MONITOR_INTERVAL.
  - remove redunt fail in log.
  - treat rte_eal_alarm_set fail as warning only.
  - adjust the multiple line comments coding style.

---
 app/pdump/main.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)
  

Hi Suanming,

snipped
> 
>  /* true if x is a power of 2 */
>  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct

Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?

> parse_val {  }
> 
>  static void
> +monitor_primary(void *arg __rte_unused) {
> +	if (quit_signal)
> +		return;
> +
> +	if (rte_eal_primary_proc_alive(NULL))
> +		rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,
> NULL);
> +	else
> +		quit_signal = 1;
> +}

This is suggestion, why not omit else part with 

`
if (rte_eal_primary_proc_alive(NULL)) {
	rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
	return;
}
`

Snipped

As suggested in v4 can you update the `pdump.rst` on the new behaviour?

Thanks
Vipin Varghese
  

On 2019/5/2 16:04, Varghese, Vipin wrote:
> Hi Suanming,
>
> snipped
>>   /* true if x is a power of 2 */
>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct
> Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?

I'm sorry, but that line is not add by this patch this time.

Maybe another commit is more suitable to fix the previous code.

>
>> parse_val {  }
>>
>>   static void
>> +monitor_primary(void *arg __rte_unused) {
>> +	if (quit_signal)
>> +		return;
>> +
>> +	if (rte_eal_primary_proc_alive(NULL))
>> +		rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,
>> NULL);
>> +	else
>> +		quit_signal = 1;
>> +}
> This is suggestion, why not omit else part with
>
> `
> if (rte_eal_primary_proc_alive(NULL)) {
> 	rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
> 	return;
> }
> `
Thanks for the suggestion. It's OK for me. If there's one more vote, I 
will do it.
>
> Snipped
>
> As suggested in v4 can you update the `pdump.rst` on the new behaviour?

As noted in the cover letter in v5. The `exit with primary` 
configuration should be made as default or not is still not confirmed 
from the maintainer.

Since `exit with primary` is now removed in the patch and made as 
default per Anatoly's suggestion and not get more information from the 
maintainer, the update of the doc is also got hung up.

>
> Thanks
> Vipin Varghese
>
>
  

On 02-May-19 9:32 AM, Suanming.Mou wrote:
> 
> On 2019/5/2 16:04, Varghese, Vipin wrote:
>> Hi Suanming,
>>
>> snipped
>>>   /* true if x is a power of 2 */
>>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct
>> Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
> 
> I'm sorry, but that line is not add by this patch this time.
> 
> Maybe another commit is more suitable to fix the previous code.

Yes, if there are issues with the code that aren't directly related to 
the patch and aren't touched by it, they should be addressed as a 
separate patch.

> 
>>
>>> parse_val {  }
>>>
>>>   static void
>>> +monitor_primary(void *arg __rte_unused) {
>>> +    if (quit_signal)
>>> +        return;
>>> +
>>> +    if (rte_eal_primary_proc_alive(NULL))
>>> +        rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,
>>> NULL);
>>> +    else
>>> +        quit_signal = 1;
>>> +}
>> This is suggestion, why not omit else part with
>>
>> `
>> if (rte_eal_primary_proc_alive(NULL)) {
>>     rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
>>     return;
>> }
>> `
> Thanks for the suggestion. It's OK for me. If there's one more vote, I 
> will do it.

No preference. Either way works, so i'd keep it as is.
  

HI Suanming,

snipped
> >>   /* true if x is a power of 2 */
> >>   #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@
> >> struct
> > Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
> 
> I'm sorry, but that line is not add by this patch this time.
> 
> Maybe another commit is more suitable to fix the previous code.
Ok

Snipped
> >
> > As suggested in v4 can you update the `pdump.rst` on the new behaviour?
> 
> As noted in the cover letter in v5. The `exit with primary` configuration should be
> made as default or not is still not confirmed from the maintainer.
> 
> Since `exit with primary` is now removed in the patch and made as default per
> Anatoly's suggestion and not get more information from the maintainer, the
> update of the doc is also got hung up.

I am not clear with this, if there is change in behaviour of the application it has to be documented.

Thanks
Vipin Varghese
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou


<snip>

>  static void
> +monitor_primary(void *arg __rte_unused) {
> +	if (quit_signal)
> +		return;
> +
> +	if (rte_eal_primary_proc_alive(NULL))
> +		rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,
> NULL);
> +	else
> +		quit_signal = 1;
> +}
> +

Adding the log message saying primary existing so, secondary.. would be helpful here.
I am ok to have it as default behaviour.  
As Vipin mentioned, can you also update the document doc/guides/tools/pdump.rst

Thanks,
Reshma
  

Hi,

Thanks for the advises from you all.

The latest v7 patch has sent out.

Br,

Mou

On 2019/5/2 17:54, Pattan, Reshma wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
>
> <snip>
>
>>   static void
>> +monitor_primary(void *arg __rte_unused) {
>> +	if (quit_signal)
>> +		return;
>> +
>> +	if (rte_eal_primary_proc_alive(NULL))
>> +		rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,
>> NULL);
>> +	else
>> +		quit_signal = 1;
>> +}
>> +
> Adding the log message saying primary existing so, secondary.. would be helpful here.
> I am ok to have it as default behaviour.
> As Vipin mentioned, can you also update the document doc/guides/tools/pdump.rst
>
> Thanks,
> Reshma
>
>