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

[Suanming.Mou <mousuanming@huawei.com>]

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