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 C6FBB42CFC; Mon, 19 Jun 2023 18:27:46 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 513D240E0F; Mon, 19 Jun 2023 18:27:46 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id D796F40DF8 for ; Mon, 19 Jun 2023 18:27:44 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 2FA57210DD9F; Mon, 19 Jun 2023 09:27:44 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2FA57210DD9F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1687192064; bh=UHk8Wu7/Jx+ToTBLzEL5yHt5aYdik/fxUqcEXXKUD6Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gVPpa8lcqGcMClHaAkNWOsD1XjpmKviCS5+76y1KTmcnaSoTb0Np3pLQFDP1zHenj dGxsdpSgBopCfEVHeRYphhI92VDNu8crYLACTrksxJ2vQ2n9UoA4mwIyEufUi5PQHj FJLz42kGDUe0c109qRBOGSJ4TRU1cE7QCjJvGqhg= Date: Mon, 19 Jun 2023 09:27:44 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: David Marchand , thomas@monjalon.net, dev@dpdk.org, Qiming Yang , Wenjun Wu Subject: Re: [PATCH] net/ixgbe: fix inverted test and set conditional Message-ID: <20230619162744.GA18752@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1686857402-29916-1-git-send-email-roretzla@linux.microsoft.com> <1686857402-29916-2-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D879E9@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D879E9@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Mon, Jun 19, 2023 at 10:19:00AM +0200, Morten Brørup wrote: > > From: David Marchand [mailto:david.marchand@redhat.com] > > Sent: Monday, 19 June 2023 09.54 > > > > On Thu, Jun 15, 2023 at 9:30 PM Tyler Retzlaff > > wrote: > > > > > > Correct a mistake when converting ixgbe to use __atomic_test_and_set > > > instead of rte_atomic32_test_and_set. The return value from > > > __atomic_test_and_set is inverted relative to rte_atomic32_test_and_set. > > > > > > Fixes: e90baf6b82f6 ("net/ixgbe: replace legacy atomics with GCC builtin > > atomics") > > > > > > Signed-off-by: Tyler Retzlaff > > Reviewed-by: David Marchand > > > > Thanks for the fix Tyler. > > rte_atomic32_test_and_set() returns the inverse of __atomic_test_and_set()! > > We need to provide a prominent warning about this when deprecating rte_atomic32_test_and_set(), or everyone else will make the same mistake as Tyler, i.e. perform a simple search/replace with the new function name, not noticing that the function's return value is inverted. > Perhaps we can also add some build time warning when using rte_atomic32_test_and_set()? i'll probably take care of this work myself after stdatomics (assuming it is something people want) and hopefully i won't make that mistake again. for now i don't want to confuse the dev list with too many patches about 'atomics' to avoid overload/confusion for david and thomas. > > PS: > Tyler, the Solarflare driver also uses rte_atomic32_test_and_set(); maybe you missed in the original patch series. it was intentionally skipped (for now). i only did the conversion for drivers that are built on windows. the assumption being the rte_atomicXX api deprecation is independent of switching to C11 stdatomic. i don't intend to ever make available rte_atomicXX on msvc/windows combination since it is being deprecated. as mentioned above as i expand the code that is built for msvc/windows it will then force me to adapt more of the rte_atomicsXX code eventually leading to most of it being gone (at least for libraries). > https://elixir.bootlin.com/dpdk/v23.07-rc1/A/ident/rte_atomic32_test_and_set >