[dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Suanming.Mou]

If primary app exited, meaningless for pdump keeps running anymore.

Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 app/pdump/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d208548fa13..f1ff2a3ceb4f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -847,6 +847,10 @@ struct parse_val {
 		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
 	if (pt->dir & RTE_PDUMP_FLAG_TX)
 		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
+
+	/* Once primary exits, so will I. */
+	if (!rte_eal_primary_proc_alive(NULL))
+		quit_signal = 1;
 }
 
 static int
-- 
1.7.12.4

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Varghese, Vipin]

Hi,

snipped
> @@ -847,6 +847,10 @@ struct parse_val {
>  		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
>  	if (pt->dir & RTE_PDUMP_FLAG_TX)
>  		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
> +
> +	/* Once primary exits, so will I. */
> +	if (!rte_eal_primary_proc_alive(NULL))
> +		quit_signal = 1;
>  }

As per the current suggested code flow check is added to while loop in function `dump_packets'. 

Questions:
1. What is impact in performance with and without patch?
2. For various packet sizes and port speed what are delta in drops for packet capture?

Note: If pdump application is still alive when primary is not running, primary cannot be started. Is this a cue that pdump is still alive and has to be terminated?

> 
>  static int
> --
> 1.7.12.4

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Suanming.Mou]

On 2019/4/25 23:51, Varghese, Vipin wrote:
> Hi,
>
> snipped
>> @@ -847,6 +847,10 @@ struct parse_val {
>>   		pdump_rxtx(pt->rx_ring, pt->rx_vdev_id, &pt->stats);
>>   	if (pt->dir & RTE_PDUMP_FLAG_TX)
>>   		pdump_rxtx(pt->tx_ring, pt->tx_vdev_id, &pt->stats);
>> +
>> +	/* Once primary exits, so will I. */
>> +	if (!rte_eal_primary_proc_alive(NULL))
>> +		quit_signal = 1;
>>   }
> As per the current suggested code flow check is added to while loop in function `dump_packets'.

Thanks for the reply. Since want to make it clean, the code was here.
However, it seems need to take care of the performance impact first.

> Questions:
> 1. What is impact in performance with and without patch?

