From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f47.google.com (mail-pg0-f47.google.com [74.125.83.47]) by dpdk.org (Postfix) with ESMTP id 9B3D947CD for ; Mon, 17 Jul 2017 17:59:02 +0200 (CEST) Received: by mail-pg0-f47.google.com with SMTP id 123so8987500pgj.1 for ; Mon, 17 Jul 2017 08:59:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=878PtyN0z/4SB+Sp7o0hlPXeqwo1svA1MuvcT/PWplU=; b=M8jS1tIvrKCPxOYIhkIhL3b18aGrKIGCoEwzPEBS9LzkKLHOZNx0U/v5cXkZnWUxUD LDlDZImc8unW+Y1R+JGjBAkzuSwnCidz9VIMivFaLJp7em0M8yqEOmOTqokhZiKHwApZ mMrpXls8kzci9rrlqHiYHroWg7bU/Je7z3fBOSunR4Ab9wMsU/+NkT5NY7ImGaC9XCjH d4NtQ0D69+y/56c2ZFC+FvhTdgfp5ZQGM6ucTIiDLF3znDpdofXXEjtCztLabfFOa49N 0IoQActktdGYbcGWdqBsf429+glPXSrgTI0XWnM5cLCQC6P2PyqXJF1g0/xNIkxM7yC4 7K7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=878PtyN0z/4SB+Sp7o0hlPXeqwo1svA1MuvcT/PWplU=; b=rAOfWPvAyXOG/QnSAJJjdYsHvQVpBb8vbETJEoPQH7Ip2fPhxAeFAXDOieXwgAadvD LpB8ahtlF4NPBhhRNhHF7rouzaVR255FMrbG0hLP6rNu6YTn+BPvIkJoLSYhfjAwncjF YTEUsvLuoUsFdaEGVF+chHCKWVfsJehaVTnp74T2YXOWRBNLbMgjTuh55UJuRu3DsoO1 WeudG29I4MyadStvUW4Qcpy0Ja77oiv35C99CNrAlCJ6GOVmtfhBAcc+m6egpH8FH2VV hE8VGgt8UpkECgLkBZ33kodDDScBQj7v6jQgOM8qvXwVfIhYr5zkYJ8KLBg1PwlhcD64 Cl/A== X-Gm-Message-State: AIVw1111FPSQxs0gIHFg6RJ9oXIFuTzleqBi2nqEdekLZjcI8HT6WCAm rCBS2gYCCp1RyfY2drLz3A== X-Received: by 10.99.171.1 with SMTP id p1mr10900430pgf.140.1500307141488; Mon, 17 Jul 2017 08:59:01 -0700 (PDT) Received: from xeon-e3 (76-14-207-240.or.wavecable.com. [76.14.207.240]) by smtp.gmail.com with ESMTPSA id f10sm32698916pgu.54.2017.07.17.08.59.01 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 17 Jul 2017 08:59:01 -0700 (PDT) Date: Mon, 17 Jul 2017 08:58:53 -0700 From: Stephen Hemminger To: Andrew Rybchenko Cc: , Stephen Hemminger Message-ID: <20170717085853.3949de50@xeon-e3> In-Reply-To: <0ce78069-6517-aaf1-cfe9-165192a4cc5f@solarflare.com> References: <20170714183027.16021-1-stephen@networkplumber.org> <20170714183027.16021-2-stephen@networkplumber.org> <0ce78069-6517-aaf1-cfe9-165192a4cc5f@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions 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: Mon, 17 Jul 2017 15:59:02 -0000 On Sun, 16 Jul 2017 16:26:06 +0300 Andrew Rybchenko wrote: > On 07/14/2017 09:30 PM, Stephen Hemminger wrote: > > Many drivers are all doing copy/paste of the same code to atomicly > > update the link status. Reduce duplication, and allow for future > > changes by having common function for this. > > > > Signed-off-by: Stephen Hemminger > > --- > > lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++ > > lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 64 insertions(+) > > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > > index a1b744704f3a..7532fc6b65f0 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link) > > } > > > > int > > +_rte_eth_link_update(struct rte_eth_dev *dev, > > + const struct rte_eth_link *link) > > +{ > > + volatile struct rte_eth_link *dev_link = &(dev->data->dev_link); > > + struct rte_eth_link old; > > + > > + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t)); > > + > > + old = *dev_link; > > + > > + /* Only reason we use cmpset rather than set is > > + * that on some architecture may use sign bit as a flag value. > > May I ask to provide more details here. rte_atomic64_set() takes an int64 argument. This code (taken from ixgbe, virtio and other drivers) uses cmpset to allow using uint64_t. My assumption is that some architecture in the past was using the sign bit a a lock value or something. On 64 bit no special support for 64bit atomic assignment is necessary. Not sure how this code got inherited that way. > > > + */ > > + while (rte_atomic64_cmpset((volatile uint64_t *)dev_link, > > + *(volatile uint64_t *)dev_link, > > + *(const uint64_t *)link) == 0) > > Shouldn't it be: > do { > old = *dev_link; > } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link, > *(uint64_t *)&old, *(const uint64_t *)link) == 0); > > At least it has some sense to guarantee transition from old to new > talking below comparison into account. Since dev_link is volatile, the compiler is required to refetch the pointer every time it evaluates the expression. Maybe clearer to alias devlink to a volatile uint64_t ptr.