From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id E11432C2B for ; Wed, 8 May 2019 12:22:34 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 63C7023156; Wed, 8 May 2019 06:22:34 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 08 May 2019 06:22:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=WFngGs6ApGtxUJiI1xIHlf3I1rUHDjlSR8kiNvaJZ90=; b=dvvSL48D4YBi FD9vz03UlMtfXWrf12qeFbR3b5WZ6kLBtCSbml+ejYaTSyasxBBs+ZELT3iJvKxo FxKHliI8dQnY5JjiOtlt4k/vMxTGtyHWETQBtb0qSJ1+k6FafwXJSEwmj6/5Vt7r L90XdIw3Tj2pKMFwi115XSsPi75jxqc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=WFngGs6ApGtxUJiI1xIHlf3I1rUHDjlSR8kiNvaJZ 90=; b=xSj9Wnv1oKvNzO9e8J5LcJMXapY2rEB72OlnwGAiVs7YosdB9DkkCGgJO c4EP64yqVX3ONNzQ8ds7iktVOb9XioSFgNvbUrJ30lHNaKYzM13HBHEQw/cleTCf UYM9H75BCn0MKf+QQJCBgLFBmug+FnFtmUx4ejxs+nUbe+sGG/325gNko0xJ3j9c 2RPhZ21RwgjOlesSwXwy7giBy8nG9jUNbnO2Fgjs8Y6oyG3pdFkiPyEiSOHPD/6T r9QXtOda/NYUILnfD46kmHXP+oLHadCYfy+29hl1s+AXxFkzy060+2Qqt5KznWN6 rJ1jRSN0zsh1XiZ80yLSpE2xuQU0w== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrkeefgddviecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id DDD8F80063; Wed, 8 May 2019 06:22:32 -0400 (EDT) From: Thomas Monjalon To: "Suanming. Mou" Cc: dev@dpdk.org, vipin.varghese@intel.com, anatoly.burakov@intel.com Date: Wed, 08 May 2019 12:22:30 +0200 Message-ID: <2761294.XqQAos14Ep@xps> In-Reply-To: <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> References: <1556800505-59917-1-git-send-email-mousuanming@huawei.com> <1677555.97jmHZBzRH@xps> <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 May 2019 10:22:35 -0000 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 342E8A0096 for ; Wed, 8 May 2019 12:22:36 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id ED8D034F0; Wed, 8 May 2019 12:22:35 +0200 (CEST) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id E11432C2B for ; Wed, 8 May 2019 12:22:34 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 63C7023156; Wed, 8 May 2019 06:22:34 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 08 May 2019 06:22:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=WFngGs6ApGtxUJiI1xIHlf3I1rUHDjlSR8kiNvaJZ90=; b=dvvSL48D4YBi FD9vz03UlMtfXWrf12qeFbR3b5WZ6kLBtCSbml+ejYaTSyasxBBs+ZELT3iJvKxo FxKHliI8dQnY5JjiOtlt4k/vMxTGtyHWETQBtb0qSJ1+k6FafwXJSEwmj6/5Vt7r L90XdIw3Tj2pKMFwi115XSsPi75jxqc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=WFngGs6ApGtxUJiI1xIHlf3I1rUHDjlSR8kiNvaJZ 90=; b=xSj9Wnv1oKvNzO9e8J5LcJMXapY2rEB72OlnwGAiVs7YosdB9DkkCGgJO c4EP64yqVX3ONNzQ8ds7iktVOb9XioSFgNvbUrJ30lHNaKYzM13HBHEQw/cleTCf UYM9H75BCn0MKf+QQJCBgLFBmug+FnFtmUx4ejxs+nUbe+sGG/325gNko0xJ3j9c 2RPhZ21RwgjOlesSwXwy7giBy8nG9jUNbnO2Fgjs8Y6oyG3pdFkiPyEiSOHPD/6T r9QXtOda/NYUILnfD46kmHXP+oLHadCYfy+29hl1s+AXxFkzy060+2Qqt5KznWN6 rJ1jRSN0zsh1XiZ80yLSpE2xuQU0w== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrkeefgddviecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id DDD8F80063; Wed, 8 May 2019 06:22:32 -0400 (EDT) From: Thomas Monjalon To: "Suanming. Mou" Cc: dev@dpdk.org, vipin.varghese@intel.com, anatoly.burakov@intel.com Date: Wed, 08 May 2019 12:22:30 +0200 Message-ID: <2761294.XqQAos14Ep@xps> In-Reply-To: <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> References: <1556800505-59917-1-git-send-email-mousuanming@huawei.com> <1677555.97jmHZBzRH@xps> <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190508102230.1JwFJipjL58g3du2Y3J67WxIEK2OZtnq1E_25F_uGH8@z> 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.