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 9FF72A0096 for ; Wed, 8 May 2019 15:14:36 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 68E8D34F0; Wed, 8 May 2019 15:14:35 +0200 (CEST) Received: from huawei.com (szxga04-in.huawei.com [45.249.212.190]) by dpdk.org (Postfix) with ESMTP id E4E7C2C2B for ; Wed, 8 May 2019 15:14:33 +0200 (CEST) Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 0DE0A3759FB746B80D20; Wed, 8 May 2019 21:14:32 +0800 (CST) Received: from [127.0.0.1] (10.177.131.206) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.439.0; Wed, 8 May 2019 21:14:27 +0800 To: Thomas Monjalon CC: , , References: <1556800505-59917-1-git-send-email-mousuanming@huawei.com> <1677555.97jmHZBzRH@xps> <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> <2761294.XqQAos14Ep@xps> From: Suanming.Mou Message-ID: Date: Wed, 8 May 2019 21:14:20 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <2761294.XqQAos14Ep@xps> Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US X-Originating-IP: [10.177.131.206] X-CFilter-Loop: Reflected 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: <20190508131420.yfB1sxE1EZzwxfSFUUj8q8eTVIqUZNUOsdcP1phIyQY@z> 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: >>> 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. 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 >> exit. > Let's write it in a simpler descriptive form: > /* Maximum delay for exiting after primary process. */ Got it. > >>> [...] >>>> + /* >>>> + * 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 emotional. >>>> + 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. > > > > . >