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