From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tan, Jianfeng" Subject: Re: [PATCH] eal/ipc: stop async IPC loop on callback request Date: Tue, 10 Apr 2018 23:16:07 +0800 Message-ID: <7305825b-1930-4453-c84e-695645902647@intel.com> References: <80b09df6-9c31-dc91-e26b-8dfde14a2fb0@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: "Burakov, Anatoly" , dev@dpdk.org 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) In-Reply-To: <80b09df6-9c31-dc91-e26b-8dfde14a2fb0@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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