From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <mousuanming@huawei.com>
Received: from huawei.com (szxga04-in.huawei.com [45.249.212.190])
 by dpdk.org (Postfix) with ESMTP id E4E7C2C2B
 for <dev@dpdk.org>; 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 <thomas@monjalon.net>
CC: <dev@dpdk.org>, <vipin.varghese@intel.com>, <anatoly.burakov@intel.com>
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 <mousuanming@huawei.com>
Message-ID: <d11450be-49d3-bad9-cf93-b00f8913b592@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 08 May 2019 13:14:34 -0000


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.
>
>
>
> .
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by dpdk.space (Postfix) with ESMTP id 9FF72A0096
	for <public@inbox.dpdk.org>; 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 <dev@dpdk.org>; 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 <thomas@monjalon.net>
CC: <dev@dpdk.org>, <vipin.varghese@intel.com>, <anatoly.burakov@intel.com>
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 <mousuanming@huawei.com>
Message-ID: <d11450be-49d3-bad9-cf93-b00f8913b592@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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.
>
>
>
> .
>