From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E49C8A0C4E; Sat, 23 Oct 2021 10:34:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 68E544069D; Sat, 23 Oct 2021 10:34:54 +0200 (CEST) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by mails.dpdk.org (Postfix) with ESMTP id 486D440041 for ; Sat, 23 Oct 2021 10:34:53 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id DC67E5C0387; Sat, 23 Oct 2021 04:34:52 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Sat, 23 Oct 2021 04:34:52 -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=fm2; bh= 9l3pqjq0OnVD2Ox0480S9Q56lotZw4HnYm6Vu0p9iNk=; b=AoUKr4QF7a0gBI/T 3LGe2l+fRciw4q0UKbXTtHwkxfROoc3MLlHXcSGMR+5/gp72rk4m1NpE2udxPAjf tf4gFpXzvq9zRfdM1LlVeu/6HvBXzwjOuxJXBOmP67SRcVG+tJgdp3MW6bR7XmRy GxezShn7MrCP0SmCgQtn5PPfJkp1h4wAeQD8C9QyyrFy08Ifz/GFxghwM2OBoCWl Yq4iXYG0VZvKWhNnmallkaxUbCtmHtTdWmRRDDyD/66raNI9u9URt3CId+P+hDdt ryljLvdrUHAugN+UXkUy6z4tl+6FQB3UDAjPNCd6HyJiv41E6OrxyGE0rP36uVwI jf0Ydw== 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=9l3pqjq0OnVD2Ox0480S9Q56lotZw4HnYm6Vu0p9i Nk=; b=ERXAeN4PA46xNXGYYI9HV3zgjkAEs309gDWsteYLz88zPrn+C3rf3f7vs VVdgiaiacXbIPYHJEE47/gjU+nOra0c3LzOlZc5u2InmqvYnJFJy3ScS0lX722Ad HRrYwdQUWZ1jYdFIqw3/yq6r8rssCuR/LXpzcM9t07Wly60F1BFLmygDfg1N9Ln1 j+4VgAQHBsVC1r4KB3WvJI3w0er+xSl8st3VH6Lj33YUeBLTdaEwqAtOPMKGKi/0 qXEma15vkKK70SH1ycwonAUroJh4w4w42utHqLvmsNuxyCBqJn67Qv6GsFtcP+ix yp30aYiap0+317SpiLpWH4apaXyMQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvdeftddgtdehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 23 Oct 2021 04:34:51 -0400 (EDT) From: Thomas Monjalon To: Bing Zhao Cc: ferruh.yigit@intel.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org, konstantin.ananyev@intel.com Date: Sat, 23 Oct 2021 10:34:49 +0200 Message-ID: <3885639.MmVk1qbWW8@thomas> In-Reply-To: <20211022211407.315068-2-bingz@nvidia.com> References: <20211022211407.315068-1-bingz@nvidia.com> <20211022211407.315068-2-bingz@nvidia.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 2/2] ethdev: fix the race condition for fp ops reset X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 22/10/2021 23:14, Bing Zhao: > In the function "eth_dev_fp_ops_reset", a structure assignment > operation is used to reset one queue's callback functions, etc., but > it is not thread safe. > > The structure assignment is not atomic, a lot of instructions will > be generated. Right now, since not all the fields are needed, the > fields in the "dummy_ops" which is not set explicitly will be 0s > based on the specification and compiler behavior. In order to make > "fpo" has the same content with "dummy_ops", some clearing to 0 > operation is needed. > > By checking the object instructions (e.g. with GCC 4.8.5) > 0x0000000000a58317 <+35>: mov %rsi,%rdi > 0x0000000000a5831a <+38>: mov %rdx,%rcx > => 0x0000000000a5831d <+41>: rep stos %rax,%es:(%rdi) > 0x0000000000a58320 <+44>: mov -0x38(%rsp),%rax > 0x0000000000a58325 <+49>: lea -0xe0(%rip),%rdx > // # 0xa5824c > > It shows that "rep stos" will clear the "fpo" structure before > assigning new values. > > In the other thread, if some data path Tx / Rx functions are still > running, there is a risk to get 0 instead of the correct dummy > content. > 1. qd = p->rxq.data[queue_id] > 2. (void **)&p->rxq.clbk[queue_id] > "data" and "clbk" may be observed with NULL (0) in other threads. > Even it is temporary, the accessing to a NULL pointer will cause a > crash. Using "memcpy" could get rid of this. > > Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure") > Cc: konstantin.ananyev@intel.com > > Signed-off-by: Bing Zhao > --- > --- a/lib/ethdev/ethdev_private.c > +++ b/lib/ethdev/ethdev_private.c > @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo) > .txq = {.data = dummy_data, .clbk = dummy_data,}, > }; > > - *fpo = dummy_ops; > + rte_memcpy(fpo, &dummy_ops, sizeof(struct rte_eth_fp_ops)); That's not trivial. Please add a comment to briefly explain that memcpy avoids zeroing of a simple assignment.