[dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support
mousuanming at huawei.com
Wed May 8 15:14:20 CEST 2019
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:
>>> 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
> 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).
Great, it becomes more official now. The original one seems little
>>>> + 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
> Indeed, "process" is more correct in this context.
>> Thanks again for the suggestions.
> You're welcome.
More information about the dev