A1. Do a little trick as the patch below to tested the impact in the single core mode on Intel(R) Xeon(R) CPU E5-2620 v4 @ 2.10GHz with no pkts.

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d208548fa13..804011b187c4 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -141,7 +141,7 @@ struct parse_val {
  
  static int num_tuples;
  static struct rte_eth_conf port_conf_default;
-static volatile uint8_t quit_signal;
+static volatile uint32_t quit_signal;
  static uint8_t multiple_core_capture;
  
  /**< display usage */
@@ -868,6 +868,7 @@ struct parse_val {
  dump_packets(void)
  {
  	int i;
+	uint64_t start, end;
  	uint32_t lcore_id = 0;
  
  	if (!multiple_core_capture) {
@@ -880,10 +881,20 @@ struct parse_val {
  				pdump_t[i].device_id,
  				pdump_t[i].queue);
  
-		while (!quit_signal) {
+		/* make it hot */
+		rte_eal_primary_proc_alive(NULL);
+		rte_eal_primary_proc_alive(NULL)
+
+		start = rte_rdtsc();
+		while (quit_signal < 50000) {
+			/* Just testing with and w/o the 'if' line below */
+			if (rte_eal_primary_proc_alive(NULL))
+				quit_signal++;
  			for (i = 0; i < num_tuples; i++)
  				pdump_packets(&pdump_t[i]);
  		}
+		end = rte_rdtsc();
+		printf("Totally count:%u, cost tsc:%lu\n", quit_signal, end - start);
  
  		return;
  	}

The total tsc cost is about 338809671 with rte_eal_primary_proc_alive().
And the tsc cost is just about 513573 without rte_eal_primary_proc_alive().
The dpdk-pdump had also used taskset to bind to specify isolate core.
So it seems the patch do a great performance impact.
Maybe another async method should be introduced to monitor the primary status.

> 2. For various packet sizes and port speed what are delta in drops for packet capture?

A2. Refer to A1, it's not needed anymore.

>
> Note: If pdump application is still alive when primary is not running, primary cannot be started. Is this a cue that pdump is still alive and has to be terminated?

Yes, some guys complained that the residual dpdk-pdump impact the restart of the primary app and refuse to add other mechanisms e.g. to kill the dpdk-pdump in the app to avoid that case.

So the patch was created.
Is there any other ways to avoid that.

>>   static int
>> --
>> 1.7.12.4
>
>

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Varghese, Vipin]

Hi,

Looks like something in email format setting is affecting the style. Please find my replies below
snipped



As per the current suggested code flow check is added to while loop in function `dump_packets'.

Thanks for the reply. Since want to make it clean, the code was here.

However, it seems need to take care of the performance impact first.

Response> thanks for acknowledging the same.



Questions:

1. What is impact in performance with and without patch?

A1. Do a little trick as the patch below to tested the impact in the single core mode on Intel(R) Xeon(R) CPU E5-2620 v4 @ 2.10GHz with no pkts.



diff --git a/app/pdump/main.c b/app/pdump/main.c

index 3d208548fa13..804011b187c4 100644

--- a/app/pdump/main.c

+++ b/app/pdump/main.c

@@ -141,7 +141,7 @@ struct parse_val {



 static int num_tuples;

 static struct rte_eth_conf port_conf_default;

-static volatile uint8_t quit_signal;

+static volatile uint32_t quit_signal;

 static uint8_t multiple_core_capture;



 /**< display usage */

@@ -868,6 +868,7 @@ struct parse_val {

 dump_packets(void)

 {

       int i;

+      uint64_t start, end;

       uint32_t lcore_id = 0;



       if (!multiple_core_capture) {

@@ -880,10 +881,20 @@ struct parse_val {

                              pdump_t[i].device_id,

                              pdump_t[i].queue);



-              while (!quit_signal) {

+              /* make it hot */

+              rte_eal_primary_proc_alive(NULL);

+              rte_eal_primary_proc_alive(NULL)

+

+              start = rte_rdtsc();

+              while (quit_signal < 50000) {

+                     /* Just testing with and w/o the 'if' line below */

+                     if (rte_eal_primary_proc_alive(NULL))

+                             quit_signal++;

                      for (i = 0; i < num_tuples; i++)

                              pdump_packets(&pdump_t[i]);

               }

+              end = rte_rdtsc();

+              printf("Totally count:%u, cost tsc:%lu\n", quit_signal, end - start);



               return;

       }



The total tsc cost is about 338809671 with rte_eal_primary_proc_alive().

And the tsc cost is just about 513573 without rte_eal_primary_proc_alive().

The dpdk-pdump had also used taskset to bind to specify isolate core.

So it seems the patch do a great performance impact.

Response> thanks for confirming the suspicion.



Maybe another async method should be introduced to monitor the primary status.

Response> yes, without affecting the capture thread.



2. For various packet sizes and port speed what are delta in drops for packet capture?

A2. Refer to A1, it's not needed anymore.

Response> A1 there is performance impact.



Note: If pdump application is still alive when primary is not running, primary cannot be started. Is this a cue that pdump is still alive and has to be terminated?

Yes, some guys complained that the residual dpdk-pdump impact the restart of the primary app and refuse to add other mechanisms e.g. to kill the dpdk-pdump in the app to avoid that case.

So the patch was created.

Is there any other ways to avoid that.

Response> in my humble opinion, best way around is add user option like ‘--exit’; which then will add periodic rte_timer for user desired seconds ‘0.1, 0.5, 1.0, 5.0’. The timer callback can run on master core which sets ‘quit_signal’ once primary is no longer alive. In case of ‘multi thread’ capture master thread is not involved in dump_packets thus avoiding any packet drops or performance issue..

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Varghese, Vipin]

I will leave this suggestion open for comments from the maintainer.

snipped

Hi,

Looks like something in email format setting is affecting the style. Please find my replies below
snipped



As per the current suggested code flow check is added to while loop in function `dump_packets'.

Thanks for the reply. Since want to make it clean, the code was here.

However, it seems need to take care of the performance impact first.

Response> thanks for acknowledging the same.



Questions:

1. What is impact in performance with and without patch?

A1. Do a little trick as the patch below to tested the impact in the single core mode on Intel(R) Xeon(R) CPU E5-2620 v4 @ 2.10GHz with no pkts.



diff --git a/app/pdump/main.c b/app/pdump/main.c

index 3d208548fa13..804011b187c4 100644

--- a/app/pdump/main.c

+++ b/app/pdump/main.c

@@ -141,7 +141,7 @@ struct parse_val {



 static int num_tuples;

 static struct rte_eth_conf port_conf_default;

-static volatile uint8_t quit_signal;

+static volatile uint32_t quit_signal;

 static uint8_t multiple_core_capture;



 /**< display usage */

@@ -868,6 +868,7 @@ struct parse_val {

 dump_packets(void)

 {

       int i;

+      uint64_t start, end;

       uint32_t lcore_id = 0;



       if (!multiple_core_capture) {

@@ -880,10 +881,20 @@ struct parse_val {

                              pdump_t[i].device_id,

                              pdump_t[i].queue);



-              while (!quit_signal) {

+              /* make it hot */

+              rte_eal_primary_proc_alive(NULL);

+              rte_eal_primary_proc_alive(NULL)

+

+              start = rte_rdtsc();

+              while (quit_signal < 50000) {

+                     /* Just testing with and w/o the 'if' line below */

+                     if (rte_eal_primary_proc_alive(NULL))

+                             quit_signal++;

                      for (i = 0; i < num_tuples; i++)

                              pdump_packets(&pdump_t[i]);

               }

+              end = rte_rdtsc();

+              printf("Totally count:%u, cost tsc:%lu\n", quit_signal, end - start);



               return;

       }



The total tsc cost is about 338809671 with rte_eal_primary_proc_alive().

And the tsc cost is just about 513573 without rte_eal_primary_proc_alive().

The dpdk-pdump had also used taskset to bind to specify isolate core.

So it seems the patch do a great performance impact.

Response> thanks for confirming the suspicion.



Maybe another async method should be introduced to monitor the primary status.

Response> yes, without affecting the capture thread.



2. For various packet sizes and port speed what are delta in drops for packet capture?

A2. Refer to A1, it's not needed anymore.

Response> A1 there is performance impact.



Note: If pdump application is still alive when primary is not running, primary cannot be started. Is this a cue that pdump is still alive and has to be terminated?

Yes, some guys complained that the residual dpdk-pdump impact the restart of the primary app and refuse to add other mechanisms e.g. to kill the dpdk-pdump in the app to avoid that case.

So the patch was created.

Is there any other ways to avoid that.

Response> in my humble opinion, best way around is add user option like ‘--exit’; which then will add periodic rte_timer for user desired seconds ‘0.1, 0.5, 1.0, 5.0’. The timer callback can run on master core which sets ‘quit_signal’ once primary is no longer alive. In case of ‘multi thread’ capture master thread is not involved in dump_packets thus avoiding any packet drops or performance issue..

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Suanming.Mou]

On 2019/4/26 18:56, Varghese, Vipin wrote:
>
> I will leave this suggestion open for comments from the maintainer.
>
Hi,

Thanks for your suggestion. I have also tried to add an slave core to 
monitor the primary status this afternoon.  It works.

I doubt if it can be add an new option as you suggested, but which will 
also require people who complain the exiting to add an extra slave core 
for that.

Please waiting for the new patch in one or two days.

> snipped
>
> Hi,
>
> Looks like something in email format setting is affecting the style. 
> Please find my replies below
>
> snipped
>
>     As per the current suggested code flow check is added to while loop in function `dump_packets'.
>
> Thanks for the reply. Since want to make it clean, the code was here.
> However, it seems need to take care of the performance impact first.
> Response> thanks for acknowledging the same.
>
>     Questions:
>
>     1. What is impact in performance with and without patch?
>
> A1. Do a little trick as the patch below to tested the impact in the single core mode on Intel(R) Xeon(R) CPU E5-2620 v4 @ 2.10GHz with no pkts.
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 3d208548fa13..804011b187c4 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -141,7 +141,7 @@ struct parse_val {
>   
>   static int num_tuples;
>   static struct rte_eth_conf port_conf_default;
> -static volatile uint8_t quit_signal;
> +static volatile uint32_t quit_signal;
>   static uint8_t multiple_core_capture;
>   
>   /**< display usage */
> @@ -868,6 +868,7 @@ struct parse_val {
>   dump_packets(void)
>   {
>         int i;
> +      uint64_t start, end;
>         uint32_t lcore_id = 0;
>   
>         if (!multiple_core_capture) {
> @@ -880,10 +881,20 @@ struct parse_val {
>                                pdump_t[i].device_id,
>                                pdump_t[i].queue);
>   
> -              while (!quit_signal) {
> +              /* make it hot */
> +              rte_eal_primary_proc_alive(NULL);
> +              rte_eal_primary_proc_alive(NULL)
> +
> +              start = rte_rdtsc();
> +              while (quit_signal < 50000) {
> +                     /* Just testing with and w/o the 'if' line below */
> +                     if (rte_eal_primary_proc_alive(NULL))
> +                             quit_signal++;
>                        for (i = 0; i < num_tuples; i++)
>                                pdump_packets(&pdump_t[i]);
>                 }
> +              end = rte_rdtsc();
> +              printf("Totally count:%u, cost tsc:%lu\n", quit_signal, end - start);
>   
>                 return;
>         }
> The total tsc cost is about 338809671 with rte_eal_primary_proc_alive().
> And the tsc cost is just about 513573 without rte_eal_primary_proc_alive().
> The dpdk-pdump had also used taskset to bind to specify isolate core.
> So it seems the patch do a great performance impact.
> Response> thanks for confirming the suspicion.
> Maybe another async method should be introduced to monitor the primary status.
> Response> yes, without affecting the capture thread.
>
>     2. For various packet sizes and port speed what are delta in drops for packet capture?
>
> A2. Refer to A1, it's not needed anymore.
> Response> A1 there is performance impact.
> Note: If pdump application is still alive when primary is not running, primary cannot be started. Is this a cue that pdump is still alive and has to be terminated?
> Yes, some guys complained that the residual dpdk-pdump impact the restart of the primary app and refuse to add other mechanisms e.g. to kill the dpdk-pdump in the app to avoid that case.
> So the patch was created.
> Is there any other ways to avoid that.
> Response> in my humble opinion, best way around is add user option 
> like ‘--exit’; which then will add periodic rte_timer for user desired 
> seconds ‘0.1, 0.5, 1.0, 5.0’. The timer callback can run on master 
> core which sets ‘quit_signal’ once primary is no longer alive. In case 
> of ‘multi thread’ capture master thread is not involved in 
> dump_packets thus avoiding any packet drops or performance issue..

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Burakov, Anatoly]

On 26-Apr-19 1:08 PM, Suanming.Mou wrote:
> 
> On 2019/4/26 18:56, Varghese, Vipin wrote:
>>
>> I will leave this suggestion open for comments from the maintainer.
>>
> Hi,
> 
> Thanks for your suggestion. I have also tried to add an slave core to 
> monitor the primary status this afternoon.  It works.
> 
> I doubt if it can be add an new option as you suggested, but which will 
> also require people who complain the exiting to add an extra slave core 
> for that.
> 
> Please waiting for the new patch in one or two days.
> 

You can use alarm API to check for this regularly. It's not like the 
interrupt thread is doing much anyway. Just set alarm to fire every N 
seconds, and that's it.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Suanming.Mou]

On 2019/4/26 21:46, Burakov, Anatoly wrote:
> On 26-Apr-19 1:08 PM, Suanming.Mou wrote:
>>
>> On 2019/4/26 18:56, Varghese, Vipin wrote:
>>>
>>> I will leave this suggestion open for comments from the maintainer.
>>>
>> Hi,
>>
>> Thanks for your suggestion. I have also tried to add an slave core to 
>> monitor the primary status this afternoon.  It works.
>>
>> I doubt if it can be add an new option as you suggested, but which 
>> will also require people who complain the exiting to add an extra 
>> slave core for that.
>>
>> Please waiting for the new patch in one or two days.
>>
>
> You can use alarm API to check for this regularly. It's not like the 
> interrupt thread is doing much anyway. Just set alarm to fire every N 
> seconds, and that's it.

Hi,

Thank you very much for the suggestion. Yes, that seems the best 
solution.  Just tested it roughly as the code below:

+static void monitor_primary(void *arg __rte_unused)
+{
+    if (quit_signal)
+        return;
+
+    if (rte_eal_primary_proc_alive(NULL))
+        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+    else
+        quit_signal = 1;
+
+    return;
+}
+
  static inline void
  dump_packets(void)
  {
      int i;
      uint32_t lcore_id = 0;

+    if (exit_with_primary)
+        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+


I will prepare the patch with option exit_with_primary.


Br,

Mou

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Burakov, Anatoly]

On 26-Apr-19 3:32 PM, Suanming.Mou wrote:
> 
> On 2019/4/26 21:46, Burakov, Anatoly wrote:
>> On 26-Apr-19 1:08 PM, Suanming.Mou wrote:
>>>
>>> On 2019/4/26 18:56, Varghese, Vipin wrote:
>>>>
>>>> I will leave this suggestion open for comments from the maintainer.
>>>>
>>> Hi,
>>>
>>> Thanks for your suggestion. I have also tried to add an slave core to 
>>> monitor the primary status this afternoon.  It works.
>>>
>>> I doubt if it can be add an new option as you suggested, but which 
>>> will also require people who complain the exiting to add an extra 
>>> slave core for that.
>>>
>>> Please waiting for the new patch in one or two days.
>>>
>>
>> You can use alarm API to check for this regularly. It's not like the 
>> interrupt thread is doing much anyway. Just set alarm to fire every N 
>> seconds, and that's it.
> 
> Hi,
> 
> Thank you very much for the suggestion. Yes, that seems the best 
> solution.  Just tested it roughly as the code below:
> 
> +static void monitor_primary(void *arg __rte_unused)
> +{
> +    if (quit_signal)
> +        return;
> +
> +    if (rte_eal_primary_proc_alive(NULL))
> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
> +    else
> +        quit_signal = 1;
> +
> +    return;
> +}
> +
>   static inline void
>   dump_packets(void)
>   {
>       int i;
>       uint32_t lcore_id = 0;
> 
> +    if (exit_with_primary)
> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
> +
> 
> 
> I will prepare the patch with option exit_with_primary.
> 

Actually, i'm curious if this really does work. Unless my knowledge is 
out of date, interrupt thread doesn't work in secondary processes, and 
by extension neither should the alarm API...

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Suanming.Mou]

On 2019/4/26 22:39, Burakov, Anatoly wrote:
> On 26-Apr-19 3:32 PM, Suanming.Mou wrote:
>>
>> On 2019/4/26 21:46, Burakov, Anatoly wrote:
>>> On 26-Apr-19 1:08 PM, Suanming.Mou wrote:
>>>>
>>>> On 2019/4/26 18:56, Varghese, Vipin wrote:
>>>>>
>>>>> I will leave this suggestion open for comments from the maintainer.
>>>>>
>>>> Hi,
>>>>
>>>> Thanks for your suggestion. I have also tried to add an slave core 
>>>> to monitor the primary status this afternoon.  It works.
>>>>
>>>> I doubt if it can be add an new option as you suggested, but which 
>>>> will also require people who complain the exiting to add an extra 
>>>> slave core for that.
>>>>
>>>> Please waiting for the new patch in one or two days.
>>>>
>>>
>>> You can use alarm API to check for this regularly. It's not like the 
>>> interrupt thread is doing much anyway. Just set alarm to fire every 
>>> N seconds, and that's it.
>>
>> Hi,
>>
>> Thank you very much for the suggestion. Yes, that seems the best 
>> solution.  Just tested it roughly as the code below:
>>
>> +static void monitor_primary(void *arg __rte_unused)
>> +{
>> +    if (quit_signal)
>> +        return;
>> +
>> +    if (rte_eal_primary_proc_alive(NULL))
>> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
>> +    else
>> +        quit_signal = 1;
>> +
>> +    return;
>> +}
>> +
>>   static inline void
>>   dump_packets(void)
>>   {
>>       int i;
>>       uint32_t lcore_id = 0;
>>
>> +    if (exit_with_primary)
>> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
>> +
>>
>>
>> I will prepare the patch with option exit_with_primary.
>>
>
> Actually, i'm curious if this really does work. Unless my knowledge is 
> out of date, interrupt thread doesn't work in secondary processes, and 
> by extension neither should the alarm API...

Uh... If I understand correctly, the alarm API has used in the secondary 
before.

Refer to handle_primary_request()....

Re: [dpdk-dev] [PATCH] app/pdump: exits once primary app exited

[Burakov, Anatoly]

On 26-Apr-19 3:49 PM, Suanming.Mou wrote:
> 
> On 2019/4/26 22:39, Burakov, Anatoly wrote:
>> On 26-Apr-19 3:32 PM, Suanming.Mou wrote:
>>>
>>> On 2019/4/26 21:46, Burakov, Anatoly wrote:
>>>> On 26-Apr-19 1:08 PM, Suanming.Mou wrote:
>>>>>
>>>>> On 2019/4/26 18:56, Varghese, Vipin wrote:
>>>>>>
>>>>>> I will leave this suggestion open for comments from the maintainer.
>>>>>>
>>>>> Hi,
>>>>>
>>>>> Thanks for your suggestion. I have also tried to add an slave core 
>>>>> to monitor the primary status this afternoon.  It works.
>>>>>
>>>>> I doubt if it can be add an new option as you suggested, but which 
>>>>> will also require people who complain the exiting to add an extra 
>>>>> slave core for that.
>>>>>
>>>>> Please waiting for the new patch in one or two days.
>>>>>
>>>>
>>>> You can use alarm API to check for this regularly. It's not like the 
>>>> interrupt thread is doing much anyway. Just set alarm to fire every 
>>>> N seconds, and that's it.
>>>
>>> Hi,
>>>
>>> Thank you very much for the suggestion. Yes, that seems the best 
>>> solution.  Just tested it roughly as the code below:
>>>
>>> +static void monitor_primary(void *arg __rte_unused)
>>> +{
>>> +    if (quit_signal)
>>> +        return;
>>> +
>>> +    if (rte_eal_primary_proc_alive(NULL))
>>> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
>>> +    else
>>> +        quit_signal = 1;
>>> +
>>> +    return;
>>> +}
>>> +
>>>   static inline void
>>>   dump_packets(void)
>>>   {
>>>       int i;
>>>       uint32_t lcore_id = 0;
>>>
>>> +    if (exit_with_primary)
>>> +        rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
>>> +
>>>
>>>
>>> I will prepare the patch with option exit_with_primary.
>>>
>>
>> Actually, i'm curious if this really does work. Unless my knowledge is 
>> out of date, interrupt thread doesn't work in secondary processes, and 
>> by extension neither should the alarm API...
> 
> Uh... If I understand correctly, the alarm API has used in the secondary 
> before.
> 
> Refer to handle_primary_request()....
> 

Then my knowledge really is out of date :)

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support.

[Suanming.Mou]

When primary app exits, the residual running pdump will stop
the primary app to restart. Add an exit_with_primary option
to make pdump exit with primary.

Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 app/pdump/main.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..3909f15 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -26,11 +26,14 @@
 #include <rte_ring.h>
 #include <rte_string_fns.h>
 #include <rte_pdump.h>
+#include <rte_alarm.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
 #define CMD_LINE_OPT_PDUMP_NUM 256
 #define CMD_LINE_OPT_MULTI "multi"
 #define CMD_LINE_OPT_MULTI_NUM 257
+#define CMD_LINE_OPT_EXIT_WP "exit_with_primary"
+#define CMD_LINE_OPT_EXIT_WP_NUM 258
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -65,6 +68,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define MONITOR_INTERVEL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -143,12 +147,14 @@ struct parse_val {
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
 static uint8_t multiple_core_capture;
+static uint8_t exit_with_primary;
 
 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
 	printf("usage: %s [EAL options]"
+			" --["CMD_LINE_OPT_EXIT_WP"]"
 			" --["CMD_LINE_OPT_MULTI"]\n"
 			" --"CMD_LINE_OPT_PDUMP" "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
@@ -383,6 +389,7 @@ struct parse_val {
 	static struct option long_option[] = {
 		{CMD_LINE_OPT_PDUMP, 1, 0, CMD_LINE_OPT_PDUMP_NUM},
 		{CMD_LINE_OPT_MULTI, 0, 0, CMD_LINE_OPT_MULTI_NUM},
+		{CMD_LINE_OPT_EXIT_WP, 0, 0, CMD_LINE_OPT_EXIT_WP_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -403,6 +410,9 @@ struct parse_val {
 		case CMD_LINE_OPT_MULTI_NUM:
 			multiple_core_capture = 1;
 			break;
+		case CMD_LINE_OPT_EXIT_WP_NUM:
+			exit_with_primary = 1;
+			break;
 		default:
 			pdump_usage(prgname);
 			return -1;
@@ -864,12 +874,28 @@ struct parse_val {
 	return 0;
 }
 
+static void monitor_primary(void *arg __rte_unused)
+{
+	if (quit_signal)
+		return;
+
+	if (rte_eal_primary_proc_alive(NULL))
+		rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+	else
+		quit_signal = 1;
+
+	return;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
 	uint32_t lcore_id = 0;
 
+	if (exit_with_primary)
+		rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+
 	if (!multiple_core_capture) {
 		printf(" core (%u), capture for (%d) tuples\n",
 				rte_lcore_id(), num_tuples);
-- 
1.8.3.4

[dpdk-dev] [PATCH v3] app/pdump: add exit_with_primary option support.

[Suanming.Mou]

When primary app exits, the residual running pdump will stop
the primary app to restart. Add an exit_with_primary option
to make pdump exit with primary.

Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 app/pdump/main.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..b69ef4e 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -26,11 +26,14 @@
 #include <rte_ring.h>
 #include <rte_string_fns.h>
 #include <rte_pdump.h>
+#include <rte_alarm.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
 #define CMD_LINE_OPT_PDUMP_NUM 256
 #define CMD_LINE_OPT_MULTI "multi"
 #define CMD_LINE_OPT_MULTI_NUM 257
+#define CMD_LINE_OPT_EXIT_WP "exit_with_primary"
+#define CMD_LINE_OPT_EXIT_WP_NUM 258
 #define PDUMP_PORT_ARG "port"
 #define PDUMP_PCI_ARG "device_id"
 #define PDUMP_QUEUE_ARG "queue"
@@ -65,6 +68,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define MONITOR_INTERVEL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -143,12 +147,14 @@ struct parse_val {
 static struct rte_eth_conf port_conf_default;
 static volatile uint8_t quit_signal;
 static uint8_t multiple_core_capture;
+static uint8_t exit_with_primary;
 
 /**< display usage */
 static void
 pdump_usage(const char *prgname)
 {
 	printf("usage: %s [EAL options]"
+			" --["CMD_LINE_OPT_EXIT_WP"]"
 			" --["CMD_LINE_OPT_MULTI"]\n"
 			" --"CMD_LINE_OPT_PDUMP" "
 			"'(port=<port id> | device_id=<pci id or vdev name>),"
@@ -383,6 +389,7 @@ struct parse_val {
 	static struct option long_option[] = {
 		{CMD_LINE_OPT_PDUMP, 1, 0, CMD_LINE_OPT_PDUMP_NUM},
 		{CMD_LINE_OPT_MULTI, 0, 0, CMD_LINE_OPT_MULTI_NUM},
+		{CMD_LINE_OPT_EXIT_WP, 0, 0, CMD_LINE_OPT_EXIT_WP_NUM},
 		{NULL, 0, 0, 0}
 	};
 
@@ -403,6 +410,9 @@ struct parse_val {
 		case CMD_LINE_OPT_MULTI_NUM:
 			multiple_core_capture = 1;
 			break;
+		case CMD_LINE_OPT_EXIT_WP_NUM:
+			exit_with_primary = 1;
+			break;
 		default:
 			pdump_usage(prgname);
 			return -1;
@@ -864,12 +874,26 @@ struct parse_val {
 	return 0;
 }
 
+static void monitor_primary(void *arg __rte_unused)
+{
+	if (quit_signal)
+		return;
+
+	if (rte_eal_primary_proc_alive(NULL))
+		rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+	else
+		quit_signal = 1;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
 	uint32_t lcore_id = 0;
 
+	if (exit_with_primary)
+		rte_eal_alarm_set(MONITOR_INTERVEL, monitor_primary, NULL);
+
 	if (!multiple_core_capture) {
 		printf(" core (%u), capture for (%d) tuples\n",
 				rte_lcore_id(), num_tuples);
-- 
1.8.3.4

[dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Suanming.Mou]

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

Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 app/pdump/main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..c3413da 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -26,6 +26,7 @@
 #include <rte_ring.h>
 #include <rte_string_fns.h>
 #include <rte_pdump.h>
+#include <rte_alarm.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
 #define CMD_LINE_OPT_PDUMP_NUM 256
@@ -65,6 +66,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -864,12 +866,26 @@ struct parse_val {
 	return 0;
 }
 
+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;
+}
+
 static inline void
 dump_packets(void)
 {
 	int i;
 	uint32_t lcore_id = 0;
 
+	/* Once primary exits, so will pdump. */
+	rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+
 	if (!multiple_core_capture) {
 		printf(" core (%u), capture for (%d) tuples\n",
 				rte_lcore_id(), num_tuples);
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Varghese, Vipin]

Thanks for the patch work with rte_eal_alaram. But I am not able to find 

1. the documentation update.
2. cover letter.
3. for signal SIGINT, set for ` rte_eal_alarm_cancel`. 

Can you share a new patch as v5 with these changes?

Thanks
Vipin Varghese

> -----Original Message-----
> From: Suanming.Mou <mousuanming@huawei.com>
> Sent: Tuesday, April 30, 2019 9:09 AM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [PATCH v4] 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.
> 
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> ---
>  app/pdump/main.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c index 3d20854..c3413da
> 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -26,6 +26,7 @@
>  #include <rte_ring.h>
>  #include <rte_string_fns.h>
>  #include <rte_pdump.h>
> +#include <rte_alarm.h>
> 
>  #define CMD_LINE_OPT_PDUMP "pdump"
>  #define CMD_LINE_OPT_PDUMP_NUM 256
> @@ -65,6 +66,7 @@
>  #define SIZE 256
>  #define BURST_SIZE 32
>  #define NUM_VDEVS 2
> +#define MONITOR_INTERVAL (500 * 1000)
> 
>  /* true if x is a power of 2 */
>  #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -864,12 +866,26 @@ struct
> parse_val {
>  	return 0;
>  }
> 
> +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;
> +}
> +
>  static inline void
>  dump_packets(void)
>  {
>  	int i;
>  	uint32_t lcore_id = 0;
> 
> +	/* Once primary exits, so will pdump. */
> +	rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> +
>  	if (!multiple_core_capture) {
>  		printf(" core (%u), capture for (%d) tuples\n",
>  				rte_lcore_id(), num_tuples);
> --
> 1.8.3.4

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Suanming.Mou]

On 2019/4/30 10:33, Varghese, Vipin wrote:
> Thanks for the patch work with rte_eal_alaram. But I am not able to find
>
> 1. the documentation update.

I'd like to reconfirm that since Anatoly suggested to make the option 
default, I deleted the option supporting code.

Is it aligned that we can make it default?

If it is aligned to make it default, then if I understand correctly, to 
add a new note in the documentation to describe pdump will exit with 
primary will be OK?

> 2. cover letter.
Sorry for that. Will add the cover letter in the V5 patch.
> 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
Ah, yes.
>
> Can you share a new patch as v5 with these changes?
Thanks for your reviewing. I will send the next patch after Q1 confirmed.
>
> Thanks
> Vipin Varghese
>
>> -----Original Message-----
>> From: Suanming.Mou <mousuanming@huawei.com>
>> Sent: Tuesday, April 30, 2019 9:09 AM
>> To: dev@dpdk.org
>> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>
>> Subject: [PATCH v4] 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.
>>
>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> ---
>>   app/pdump/main.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/app/pdump/main.c b/app/pdump/main.c index 3d20854..c3413da
>> 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -26,6 +26,7 @@
>>   #include <rte_ring.h>
>>   #include <rte_string_fns.h>
>>   #include <rte_pdump.h>
>> +#include <rte_alarm.h>
>>
>>   #define CMD_LINE_OPT_PDUMP "pdump"
>>   #define CMD_LINE_OPT_PDUMP_NUM 256
>> @@ -65,6 +66,7 @@
>>   #define SIZE 256
>>   #define BURST_SIZE 32
>>   #define NUM_VDEVS 2
>> +#define MONITOR_INTERVAL (500 * 1000)
>>
>>   /* true if x is a power of 2 */
>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -864,12 +866,26 @@ struct
>> parse_val {
>>   	return 0;
>>   }
>>
>> +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;
>> +}
>> +
>>   static inline void
>>   dump_packets(void)
>>   {
>>   	int i;
>>   	uint32_t lcore_id = 0;
>>
>> +	/* Once primary exits, so will pdump. */
>> +	rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>> +
>>   	if (!multiple_core_capture) {
>>   		printf(" core (%u), capture for (%d) tuples\n",
>>   				rte_lcore_id(), num_tuples);
>> --
>> 1.8.3.4
>
> .
>

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Varghese, Vipin]

snipped
> >
> > 1. the documentation update.
> 
> I'd like to reconfirm that since Anatoly suggested to make the option default, I
> deleted the option supporting code.
> 
> Is it aligned that we can make it default?
> 
> If it is aligned to make it default, then if I understand correctly, to add a new note
> in the documentation to describe pdump will exit with primary will be OK?

I am not aware of the conversation and will leave this to maintainer if the option should be `--exit-with-primary` or `--do-not-exit-primary`. 

Please add the changes to `doc/guides/tools/proc_info.rst` of the changes and new option that gets added.

> 
> > 2. cover letter.
> Sorry for that. Will add the cover letter in the V5 patch.
thanks

> > 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
> Ah, yes.
Thanks

> >
> > Can you share a new patch as v5 with these changes?
> Thanks for your reviewing. I will send the next patch after Q1 confirmed.
Ok


snipped

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Burakov, Anatoly]

On 30-Apr-19 3:33 AM, Varghese, Vipin wrote:
> Thanks for the patch work with rte_eal_alaram. But I am not able to find
> 
> 1. the documentation update.
> 2. cover letter.

Why would a single patch need a cover letter? I don't think it's needed 
in this case. The commit message is enough.

> 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
> 
> Can you share a new patch as v5 with these changes?
> 
> Thanks
> Vipin Varghese
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Varghese, Vipin]

snipped
> > Thanks for the patch work with rte_eal_alaram. But I am not able to
> > find
> >
> > 1. the documentation update.
> > 2. cover letter.
> 
> Why would a single patch need a cover letter? I don't think it's needed in this
> case. The commit message is enough.

In my opinion, the cover letter is to be added as it is new feature and explains the reasoning behind the new change. Please let me know if there change in the same?

snipped

Re: [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support.

[Burakov, Anatoly]

On 30-Apr-19 11:37 AM, Varghese, Vipin wrote:
> snipped
>>> Thanks for the patch work with rte_eal_alaram. But I am not able to
>>> find
>>>
>>> 1. the documentation update.
>>> 2. cover letter.
>>
>> Why would a single patch need a cover letter? I don't think it's needed in this
>> case. The commit message is enough.
> 
> In my opinion, the cover letter is to be added as it is new feature and explains the reasoning behind the new change. Please let me know if there change in the same?
> 
> snipped
> 

I'm obviously not an expert in cover letters, but in my view, cover 
letter is only necessary whenever there is a complex patchset that 
requires some explanation, background, etc. If there is only one patch, 
everything that you could reasonably put in the cover letter should go 
either into the commit message itself, or into commit notes if there is 
some supplemental data (e.g. benchmark results etc.). Creating cover 
letters for single patches is just unnecessary work IMO.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v5] Make pdump exits with primary.

[Suanming.Mou]

When primary exits, pdump as the sencondary keeps running will make
primary fail to restart. Add the primary monitor in pdump to make it
eixts with primary.

Note:
One more thing still not confirmed is that if it can be set default.
In the V5 patch, it is still set as default. Since it seems there is
no disadvantages to make it exit with primary as default as Anatoly
suggested. But the exit_with_primary configuration is needed, there
will be a v6 patch and the doc update.

Change in v5:
  - add rte_eal_alarm_cancel
  - make the primary monitor enable/disable more indepence.

Change in v4:
  - remove the exit_with_primary option as Anatoly suggested.
  - fix typo.

Change in v3:
  - remove the redundant return.

Change in v2:
  - add exit_with_primary option.
  - use rte_eal_alarm_set to monitor the primary.

Suanming.Mou (1):
  app/pdump: add pudmp exits with primary support.

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

-- 
1.8.3.4

[dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Suanming.Mou]

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

Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
 app/pdump/main.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..cc46f65 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -26,6 +26,7 @@
 #include <rte_ring.h>
 #include <rte_string_fns.h>
 #include <rte_pdump.h>
+#include <rte_alarm.h>
 
 #define CMD_LINE_OPT_PDUMP "pdump"
 #define CMD_LINE_OPT_PDUMP_NUM 256
@@ -65,6 +66,7 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +415,18 @@ struct 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;
+}
+
+static void
 print_pdump_stats(void)
 {
 	int i;
@@ -537,6 +551,18 @@ struct parse_val {
 }
 
 static void
+disable_primary_monitor(void)
+{
+	int ret;
+
+	/* Don't worry about it is primary exit case. The alarm cancel
+	 * function will take care about that. */
+	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to disable monitor fail:%d\n", ret);
+}
+
+static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
@@ -910,6 +936,19 @@ struct parse_val {
 		;
 }
 
+static void
+enable_primary_monitor(void)
+{
+	int ret;
+
+	/* Once primary exits, so will pdump. */
+	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+	if (ret < 0) {
+		cleanup_pdump_resources();
+		rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
+	}
+}
+
 int
 main(int argc, char **argv)
 {
@@ -950,11 +989,13 @@ struct parse_val {
 			rte_exit(EXIT_FAILURE, "Invalid argument\n");
 	}
 
-	/* create mempool, ring and vdevs info */
+	/* create mempool, ring, vdevs info and primary monitor */
 	create_mp_ring_vdev();
 	enable_pdump();
+	enable_primary_monitor();
 	dump_packets();
 
+	disable_primary_monitor();
 	cleanup_pdump_resources();
 	/* dump debug stats */
 	print_pdump_stats();
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Burakov, Anatoly]

On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
> When primary app exits, the residual running pdump will stop the
> primary app to restart. Add pdump exits with primary support.
> 
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> ---

<snip>

>   static void
> +disable_primary_monitor(void)
> +{
> +	int ret;
> +
> +	/* Don't worry about it is primary exit case. The alarm cancel
> +	 * function will take care about that. */
> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> +	if (ret < 0)
> +		printf("Fail to disable monitor fail:%d\n", ret);

Double fail :)

> +}
> +
> +static void
>   signal_handler(int sig_num)
>   {
>   	if (sig_num == SIGINT) {
> @@ -910,6 +936,19 @@ struct parse_val {
>   		;
>   }
>   
> +static void
> +enable_primary_monitor(void)
> +{
> +	int ret;
> +
> +	/* Once primary exits, so will pdump. */
> +	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> +	if (ret < 0) {
> +		cleanup_pdump_resources();
> +		rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
> +	}

Why is this function void, when you could've called rte_exit() in the 
caller on failure? And why is it such a fatal error to set up the timer? 
IMO just a warning would've been enough.

> +}
> +
>   int
>   main(int argc, char **argv)
>   {
> @@ -950,11 +989,13 @@ struct parse_val {
>   			rte_exit(EXIT_FAILURE, "Invalid argument\n");
>   	}
>   
> -	/* create mempool, ring and vdevs info */
> +	/* create mempool, ring, vdevs info and primary monitor */
>   	create_mp_ring_vdev();
>   	enable_pdump();
> +	enable_primary_monitor();
>   	dump_packets();
>   
> +	disable_primary_monitor();
>   	cleanup_pdump_resources();
>   	/* dump debug stats */
>   	print_pdump_stats();
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Suanming.Mou]

On 2019/4/30 17:42, Burakov, Anatoly wrote:
> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>> When primary app exits, the residual running pdump will stop the
>> primary app to restart. Add pdump exits with primary support.
>>
>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> ---
>
> <snip>
>
>>   static void
>> +disable_primary_monitor(void)
>> +{
>> +    int ret;
>> +
>> +    /* Don't worry about it is primary exit case. The alarm cancel
>> +     * function will take care about that. */
>> +    ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>> +    if (ret < 0)
>> +        printf("Fail to disable monitor fail:%d\n", ret);
>
> Double fail :)
Ah, yes, sorry for that the code gets worse.  :(
>
>> +}
>> +
>> +static void
>>   signal_handler(int sig_num)
>>   {
>>       if (sig_num == SIGINT) {
>> @@ -910,6 +936,19 @@ struct parse_val {
>>           ;
>>   }
>>   +static void
>> +enable_primary_monitor(void)
>> +{
>> +    int ret;
>> +
>> +    /* Once primary exits, so will pdump. */
>> +    ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>> +    if (ret < 0) {
>> +        cleanup_pdump_resources();
>> +        rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>> +    }
>
> Why is this function void, when you could've called rte_exit() in the 
> caller on failure? And why is it such a fatal error to set up the 
> timer? IMO just a warning would've been enough.

Here comes with two issues:

Q1. The return value of the function:

A1: I'm so sorry that it does not seem to make sense to check the 
function's return value. Does it mean if we change the timer set up from 
error to warning, then we can use the return value to judge if need to 
disable the primary_monitor?

Q2. The choice when rte_eal_alarm_set fail:

A2: OK, agree with that.

>
>> +}
>> +
>>   int
>>   main(int argc, char **argv)
>>   {
>> @@ -950,11 +989,13 @@ struct parse_val {
>>               rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>       }
>>   -    /* create mempool, ring and vdevs info */
>> +    /* create mempool, ring, vdevs info and primary monitor */
>>       create_mp_ring_vdev();
>>       enable_pdump();
>> +    enable_primary_monitor();
>>       dump_packets();
>>   +    disable_primary_monitor();
>>       cleanup_pdump_resources();
>>       /* dump debug stats */
>>       print_pdump_stats();
>>
>
>

Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Burakov, Anatoly]

On 30-Apr-19 12:25 PM, Suanming.Mou wrote:
> 
> On 2019/4/30 17:42, Burakov, Anatoly wrote:
>> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>>> When primary app exits, the residual running pdump will stop the
>>> primary app to restart. Add pdump exits with primary support.
>>>
>>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>> ---
>>
>> <snip>
>>
>>>   static void
>>> +disable_primary_monitor(void)
>>> +{
>>> +    int ret;
>>> +
>>> +    /* Don't worry about it is primary exit case. The alarm cancel
>>> +     * function will take care about that. */
>>> +    ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>> +    if (ret < 0)
>>> +        printf("Fail to disable monitor fail:%d\n", ret);
>>
>> Double fail :)
> Ah, yes, sorry for that the code gets worse.  :(
>>
>>> +}
>>> +
>>> +static void
>>>   signal_handler(int sig_num)
>>>   {
>>>       if (sig_num == SIGINT) {
>>> @@ -910,6 +936,19 @@ struct parse_val {
>>>           ;
>>>   }
>>>   +static void
>>> +enable_primary_monitor(void)
>>> +{
>>> +    int ret;
>>> +
>>> +    /* Once primary exits, so will pdump. */
>>> +    ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>>> +    if (ret < 0) {
>>> +        cleanup_pdump_resources();
>>> +        rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>>> +    }
>>
>> Why is this function void, when you could've called rte_exit() in the 
>> caller on failure? And why is it such a fatal error to set up the 
>> timer? IMO just a warning would've been enough.
> 
> Here comes with two issues:
> 
> Q1. The return value of the function:
> 
> A1: I'm so sorry that it does not seem to make sense to check the 
> function's return value. Does it mean if we change the timer set up from 
> error to warning, then we can use the return value to judge if need to 
> disable the primary_monitor?
> 
> Q2. The choice when rte_eal_alarm_set fail:
> 
> A2: OK, agree with that.

If this is non-fatal, no need to change anything - just print out a 
warning instead of rte_exit, and no more changes needed here.

> 
>>
>>> +}
>>> +
>>>   int
>>>   main(int argc, char **argv)
>>>   {
>>> @@ -950,11 +989,13 @@ struct parse_val {
>>>               rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>>       }
>>>   -    /* create mempool, ring and vdevs info */
>>> +    /* create mempool, ring, vdevs info and primary monitor */
>>>       create_mp_ring_vdev();
>>>       enable_pdump();
>>> +    enable_primary_monitor();
>>>       dump_packets();
>>>   +    disable_primary_monitor();
>>>       cleanup_pdump_resources();
>>>       /* dump debug stats */
>>>       print_pdump_stats();
>>>
>>
>>
> 
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Suanming.Mou]

On 2019/5/1 0:39, Burakov, Anatoly wrote:
> On 30-Apr-19 12:25 PM, Suanming.Mou wrote:
>>
>> On 2019/4/30 17:42, Burakov, Anatoly wrote:
>>> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>>>> When primary app exits, the residual running pdump will stop the
>>>> primary app to restart. Add pdump exits with primary support.
>>>>
>>>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>>>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>>> ---
>>>
>>> <snip>
>>>
>>>>   static void
>>>> +disable_primary_monitor(void)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    /* Don't worry about it is primary exit case. The alarm cancel
>>>> +     * function will take care about that. */
>>>> +    ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>>> +    if (ret < 0)
>>>> +        printf("Fail to disable monitor fail:%d\n", ret);
>>>
>>> Double fail :)
>> Ah, yes, sorry for that the code gets worse.  :(
>>>
>>>> +}
>>>> +
>>>> +static void
>>>>   signal_handler(int sig_num)
>>>>   {
>>>>       if (sig_num == SIGINT) {
>>>> @@ -910,6 +936,19 @@ struct parse_val {
>>>>           ;
>>>>   }
>>>>   +static void
>>>> +enable_primary_monitor(void)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    /* Once primary exits, so will pdump. */
>>>> +    ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>>>> +    if (ret < 0) {
>>>> +        cleanup_pdump_resources();
>>>> +        rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>>>> +    }
>>>
>>> Why is this function void, when you could've called rte_exit() in 
>>> the caller on failure? And why is it such a fatal error to set up 
>>> the timer? IMO just a warning would've been enough.
>>
>> Here comes with two issues:
>>
>> Q1. The return value of the function:
>>
>> A1: I'm so sorry that it does not seem to make sense to check the 
>> function's return value. Does it mean if we change the timer set up 
>> from error to warning, then we can use the return value to judge if 
>> need to disable the primary_monitor?
>>
>> Q2. The choice when rte_eal_alarm_set fail:
>>
>> A2: OK, agree with that.
>
> If this is non-fatal, no need to change anything - just print out a 
> warning instead of rte_exit, and no more changes needed here.
Happy may day holiday! Thanks for your confirmation.
>
>>
>>>
>>>> +}
>>>> +
>>>>   int
>>>>   main(int argc, char **argv)
>>>>   {
>>>> @@ -950,11 +989,13 @@ struct parse_val {
>>>>               rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>>>       }
>>>>   -    /* create mempool, ring and vdevs info */
>>>> +    /* create mempool, ring, vdevs info and primary monitor */
>>>>       create_mp_ring_vdev();
>>>>       enable_pdump();
>>>> +    enable_primary_monitor();
>>>>       dump_packets();
>>>>   +    disable_primary_monitor();
>>>>       cleanup_pdump_resources();
>>>>       /* dump debug stats */
>>>>       print_pdump_stats();
>>>>
>>>
>>>
>>
>>
>
>

Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
> Sent: Tuesday, April 30, 2019 12:35 PM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v5] 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.
> 
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>

You need to run below scripts, and fix the issues reported before sending next version.

1)
Ex:
export DPDK_CHECKPATCH_PATH=/usr/src/kernels/4.18.9-200.fc28.x86_64/scripts/checkpatch.pl
[DPDK]# ./devtools/checkpatches.sh

### app/pdump: add pudmp exits with primary support.

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#62: FILE: app/pdump/main.c:559:
+        * function will take care about that. */

total: 0 errors, 1 warnings, 83 lines checked

0/1 valid patch

2)
[DPDK]# ./devtools/check-git-log.sh
Wrong headline format:
        app/pdump: add pudmp exits with primary support.

Need to remove full stop at the end.

Wrong tag:
        Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
        Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>

Might need to remove ,

> @@ -26,6 +26,7 @@
>  #include <rte_ring.h>
>  #include <rte_string_fns.h>
>  #include <rte_pdump.h>
> +#include <rte_alarm.h>
> 

Nitpick:

As per the coding guidelines(https://doc.dpdk.org/guides/contributing/coding_style.html)
the order of the headers is below, so you can  include rte_alarm.h after <rte_eal.h> 

libc includes (system includes first)
DPDK EAL includes
DPDK misc libraries includes
application-specific includes

> +#define MONITOR_INTERVAL (500 * 1000)

Adding comment on how many ms/us would be helpful.

<snipped>
> +	/* Don't worry about it is primary exit case. The alarm cancel
> +	 * function will take care about that. */

Multiline comments should be like below.


/*
 * Multi-line comments look like this.  Make them real sentences. Fill
 * them so they look like real paragraphs.
 */

Source: https://doc.dpdk.org/guides/contributing/coding_style.html


Thanks,
Reshma

[dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

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(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..947fea3 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -13,6 +13,7 @@
 #include <net/if.h>
 
 #include <rte_eal.h>
+#include <rte_alarm.h>
 #include <rte_common.h>
 #include <rte_debug.h>
 #include <rte_ethdev.h>
@@ -65,6 +66,8 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+/* Enough to set it to 500ms for exiting. */
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +416,18 @@ struct 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;
+}
+
+static void
 print_pdump_stats(void)
 {
 	int i;
@@ -537,6 +552,20 @@ struct parse_val {
 }
 
 static void
+disable_primary_monitor(void)
+{
+	int ret;
+
+	/*
+	 * Don't worry about it is primary exit case. The alarm cancel
+	 * function will take care about that. Ignore the ENOENT case.
+	 */
+	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to disable monitor:%d\n", ret);
+}
+
+static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
@@ -910,6 +939,17 @@ struct parse_val {
 		;
 }
 
+static void
+enable_primary_monitor(void)
+{
+	int ret;
+
+	/* Once primary exits, so will pdump. */
+	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to enable monitor:%d\n", ret);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -950,11 +990,13 @@ struct parse_val {
 			rte_exit(EXIT_FAILURE, "Invalid argument\n");
 	}
 
-	/* create mempool, ring and vdevs info */
+	/* create mempool, ring, vdevs info and primary monitor */
 	create_mp_ring_vdev();
 	enable_pdump();
+	enable_primary_monitor();
 	dump_packets();
 
+	disable_primary_monitor();
 	cleanup_pdump_resources();
 	/* dump debug stats */
 	print_pdump_stats();
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Varghese, Vipin]

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

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

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
>
>

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Burakov, Anatoly]

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.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Varghese, Vipin]

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

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Pattan, Reshma]

> -----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

Re: [dpdk-dev] [PATCH v6] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

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
>
>

[dpdk-dev] [PATCH v7] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

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>
---
V7:
* omit `else` in monitor_primary.
* add eixting due to primary is not alive message.
* add pdump.rst update.

 app/pdump/main.c           | 47 +++++++++++++++++++++++++++++++++++++++++++++-
 doc/guides/tools/pdump.rst |  2 ++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..0b74c27 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -13,6 +13,7 @@
 #include <net/if.h>
 
 #include <rte_eal.h>
+#include <rte_alarm.h>
 #include <rte_common.h>
 #include <rte_debug.h>
 #include <rte_ethdev.h>
@@ -65,6 +66,8 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+/* Enough to set it to 500ms for exiting. */
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +416,21 @@ struct 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);
+		return;
+	}
+
+	printf("Exiting dump while primary is not alive...\n");
+	quit_signal = 1;
+}
+
+static void
 print_pdump_stats(void)
 {
 	int i;
@@ -537,6 +555,20 @@ struct parse_val {
 }
 
 static void
+disable_primary_monitor(void)
+{
+	int ret;
+
+	/*
+	 * Don't worry about it is primary exit case. The alarm cancel
+	 * function will take care about that. Ignore the ENOENT case.
+	 */
+	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to disable monitor:%d\n", ret);
+}
+
+static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
@@ -910,6 +942,17 @@ struct parse_val {
 		;
 }
 
+static void
+enable_primary_monitor(void)
+{
+	int ret;
+
+	/* Once primary exits, so will pdump. */
+	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to enable monitor:%d\n", ret);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -950,11 +993,13 @@ struct parse_val {
 			rte_exit(EXIT_FAILURE, "Invalid argument\n");
 	}
 
-	/* create mempool, ring and vdevs info */
+	/* create mempool, ring, vdevs info and primary monitor */
 	create_mp_ring_vdev();
 	enable_pdump();
+	enable_primary_monitor();
 	dump_packets();
 
+	disable_primary_monitor();
 	cleanup_pdump_resources();
 	/* dump debug stats */
 	print_pdump_stats();
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 53cd2b4..62b4a5e 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -26,6 +26,8 @@ a DPDK secondary process and is capable of enabling packet capture on dpdk ports
         Once the libpcap development files are installed, the libpcap based PMD
         can be enabled by setting CONFIG_RTE_LIBRTE_PMD_PCAP=y and recompiling the DPDK.
 
+      * The ``dpdk-pdump`` tool runs as a DPDK secondary process. It exits when
+        the primary application exits.
 
 Running the Application
 -----------------------
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v7] app/pdump: add pudmp exits with primary support

[Pattan, Reshma]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
> Sent: Thursday, May 2, 2019 1:35 PM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v7] 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>

Acked-by: Reshma Pattan  <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH v7] app/pdump: add pudmp exits with primary support

[Burakov, Anatoly]

On 02-May-19 1:35 PM, Suanming.Mou wrote:
> 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>
> ---

<...>

> +	if (rte_eal_primary_proc_alive(NULL)) {
> +		rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> +		return;
> +	}
> +
> +	printf("Exiting dump while primary is not alive...\n");

May i suggest rewording:

Primary process is no longer active, exiting

I think the above would be clearer.

Otherwise,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

> +	quit_signal = 1;
> +}
> +
> +static void
>   print_pdump_stats(void)
>   {

<snip>

> +	disable_primary_monitor();
>   	cleanup_pdump_resources();
>   	/* dump debug stats */
>   	print_pdump_stats();
> diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
> index 53cd2b4..62b4a5e 100644
> --- a/doc/guides/tools/pdump.rst
> +++ b/doc/guides/tools/pdump.rst
> @@ -26,6 +26,8 @@ a DPDK secondary process and is capable of enabling packet capture on dpdk ports
>           Once the libpcap development files are installed, the libpcap based PMD
>           can be enabled by setting CONFIG_RTE_LIBRTE_PMD_PCAP=y and recompiling the DPDK.
>   
> +      * The ``dpdk-pdump`` tool runs as a DPDK secondary process. It exits when
> +        the primary application exits.
>   
>   Running the Application
>   -----------------------
> 

Worth adding this to release notes as well?

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

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>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
---
V8:
* reword the print info in monitor_primary.
* add release_19_05.rst update.
* add `Reviewed-by` tag.

 app/pdump/main.c                       | 47 +++++++++++++++++++++++++++++++++-
 doc/guides/rel_notes/release_19_05.rst |  4 +++
 doc/guides/tools/pdump.rst             |  2 ++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..f92098f 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -13,6 +13,7 @@
 #include <net/if.h>
 
 #include <rte_eal.h>
+#include <rte_alarm.h>
 #include <rte_common.h>
 #include <rte_debug.h>
 #include <rte_ethdev.h>
@@ -65,6 +66,8 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+/* Enough to set it to 500ms for exiting. */
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +416,21 @@ struct 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);
+		return;
+	}
+
+	printf("Primary process is no longer active, exiting...\n");
+	quit_signal = 1;
+}
+
+static void
 print_pdump_stats(void)
 {
 	int i;
@@ -537,6 +555,20 @@ struct parse_val {
 }
 
 static void
+disable_primary_monitor(void)
+{
+	int ret;
+
+	/*
+	 * Don't worry about it is primary exit case. The alarm cancel
+	 * function will take care about that. Ignore the ENOENT case.
+	 */
+	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to disable monitor:%d\n", ret);
+}
+
+static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
@@ -910,6 +942,17 @@ struct parse_val {
 		;
 }
 
+static void
+enable_primary_monitor(void)
+{
+	int ret;
+
+	/* Once primary exits, so will pdump. */
+	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to enable monitor:%d\n", ret);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -950,11 +993,13 @@ struct parse_val {
 			rte_exit(EXIT_FAILURE, "Invalid argument\n");
 	}
 
-	/* create mempool, ring and vdevs info */
+	/* create mempool, ring, vdevs info and primary monitor */
 	create_mp_ring_vdev();
 	enable_pdump();
+	enable_primary_monitor();
 	dump_packets();
 
+	disable_primary_monitor();
 	cleanup_pdump_resources();
 	/* dump debug stats */
 	print_pdump_stats();
diff --git a/doc/guides/rel_notes/release_19_05.rst b/doc/guides/rel_notes/release_19_05.rst
index 468e325..7c4b39e 100644
--- a/doc/guides/rel_notes/release_19_05.rst
+++ b/doc/guides/rel_notes/release_19_05.rst
@@ -200,6 +200,10 @@ New Features
   Improved testpmd application performance on ARM platform. For ``macswap``
   forwarding mode, NEON intrinsics were used to do swap to save CPU cycles.
 
+* **Updated the pdump application.**
+
+  Add support for pdump to exit with primary process.
+
 
 Removed Items
 -------------
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 53cd2b4..62b4a5e 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -26,6 +26,8 @@ a DPDK secondary process and is capable of enabling packet capture on dpdk ports
         Once the libpcap development files are installed, the libpcap based PMD
         can be enabled by setting CONFIG_RTE_LIBRTE_PMD_PCAP=y and recompiling the DPDK.
 
+      * The ``dpdk-pdump`` tool runs as a DPDK secondary process. It exits when
+        the primary application exits.
 
 Running the Application
 -----------------------
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Thomas Monjalon]

Hi,

03/05/2019 07:48, Suanming. Mou:
> When primary app exits, the residual running pdump will stop the
> primary app to restart. Add pdump exits with primary support.

Sorry I fail to parse this sentence.
Maybe it should be longer to be more explicit about
what it the current issue and how it is solved.
Some comments in the code may also be improved.

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

I think you should remove the dot in your name.

> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> V8:
> * reword the print info in monitor_primary.
> * add release_19_05.rst update.

As it is not a fix, it will slip in 19.08.

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

On 2019/5/5 5:17, Thomas Monjalon wrote:
> Hi,
>
> 03/05/2019 07:48, Suanming. Mou:
>> When primary app exits, the residual running pdump will stop the
>> primary app to restart. Add pdump exits with primary support.
> Sorry I fail to parse this sentence.
> Maybe it should be longer to be more explicit about
> what it the current issue and how it is solved.
Thanks for the suggestion. It's fine to add more contents.
> Some comments in the code may also be improved.

Could you please help to show the detail? Or it will be hard to do the 
improvement.

` There are a thousand Hamlets in a thousand people's eyes.`

>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> I think you should remove the dot in your name.

Sorry for that, maybe I missed some important instructions which notes 
the dot.

I will remove it later.

>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
>> ---
>> V8:
>> * reword the print info in monitor_primary.
>> * add release_19_05.rst update.
> As it is not a fix, it will slip in 19.08.

It seems 19.08 is still not started yet.

Does that mean the patch should be hung till 19.08 be started?

>
>
>
>
>

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Thomas Monjalon]

05/05/2019 03:20, Suanming. Mou:
> On 2019/5/5 5:17, Thomas Monjalon wrote:
> > Hi,
> >
> > 03/05/2019 07:48, Suanming. Mou:
> >> When primary app exits, the residual running pdump will stop the
> >> primary app to restart. Add pdump exits with primary support.
> > Sorry I fail to parse this sentence.
> > Maybe it should be longer to be more explicit about
> > what it the current issue and how it is solved.
> Thanks for the suggestion. It's fine to add more contents.
> > Some comments in the code may also be improved.
> 
> Could you please help to show the detail? Or it will be hard to do the 
> improvement.
> 
> ` There are a thousand Hamlets in a thousand people's eyes.`

Yes, you're right :)

[..]
> >> V8:
> >> * reword the print info in monitor_primary.
> >> * add release_19_05.rst update.
> > As it is not a fix, it will slip in 19.08.
> 
> It seems 19.08 is still not started yet.
> 
> Does that mean the patch should be hung till 19.08 be started?

Yes, either to wait one week, or fake the 19.08 release notes template
to do your patch on top.

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

On 2019/5/5 17:42, Thomas Monjalon wrote:
> Yes, either to wait one week, or fake the 19.08 release notes template
> to do your patch on top.
Thanks.  I think it will be updated when 19.08 started.

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Thomas Monjalon]

Hi,

I try to suggest some rewording below.

03/05/2019 07:48, Suanming. Mou:
> +/* Enough to set it to 500ms for exiting. */
> +#define MONITOR_INTERVAL (500 * 1000)

What is "enough"?
What is "it"?
What is the relation between MONITOR_INTERVAL and exiting?

[...]
> +	/*
> +	 * Don't worry about it is primary exit case. The alarm cancel
> +	 * function will take care about that. Ignore the ENOENT case.
> +	 */

I don't understand the comment.
Maybe you can explicit what is "it" and "that".
It would be probably simpler if you just describe why you cancel
this alarm.
How is it related to ENOENT?

> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> +	if (ret < 0)
> +		printf("Fail to disable monitor:%d\n", ret);

[...]
> -	/* create mempool, ring and vdevs info */
> +	/* create mempool, ring, vdevs info and primary monitor */

I don't see any value to this comment.
You may just drop it.

>  	create_mp_ring_vdev();
>  	enable_pdump();
> +	enable_primary_monitor();

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

On 2019/5/8 16:04, Thomas Monjalon wrote:
> Hi,
>
> I try to suggest some rewording below.

Thanks very much.

First,  let me explain what is the patch work for.

As we all know, the pdump tool works as the secondary process.

When the primary process exits, if the secondary process keeps running 
there, it will make the primary process can't start up again.

Since the ex-fbarry files are still attached by the secondary process 
pdump, the 'new' primary process can't get these files locked.

The patch is just to set up an alarm which runs every 0.5s periodically 
to monitor the primary process in the pdump.

Once the primary exits, so will the pdump.

>
> 03/05/2019 07:48, Suanming. Mou:
>> +/* Enough to set it to 500ms for exiting. */
>> +#define MONITOR_INTERVAL (500 * 1000)
> What is "enough"?

The 'enough' here means it will be fine to make the alarm to run 
periodically in every 500ms to check if the primary process exits.

(Yes, someone can also suggest, "Well , I think xxxms will be better.")

> What is "it"?

So, the "it" here means the MONITOR_INTERVAL...

> What is the relation between MONITOR_INTERVAL and exiting?

Once the primary exits, as the alarm runs every 500ms, the alarm will 
help the pdump to exit, the worst case will take 500ms for the pudmp to 
exit.

>
> [...]
>> +	/*
>> +	 * Don't worry about it is primary exit case. The alarm cancel
>> +	 * function will take care about that. Ignore the ENOENT case.
>> +	 */
> I don't understand the comment.
> Maybe you can explicit what is "it" and "that".
> It would be probably simpler if you just describe why you cancel
> this alarm.
> How is it related to ENOENT?

The 'it' and 'that' both means the pdump 'monitor_primary' recognises 
the primary process exited and set the 'quit_signal' case.

In that case, the 'monitor_primary' is not readd to the alarm. I add the 
comment in case someone want to say that "You are canceling a 
non-existent alarm."

It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just 
return 0 and set the ENOENT errno.

>
>> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>> +	if (ret < 0)
>> +		printf("Fail to disable monitor:%d\n", ret);
> [...]
>> -	/* create mempool, ring and vdevs info */
>> +	/* create mempool, ring, vdevs info and primary monitor */
> I don't see any value to this comment.
> You may just drop it.
That's OK.
>
>>   	create_mp_ring_vdev();
>>   	enable_pdump();
>> +	enable_primary_monitor();
>
>
>
And one more word, the 'app' in the git log  means 'application'.

Maybe it's better to change it to  'process' to make it describes more 
clearly.

Thanks again for the suggestions.

BR,

Mou

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Thomas Monjalon]

About the title, you can write: "app/pdump: exit with primary process"

08/05/2019 11:37, Suanming. Mou:
> On 2019/5/8 16:04, Thomas Monjalon wrote:
> > Hi,
> >
> > I try to suggest some rewording below.
> 
> Thanks very much.
> 
> First,  let me explain what is the patch work for.
> 
> As we all know, the pdump tool works as the secondary process.
> 
> When the primary process exits, if the secondary process keeps running 
> there, it will make the primary process can't start up again.
> 
> Since the ex-fbarry files are still attached by the secondary process 
> pdump, the 'new' primary process can't get these files locked.
> 
> The patch is just to set up an alarm which runs every 0.5s periodically 
> to monitor the primary process in the pdump.
> 
> Once the primary exits, so will the pdump.

If you feel some explanation is missing,
feel free to add it in the commit log.

> > 03/05/2019 07:48, Suanming. Mou:
> >> +/* Enough to set it to 500ms for exiting. */
> >> +#define MONITOR_INTERVAL (500 * 1000)
> > What is "enough"?
> 
> The 'enough' here means it will be fine to make the alarm to run 
> periodically in every 500ms to check if the primary process exits.
> 
> (Yes, someone can also suggest, "Well , I think xxxms will be better.")

So it looks like you reply to a ghost in the code comment :)

> > What is "it"?
> 
> So, the "it" here means the MONITOR_INTERVAL...
> 
> > What is the relation between MONITOR_INTERVAL and exiting?
> 
> Once the primary exits, as the alarm runs every 500ms, the alarm will 
> help the pdump to exit, the worst case will take 500ms for the pudmp to 
> exit.

Let's write it in a simpler descriptive form:
	/* Maximum delay for exiting after primary process. */

> > [...]
> >> +	/*
> >> +	 * Don't worry about it is primary exit case. The alarm cancel
> >> +	 * function will take care about that. Ignore the ENOENT case.
> >> +	 */
> > I don't understand the comment.
> > Maybe you can explicit what is "it" and "that".
> > It would be probably simpler if you just describe why you cancel
> > this alarm.
> > How is it related to ENOENT?
> 
> The 'it' and 'that' both means the pdump 'monitor_primary' recognises 
> the primary process exited and set the 'quit_signal' case.
> 
> In that case, the 'monitor_primary' is not readd to the alarm. I add the 
> comment in case someone want to say that "You are canceling a 
> non-existent alarm."
> 
> It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just 
> return 0 and set the ENOENT errno.

OK, so let's be more explicit:
"
	Cancel monitoring of primary process.
	There will be no error if no alarm is set
	(in case primary process kill was detected earlier).
"

> >> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> >> +	if (ret < 0)
> >> +		printf("Fail to disable monitor:%d\n", ret);

[...]
> And one more word, the 'app' in the git log  means 'application'.
> 
> Maybe it's better to change it to  'process' to make it describes more 
> clearly.

Indeed, "process" is more correct in this context.

> Thanks again for the suggestions.

You're welcome.

Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

[Suanming.Mou]

On 2019/5/8 18:22, Thomas Monjalon wrote:
> About the title, you can write: "app/pdump: exit with primary process"
>
> 08/05/2019 11:37, Suanming. Mou:
>> On 2019/5/8 16:04, Thomas Monjalon wrote:
>>> Hi,
>>>
>>> I try to suggest some rewording below.
>> Thanks very much.
>>
>> First,  let me explain what is the patch work for.
>>
>> As we all know, the pdump tool works as the secondary process.
>>
>> When the primary process exits, if the secondary process keeps running
>> there, it will make the primary process can't start up again.
>>
>> Since the ex-fbarry files are still attached by the secondary process
>> pdump, the 'new' primary process can't get these files locked.
>>
>> The patch is just to set up an alarm which runs every 0.5s periodically
>> to monitor the primary process in the pdump.
>>
>> Once the primary exits, so will the pdump.
> If you feel some explanation is missing,
> feel free to add it in the commit log.
Yes, it will be updated in the next patch.
>
>>> 03/05/2019 07:48, Suanming. Mou:
>>>> +/* Enough to set it to 500ms for exiting. */
>>>> +#define MONITOR_INTERVAL (500 * 1000)
>>> What is "enough"?
>> The 'enough' here means it will be fine to make the alarm to run
>> periodically in every 500ms to check if the primary process exits.
>>
>> (Yes, someone can also suggest, "Well , I think xxxms will be better.")
> So it looks like you reply to a ghost in the code comment :)
:)
>
>>> What is "it"?
>> So, the "it" here means the MONITOR_INTERVAL...
>>
>>> What is the relation between MONITOR_INTERVAL and exiting?
>> Once the primary exits, as the alarm runs every 500ms, the alarm will
>> help the pdump to exit, the worst case will take 500ms for the pudmp to
>> exit.
> Let's write it in a simpler descriptive form:
> 	/* Maximum delay for exiting after primary process. */
Got it.
>
>>> [...]
>>>> +	/*
>>>> +	 * Don't worry about it is primary exit case. The alarm cancel
>>>> +	 * function will take care about that. Ignore the ENOENT case.
>>>> +	 */
>>> I don't understand the comment.
>>> Maybe you can explicit what is "it" and "that".
>>> It would be probably simpler if you just describe why you cancel
>>> this alarm.
>>> How is it related to ENOENT?
>> The 'it' and 'that' both means the pdump 'monitor_primary' recognises
>> the primary process exited and set the 'quit_signal' case.
>>
>> In that case, the 'monitor_primary' is not readd to the alarm. I add the
>> comment in case someone want to say that "You are canceling a
>> non-existent alarm."
>>
>> It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just
>> return 0 and set the ENOENT errno.
> OK, so let's be more explicit:
> "
> 	Cancel monitoring of primary process.
> 	There will be no error if no alarm is set
> 	(in case primary process kill was detected earlier).
> "
Great, it becomes more official now.  The original one seems little 
emotional.
>>>> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>>> +	if (ret < 0)
>>>> +		printf("Fail to disable monitor:%d\n", ret);
> [...]
>> And one more word, the 'app' in the git log  means 'application'.
>>
>> Maybe it's better to change it to  'process' to make it describes more
>> clearly.
> Indeed, "process" is more correct in this context.
>
>> Thanks again for the suggestions.
> You're welcome.
>
>
>
> .
>

