From: Thomas Monjalon <thomas@monjalon.net>
To: "Suanming. Mou" <mousuanming@huawei.com>
Cc: dev@dpdk.org, vipin.varghese@intel.com, anatoly.burakov@intel.com
Subject: Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support
Date: Wed, 08 May 2019 12:22:30 +0200 [thread overview]
Message-ID: <2761294.XqQAos14Ep@xps> (raw)
Message-ID: <20190508102230.1JwFJipjL58g3du2Y3J67WxIEK2OZtnq1E_25F_uGH8@z> (raw)
In-Reply-To: <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com>
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.
next prev parent reply other threads:[~2019-05-08 10:22 UTC|newest]
Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-25 16:35 [dpdk-dev] [PATCH] app/pdump: exits once primary app exited Suanming.Mou
2019-04-25 15:51 ` Varghese, Vipin
2019-04-25 15:51 ` Varghese, Vipin
2019-04-26 7:11 ` Suanming.Mou
2019-04-26 7:11 ` Suanming.Mou
2019-04-26 10:54 ` Varghese, Vipin
2019-04-26 10:54 ` Varghese, Vipin
2019-04-26 10:56 ` Varghese, Vipin
2019-04-26 10:56 ` Varghese, Vipin
2019-04-26 12:08 ` Suanming.Mou
2019-04-26 12:08 ` Suanming.Mou
2019-04-26 13:46 ` Burakov, Anatoly
2019-04-26 13:46 ` Burakov, Anatoly
2019-04-26 14:32 ` Suanming.Mou
2019-04-26 14:32 ` Suanming.Mou
2019-04-26 14:39 ` Burakov, Anatoly
2019-04-26 14:39 ` Burakov, Anatoly
2019-04-26 14:49 ` Suanming.Mou
2019-04-26 14:49 ` Suanming.Mou
2019-04-26 14:50 ` Burakov, Anatoly
2019-04-26 14:50 ` Burakov, Anatoly
2019-04-25 16:35 ` Suanming.Mou
2019-04-28 4:58 ` [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support Suanming.Mou
2019-04-28 4:58 ` Suanming.Mou
2019-04-28 5:07 ` [dpdk-dev] [PATCH v3] " Suanming.Mou
2019-04-28 5:07 ` Suanming.Mou
2019-04-30 3:39 ` [dpdk-dev] [PATCH v4] app/pdump: add pudmp exits with primary support Suanming.Mou
2019-04-30 2:33 ` Varghese, Vipin
2019-04-30 2:33 ` Varghese, Vipin
2019-04-30 3:43 ` Suanming.Mou
2019-04-30 3:43 ` Suanming.Mou
2019-04-30 5:03 ` Varghese, Vipin
2019-04-30 5:03 ` Varghese, Vipin
2019-04-30 9:34 ` Burakov, Anatoly
2019-04-30 9:34 ` Burakov, Anatoly
2019-04-30 10:37 ` Varghese, Vipin
2019-04-30 10:37 ` Varghese, Vipin
2019-04-30 16:38 ` Burakov, Anatoly
2019-04-30 16:38 ` Burakov, Anatoly
2019-04-30 3:39 ` Suanming.Mou
2019-04-30 11:35 ` [dpdk-dev] [PATCH v5] Make pdump exits with primary Suanming.Mou
2019-04-30 11:35 ` Suanming.Mou
2019-04-30 11:35 ` [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support Suanming.Mou
2019-04-30 9:42 ` Burakov, Anatoly
2019-04-30 9:42 ` Burakov, Anatoly
2019-04-30 11:25 ` Suanming.Mou
2019-04-30 11:25 ` Suanming.Mou
2019-04-30 16:39 ` Burakov, Anatoly
2019-04-30 16:39 ` Burakov, Anatoly
2019-05-02 3:07 ` Suanming.Mou
2019-05-02 3:07 ` Suanming.Mou
2019-04-30 11:35 ` Suanming.Mou
2019-04-30 12:44 ` Pattan, Reshma
2019-04-30 12:44 ` Pattan, Reshma
2019-05-02 5:20 ` [dpdk-dev] [PATCH v6] " Suanming.Mou
2019-05-02 5:20 ` Suanming.Mou
2019-05-02 8:04 ` Varghese, Vipin
2019-05-02 8:04 ` Varghese, Vipin
2019-05-02 8:32 ` Suanming.Mou
2019-05-02 8:32 ` Suanming.Mou
2019-05-02 9:12 ` Burakov, Anatoly
2019-05-02 9:12 ` Burakov, Anatoly
2019-05-02 9:22 ` Varghese, Vipin
2019-05-02 9:22 ` Varghese, Vipin
2019-05-02 9:54 ` Pattan, Reshma
2019-05-02 9:54 ` Pattan, Reshma
2019-05-02 10:40 ` Suanming.Mou
2019-05-02 10:40 ` Suanming.Mou
2019-05-02 12:35 ` [dpdk-dev] [PATCH v7] " Suanming.Mou
2019-05-02 11:03 ` Pattan, Reshma
2019-05-02 11:03 ` Pattan, Reshma
2019-05-02 11:31 ` Burakov, Anatoly
2019-05-02 11:31 ` Burakov, Anatoly
2019-05-02 12:35 ` mousuanming
2019-05-02 12:35 ` mousuanming
2019-05-02 12:35 ` Suanming.Mou
2019-05-03 5:48 ` [dpdk-dev] [PATCH v8] " Suanming.Mou
2019-05-03 5:48 ` Suanming.Mou
2019-05-04 21:17 ` Thomas Monjalon
2019-05-04 21:17 ` Thomas Monjalon
2019-05-05 1:20 ` Suanming.Mou
2019-05-05 1:20 ` Suanming.Mou
2019-05-05 9:42 ` Thomas Monjalon
2019-05-05 9:42 ` Thomas Monjalon
2019-05-05 11:13 ` Suanming.Mou
2019-05-05 11:13 ` Suanming.Mou
2019-05-08 8:04 ` Thomas Monjalon
2019-05-08 8:04 ` Thomas Monjalon
2019-05-08 9:37 ` Suanming.Mou
2019-05-08 9:37 ` Suanming.Mou
2019-05-08 10:22 ` Thomas Monjalon [this message]
2019-05-08 10:22 ` Thomas Monjalon
2019-05-08 13:14 ` Suanming.Mou
2019-05-08 13:14 ` Suanming.Mou
2019-05-15 5:10 ` [dpdk-dev] [PATCH v9] app/pdump: exit with primary process Suanming.Mou
2019-05-15 5:10 ` Suanming.Mou
2019-06-20 12:32 ` Pattan, Reshma
2019-06-23 22:30 ` Thomas Monjalon
2019-07-10 14:04 ` Suanming Mou
2019-07-10 22:28 ` Thomas Monjalon
2019-04-29 9:14 ` [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support Burakov, Anatoly
2019-04-29 9:14 ` Burakov, Anatoly
2019-04-29 9:43 ` Suanming.Mou
2019-04-29 9:43 ` Suanming.Mou
2019-04-29 10:42 ` Burakov, Anatoly
2019-04-29 10:42 ` Burakov, Anatoly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2761294.XqQAos14Ep@xps \
--to=thomas@monjalon.net \
--cc=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=mousuanming@huawei.com \
--cc=vipin.varghese@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).