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

[Thomas Monjalon <thomas@monjalon.net>]

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.