[dpdk-dev] [PATCH v9] app/pdump: exit with primary process

[Suanming.Mou]

From: Suanming Mou <mousuanming@huawei.com>

The pdump tool works as the secondary process. When the primary process
exits and the residual secondary process keeps running, it will make the
primary process can't start up again. Since the ex-fbarry files are still
attached by the secondary process pdump, the 'new' primary process can't
get these files locked.

The patch is to set up an alarm which runs every 0.5s periodically
to monitor the primary process in the pdump. Once the primary exits,
so will the pdump.

Signed-off-by: Suanming Mou <mousuanming@huawei.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
Reviewed-by: Thomas Monjalon <thomas@monjalon.net>
---
V9:
* reword the comments and update the git log.
* move release note to release_19_08.rst.
* remove dot in name.

 app/pdump/main.c                       | 46 ++++++++++++++++++++++++++++++++++
 doc/guides/rel_notes/release_19_08.rst |  3 +++
 doc/guides/tools/pdump.rst             |  2 ++
 3 files changed, 51 insertions(+)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index 3d20854..859a96c 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -13,6 +13,7 @@
 #include <net/if.h>
 
 #include <rte_eal.h>
+#include <rte_alarm.h>
 #include <rte_common.h>
 #include <rte_debug.h>
 #include <rte_ethdev.h>
