From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <anatoly.burakov@intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id 4A624324B
 for <dev@dpdk.org>; Thu, 26 Jul 2018 11:42:46 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga002.jf.intel.com ([10.7.209.21])
 by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 26 Jul 2018 02:42:43 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.51,404,1526367600"; d="scan'208";a="77978153"
Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.86])
 ([10.237.220.86])
 by orsmga002.jf.intel.com with ESMTP; 26 Jul 2018 02:41:25 -0700
To: Stephen Hemminger <stephen@networkplumber.org>, dev@dpdk.org
Cc: Stephen Hemminger <sthemmin@microsoft.com>
References: <20180725182019.31518-1-stephen@networkplumber.org>
 <20180725182019.31518-4-stephen@networkplumber.org>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <578857d9-e544-f1ce-bad2-a79fa6d78f94@intel.com>
Date: Thu, 26 Jul 2018 10:41:24 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180725182019.31518-4-stephen@networkplumber.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH 3/4] eal: don't crash if alarm set fails
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: Thu, 26 Jul 2018 09:42:46 -0000

On 25-Jul-18 7:20 PM, Stephen Hemminger wrote:
> There is no need to call rte_exit and crash the application here;
> better to let the application handle the error itself.
> 
> Remove the gratuitous profanity which would be visible if
> the rte_exit was still there.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index 9fcb9121908d..07b7579c565a 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -841,14 +841,12 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
>   
>   	param->user_reply.nb_sent++;
>   
> -	if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
> -			      async_reply_handle, pending_req) < 0) {
> +	ret = rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
> +				async_reply_handle, pending_req);
> +	if (ret < 0)
>   		RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n",
>   			dst, req->name);
> -		rte_panic("Fix the above shit to properly free all memory\n");

Profanity aside, i think the message was trying to tell me something - 
namely, that if alarm_set fails, we're risking to leak this memory if 
reply from the peer never comes, and we're risking leaving the 
application hanging because the timeout never triggers. I'm not sure if 
leaving this "to the user" is the right choice, because there is no way 
for the user to free IPC-internal memory if it leaks.

So i think the proper way to handle this would've been to set the alarm 
first, then, if it fails, don't sent the message in the first place.

-- 
Thanks,
Anatoly