[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dpdk-dev
Subject:    Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.
From:       "Burakov, Anatoly" <anatoly.burakov () intel ! com>
Date:       2019-04-30 16:39:14
Message-ID: 491c12ba-2cd8-e1bd-6622-0549439ecbc8 () intel ! com
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]