@@ -65,6 +66,8 @@
 #define SIZE 256
 #define BURST_SIZE 32
 #define NUM_VDEVS 2
+/* Maximum delay for exiting after primary process. */
+#define MONITOR_INTERVAL (500 * 1000)
 
 /* true if x is a power of 2 */
 #define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +416,21 @@ struct 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);
+		return;
+	}
+
+	printf("Primary process is no longer active, exiting...\n");
+	quit_signal = 1;
+}
+
+static void
 print_pdump_stats(void)
 {
 	int i;
@@ -537,6 +555,21 @@ struct parse_val {
 }
 
 static void
+disable_primary_monitor(void)
+{
+	int ret;
+
+	/*
+	 * Cancel monitoring of primary process.
+	 * There will be no error if no alarm is set
+	 * (in case primary process kill was detected earlier).
+	 */
+	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to disable monitor:%d\n", ret);
+}
+
+static void
 signal_handler(int sig_num)
 {
 	if (sig_num == SIGINT) {
@@ -910,6 +943,17 @@ struct parse_val {
 		;
 }
 
+static void
+enable_primary_monitor(void)
+{
+	int ret;
+
+	/* Once primary exits, so will pdump. */
+	ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+	if (ret < 0)
+		printf("Fail to enable monitor:%d\n", ret);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -953,8 +997,10 @@ struct parse_val {
 	/* create mempool, ring and vdevs info */
 	create_mp_ring_vdev();
 	enable_pdump();
+	enable_primary_monitor();
 	dump_packets();
 
+	disable_primary_monitor();
 	cleanup_pdump_resources();
 	/* dump debug stats */
 	print_pdump_stats();
diff --git a/doc/guides/rel_notes/release_19_08.rst b/doc/guides/rel_notes/release_19_08.rst
index b9510f9..e3a22e0 100644
--- a/doc/guides/rel_notes/release_19_08.rst
+++ b/doc/guides/rel_notes/release_19_08.rst
@@ -54,6 +54,9 @@ New Features
      Also, make sure to start the actual text at the margin.
      =========================================================
 
+* **Updated the pdump application.**
+
+  Add support for pdump to exit with primary process.
 
 Removed Items
 -------------
diff --git a/doc/guides/tools/pdump.rst b/doc/guides/tools/pdump.rst
index 53cd2b4..62b4a5e 100644
--- a/doc/guides/tools/pdump.rst
+++ b/doc/guides/tools/pdump.rst
@@ -26,6 +26,8 @@ a DPDK secondary process and is capable of enabling packet capture on dpdk ports
         Once the libpcap development files are installed, the libpcap based PMD
         can be enabled by setting CONFIG_RTE_LIBRTE_PMD_PCAP=y and recompiling the DPDK.
 
+      * The ``dpdk-pdump`` tool runs as a DPDK secondary process. It exits when
+        the primary application exits.
 
 Running the Application
 -----------------------
-- 
1.8.3.4

Re: [dpdk-dev] [PATCH v9] app/pdump: exit with primary process

[Pattan, Reshma]

> -----Original Message-----
> From: Suanming.Mou [mailto:mousuanming@huawei.com]
> Sent: Wednesday, May 15, 2019 6:11 AM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; thomas@monjalon.net; Pattan, Reshma
> <reshma.pattan@intel.com>
> Subject: [PATCH v9] app/pdump: exit with primary process
> 
> From: Suanming Mou <mousuanming@huawei.com>
> 
> The pdump tool works as the secondary process. When the primary process
> exits and the residual secondary process keeps running, it will make the
> primary process can't start up again. Since the ex-fbarry files are still
> attached by the secondary process pdump, the 'new' primary process can't
> get these files locked.
> 
> The patch is to set up an alarm which runs every 0.5s periodically to monitor
> the primary process in the pdump. Once the primary exits, so will the
> pdump.
> 
> Signed-off-by: Suanming Mou <mousuanming@huawei.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> Reviewed-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> V9:
> * reword the comments and update the git log.
> * move release note to release_19_08.rst.
> * remove dot in name.
> 
Acked-by line is missing, so re acking. Keep my ack if you have to send next version.
Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH v9] app/pdump: exit with primary process

[Thomas Monjalon]

20/06/2019 15:32, Pattan, Reshma:
> From: Suanming.Mou [mailto:mousuanming@huawei.com]
> > Signed-off-by: Suanming Mou <mousuanming@huawei.com>
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
> > Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> > Reviewed-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > V9:
> > * reword the comments and update the git log.
> > * move release note to release_19_08.rst.
> > * remove dot in name.
> > 
> Acked-by line is missing, so re acking. Keep my ack if you have to send next version.
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>

That's not correct to add Reviewed-by lines if it was not explicitly
found in a previous reply.
Suanming, I think you need to remove some lines above.

Re: [dpdk-dev] [PATCH v9] app/pdump: exit with primary process

[Suanming Mou]

On 6/24/2019 6:30 AM, Thomas Monjalon wrote:
> 20/06/2019 15:32, Pattan, Reshma:
>> From: Suanming.Mou [mailto:mousuanming@huawei.com]
>>> Signed-off-by: Suanming Mou <mousuanming@huawei.com>
>>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
>>> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
>>> Reviewed-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>> V9:
>>> * reword the comments and update the git log.
>>> * move release note to release_19_08.rst.
>>> * remove dot in name.
>>>
>> Acked-by line is missing, so re acking. Keep my ack if you have to send next version.
>> Acked-by: Reshma Pattan <reshma.pattan@intel.com>
> That's not correct to add Reviewed-by lines if it was not explicitly
> found in a previous reply.
> Suanming, I think you need to remove some lines above.
>
>
>
>
Hi Thomas,

Thanks for the guidelines.
Yes, the tags should be as below since just Anatoly gave the Reviewed-by 
tag and Reshma gave the Acked-by tag in the previous mail.
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Reshma Pattan reshma.pattan@intel.com

Sorry for the mistake and the late response.
(I wish my thunderbird would correctly handle the In-Reply-To header by 
direct click the mailto link.)

BR
SuanmingMou

Re: [dpdk-dev] [PATCH v9] app/pdump: exit with primary process

[Thomas Monjalon]

10/07/2019 16:04, Suanming Mou:
> On 6/24/2019 6:30 AM, Thomas Monjalon wrote:
> > 20/06/2019 15:32, Pattan, Reshma:
> >> From: Suanming.Mou [mailto:mousuanming@huawei.com]
> >>> Signed-off-by: Suanming Mou <mousuanming@huawei.com>
> >>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >>> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
> >>> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> >>> Reviewed-by: Thomas Monjalon <thomas@monjalon.net>
> >>> ---
> >>> V9:
> >>> * reword the comments and update the git log.
> >>> * move release note to release_19_08.rst.
> >>> * remove dot in name.
> >>>
> >> Acked-by line is missing, so re acking. Keep my ack if you have to send next version.
> >> Acked-by: Reshma Pattan <reshma.pattan@intel.com>
> > That's not correct to add Reviewed-by lines if it was not explicitly
> > found in a previous reply.
> > Suanming, I think you need to remove some lines above.
> 
> Thanks for the guidelines.
> Yes, the tags should be as below since just Anatoly gave the Reviewed-by 
> tag and Reshma gave the Acked-by tag in the previous mail.
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Reshma Pattan reshma.pattan@intel.com
> 
> Sorry for the mistake and the late response.
> (I wish my thunderbird would correctly handle the In-Reply-To header by 
> direct click the mailto link.)

Applied, thanks

Re: [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support.

[Burakov, Anatoly]

On 28-Apr-19 5:58 AM, Suanming.Mou wrote:
> When primary app exits, the residual running pdump will stop
> the primary app to restart. Add an exit_with_primary option
> to make pdump exit with primary.
> 
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> ---
>   app/pdump/main.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/app/pdump/main.c b/app/pdump/main.c
> index 3d20854..3909f15 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -26,11 +26,14 @@
>   #include <rte_ring.h>
>   #include <rte_string_fns.h>
>   #include <rte_pdump.h>
> +#include <rte_alarm.h>
>   
>   #define CMD_LINE_OPT_PDUMP "pdump"
>   #define CMD_LINE_OPT_PDUMP_NUM 256
>   #define CMD_LINE_OPT_MULTI "multi"
>   #define CMD_LINE_OPT_MULTI_NUM 257
> +#define CMD_LINE_OPT_EXIT_WP "exit_with_primary"
> +#define CMD_LINE_OPT_EXIT_WP_NUM 258

Unrelated to this patch, but seems very flaky and prone to error. How 
about replacing this stuff with enum-based automatic value assignment, 
like in lib/librte_eal/common/eal_options.h ?

>   #define PDUMP_PORT_ARG "port"
>   #define PDUMP_PCI_ARG "device_id"
>   #define PDUMP_QUEUE_ARG "queue"
> @@ -65,6 +68,7 @@
>   #define SIZE 256
>   #define BURST_SIZE 32
>   #define NUM_VDEVS 2
> +#define MONITOR_INTERVEL (500 * 1000)

I believe it should be INTERVAL

>   
>   /* true if x is a power of 2 */
>   #define POWEROF2(x) ((((x)-1) & (x)) == 0)
> @@ -143,12 +147,14 @@ struct parse_val {
>   static struct rte_eth_conf port_conf_default;
>   static volatile uint8_t quit_signal;
>   static uint8_t multiple_core_capture;
> +static uint8_t exit_with_primary;
>   

<snip>

>   
> @@ -403,6 +410,9 @@ struct parse_val {
>   		case CMD_LINE_OPT_MULTI_NUM:
>   			multiple_core_capture = 1;
>   			break;
> +		case CMD_LINE_OPT_EXIT_WP_NUM:
> +			exit_with_primary = 1;
> +			break;

Any particular reason why it is not made the default?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support.

[Suanming.Mou]

On 2019/4/29 17:14, Burakov, Anatoly wrote:
> On 28-Apr-19 5:58 AM, Suanming.Mou wrote:
>> When primary app exits, the residual running pdump will stop
>> the primary app to restart. Add an exit_with_primary option
>> to make pdump exit with primary.
>>
>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> ---
>>   app/pdump/main.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/app/pdump/main.c b/app/pdump/main.c
>> index 3d20854..3909f15 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -26,11 +26,14 @@
>>   #include <rte_ring.h>
>>   #include <rte_string_fns.h>
>>   #include <rte_pdump.h>
>> +#include <rte_alarm.h>
>>     #define CMD_LINE_OPT_PDUMP "pdump"
>>   #define CMD_LINE_OPT_PDUMP_NUM 256
>>   #define CMD_LINE_OPT_MULTI "multi"
>>   #define CMD_LINE_OPT_MULTI_NUM 257
>> +#define CMD_LINE_OPT_EXIT_WP "exit_with_primary"
>> +#define CMD_LINE_OPT_EXIT_WP_NUM 258
>
> Unrelated to this patch, but seems very flaky and prone to error. How 
> about replacing this stuff with enum-based automatic value assignment, 
> like in lib/librte_eal/common/eal_options.h ?
:)
>
>>   #define PDUMP_PORT_ARG "port"
>>   #define PDUMP_PCI_ARG "device_id"
>>   #define PDUMP_QUEUE_ARG "queue"
>> @@ -65,6 +68,7 @@
>>   #define SIZE 256
>>   #define BURST_SIZE 32
>>   #define NUM_VDEVS 2
>> +#define MONITOR_INTERVEL (500 * 1000)
>
> I believe it should be INTERVAL
Ah, yes, sorry for the typo.
>
>>     /* true if x is a power of 2 */
>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0)
>> @@ -143,12 +147,14 @@ struct parse_val {
>>   static struct rte_eth_conf port_conf_default;
>>   static volatile uint8_t quit_signal;
>>   static uint8_t multiple_core_capture;
>> +static uint8_t exit_with_primary;
>
> <snip>
Could you please help to confirm that the 'snip' here mean we should 
delete the 'exit_with_primary' code?
>
>>   @@ -403,6 +410,9 @@ struct parse_val {
>>           case CMD_LINE_OPT_MULTI_NUM:
>>               multiple_core_capture = 1;
>>               break;
>> +        case CMD_LINE_OPT_EXIT_WP_NUM:
>> +            exit_with_primary = 1;
>> +            break;
>
> Any particular reason why it is not made the default?

It's OK to make it default.  How about Varghese ?

Thank you for the review.

Re: [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support.

[Burakov, Anatoly]

On 29-Apr-19 10:43 AM, Suanming.Mou wrote:
> 

<snip> :)

>>>     /* true if x is a power of 2 */
>>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0)
>>> @@ -143,12 +147,14 @@ struct parse_val {
>>>   static struct rte_eth_conf port_conf_default;
>>>   static volatile uint8_t quit_signal;
>>>   static uint8_t multiple_core_capture;
>>> +static uint8_t exit_with_primary;
>>
>> <snip>
> Could you please help to confirm that the 'snip' here mean we should 
> delete the 'exit_with_primary' code?

No, "snip" means i'm skipping irrelevant sections, just to keep my reply 
shorter and to the point :)

-- 
Thanks,
Anatoly