From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id D5F5E44BE for ; Thu, 25 Oct 2018 01:51:32 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 500D322165; Wed, 24 Oct 2018 19:51:32 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 24 Oct 2018 19:51:32 -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=ziEODcJYEJJTGD+naoFbDl6VVusF2LdH8ZTnCYH0MJI=; b=qdxFiL+HV73N R/gDCcKBlYBD61sh/wlvghgBPwEUupGuN97eU42vkXE3CVULk7p7yL+HTnxToDCC S57BRRCMExNTnoEOKJADBTc2fIFqBc4PM6Ije5TEConQTq6buysIbGmept9D3T6l p5cQ4fHx0lRfRuq2cTXuMpQB1BxxGTg= 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=fm1; bh=ziEODcJYEJJTGD+naoFbDl6VVusF2LdH8ZTnCYH0M JI=; b=MqVaCdAOW+O7wFspobiQh7Fw6k+NTpGQNEF1L1jsUMj1mSMX+ia87OanC ExG1IzyTa5KQ5QuQaVS1Y+B9+KbqEGbSkAZSLgr5ZNyo2Qc/rPz3WE6yLySJQW6I /APeEsAEEE0XCFpAt6pFsgr4pMZS2oXBIiktbceC/Mswy2jQqRx7NO0mrkP5ACa8 XhQR+2015/TTbClrndFG5skfNCZwZxgoV/t8x7eUyc7CCdToox2l1zQ60ZI7I67h CjdRSI0GARdLoQFYncv35O3tdQYNnx2MQ7gd4AP+o8xEstGKEmluqrdJjVHF0HzW Kn8u3WlxZUkdH2Hrzf//OX7vfJFKg== X-ME-Sender: 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 63434102E9; Wed, 24 Oct 2018 19:51:31 -0400 (EDT) From: Thomas Monjalon To: "Burakov, Anatoly" , Stephen Hemminger Cc: dev@dpdk.org, Stephen Hemminger Date: Thu, 25 Oct 2018 01:51:34 +0200 Message-ID: <1682249.U5QVuGPMnJ@xps> In-Reply-To: References: <20180725182019.31518-1-stephen@networkplumber.org> <1622402.chDL49Ktjv@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Oct 2018 23:51:33 -0000 18/09/2018 12:16, Burakov, Anatoly: > On 18-Sep-18 10:43 AM, Thomas Monjalon wrote: > > 26/07/2018 11:41, Burakov, Anatoly: > >> 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 > >>> --- > >>> --- 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. > > > > What should be done here? OK to remove rte_panic for now? > > > > As i said, the above fix is wrong because it leaks memory (however > unlikely it may be). > > The alarm set call should be moved to before we do send_msg() call (and > goto fail; on failure). That way, even if alarm triggers too early (i.e. > immediately), the requests tailq will still be locked until we complete > our request sends - so we appropriately free memory on response, on > timeout or in our failure handler if alarm set has failed. Someone to fix it, please?