From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 5E0D51DB8 for ; Tue, 10 Apr 2018 17:16:10 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Apr 2018 08:16:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,432,1517904000"; d="scan'208";a="32282086" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.255.30.205]) ([10.255.30.205]) by orsmga007.jf.intel.com with ESMTP; 10 Apr 2018 08:16:08 -0700 To: "Burakov, Anatoly" , dev@dpdk.org References: <80b09df6-9c31-dc91-e26b-8dfde14a2fb0@intel.com> From: "Tan, Jianfeng" Message-ID: <7305825b-1930-4453-c84e-695645902647@intel.com> Date: Tue, 10 Apr 2018 23:16:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <80b09df6-9c31-dc91-e26b-8dfde14a2fb0@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] eal/ipc: stop async IPC loop on callback request 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: Tue, 10 Apr 2018 15:16:10 -0000 On 4/10/2018 10:17 PM, Burakov, Anatoly wrote: > On 10-Apr-18 2:53 PM, Tan, Jianfeng wrote: >> >> >> On 4/10/2018 6:03 PM, Anatoly Burakov wrote: >>> EAL did not stop processing further asynchronous requests on >>> encountering a request that should trigger the callback. This >>> resulted in erasing valid requests but not triggering them. >> >> That means one wakeup could process multiple replies, and following >> process_async_request() will erase some valid requests? > > Yes. > >> >>> >>> Fix this by stopping the loop once we have a request that we >>> can trigger. Also, remove unnecessary check for trigger >>> request being NULL. >>> >>> Fixes: f05e26051c15 ("eal: add IPC asynchronous request") >>> Cc: anatoly.burakov@intel.com >>> >>> Signed-off-by: Anatoly Burakov >> Acked-by: Jianfeng Tan >> >>> --- >>> lib/librte_eal/common/eal_common_proc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/librte_eal/common/eal_common_proc.c >>> b/lib/librte_eal/common/eal_common_proc.c >>> index f98622f..1ea3b58 100644 >>> --- a/lib/librte_eal/common/eal_common_proc.c >>> +++ b/lib/librte_eal/common/eal_common_proc.c >>> @@ -510,11 +510,11 @@ async_reply_handle(void *arg __rte_unused) >>> TAILQ_REMOVE(&pending_requests.requests, >>> sr, next); >>> free(sr); >>> - } else if (action == ACTION_TRIGGER && >>> - trigger == NULL) { >>> + } else if (action == ACTION_TRIGGER) { >>> TAILQ_REMOVE(&pending_requests.requests, >>> sr, next); >>> trigger = sr; >>> + break; >> >> If I understand it correctly above, break here, we will trigger an >> async action, and then go back to sleep with some ready requests not >> handled? Seems that we shall unlock, process, and lock here. Right? > > Well, we won't go back to sleep - we'll just loop around and come back. > > See eal_common_proc.c:472: > > /* sometimes, we don't even wait */ > if (sr->reply_received) { > nowait = true; > break; > } > > Followed by line 478: > > if (nowait) > ret = 0; > > Followed by line 495: > > if (ret == 0 || ret == ETIMEDOUT) { > > So, having messages with replies already in the queue will cause wait > to be cancelled. It's not much slower than unlocking, triggering, and > locking again. Ah, sorry, I overlooked that fact that every iteration we re-scan the request list. > > However, if you're OK with > lock-loop-unlock-trigger-lock-loop-unlock-... sequence until we run > out of triggers, then sure, i can add that. Don't see the reason for that. Thanks, Jianfeng