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 AC36DA0096 for ; Wed, 8 May 2019 11:38:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7C1DF49DF; Wed, 8 May 2019 11:38:02 +0200 (CEST) Received: from huawei.com (szxga06-in.huawei.com [45.249.212.32]) by dpdk.org (Postfix) with ESMTP id 695E137AF for ; Wed, 8 May 2019 11:38:00 +0200 (CEST) Received: from DGGEMS402-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id CB79BC2DD34D417EAB22; Wed, 8 May 2019 17:37:58 +0800 (CST) Received: from [127.0.0.1] (10.177.131.206) by DGGEMS402-HUB.china.huawei.com (10.3.19.202) with Microsoft SMTP Server id 14.3.439.0; Wed, 8 May 2019 17:37:53 +0800 To: Thomas Monjalon CC: , , References: <1556800505-59917-1-git-send-email-mousuanming@huawei.com> <1556862508-61677-1-git-send-email-mousuanming@huawei.com> <1677555.97jmHZBzRH@xps> From: Suanming.Mou Message-ID: <38d2212b-52ce-b4fd-0ad3-45bb21c389fb@huawei.com> Date: Wed, 8 May 2019 17:37:49 +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: <1677555.97jmHZBzRH@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: <20190508093749.-z1GMfrRYWR9RBxR22yE3goMsO396B6mjS-w1_JQmL8@z> 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. > > 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.") > 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. > > [...] >> + /* >> + * 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. > >> + ret = rte_eal_alarm_cancel(monitor_primary, NULL); >> + if (ret < 0) >> + printf("Fail to disable monitor:%d\n", ret); > [...] >> - /* create mempool, ring and vdevs info */ >> + /* create mempool, ring, vdevs info and primary monitor */ > I don't see any value to this comment. > You may just drop it. That's OK. > >> create_mp_ring_vdev(); >> enable_pdump(); >> + enable_primary_monitor(); > > > 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. Thanks again for the suggestions. BR, Mou