From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f51.google.com (mail-pg0-f51.google.com [74.125.83.51]) by dpdk.org (Postfix) with ESMTP id 2E853376C for ; Mon, 17 Jul 2017 18:21:59 +0200 (CEST) Received: by mail-pg0-f51.google.com with SMTP id v190so24270385pgv.2 for ; Mon, 17 Jul 2017 09:21:59 -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=KnUxC2m3emVu428viJ6q6n27ikm4XDTCUELiHjvA+V8=; b=AYgBj8NBautzMgq315bgY00rpE8VIl2QjNsiPa3sBFJ2bWml5QN2S5vB4OREXO1dA5 JkiiRzXZTBKuq1EFnN1NxtFN5eCHIuvV/I57BwOaL7A2oy+fTmlz/yIfq+RKjomLMqEc we/QczucFPRpLwuBsW50lnt5VFqxpicF8QBQJc3A5k9Grvt8uaXVjJunGaDOTZDEtxIg eX4lc/8uxpE8D1tTTU+/OBAqvnTALk2OThpK/+L+27JkLn2Yhvpsdfo7XCOIH5hSsFUt G3Awp0n1Y686C4QVdYkHElnJSrfc2FfmDeg3MoOnaB/1Sn/UlPv/ddTcfovZDcjT+Gm3 4yBg== 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=KnUxC2m3emVu428viJ6q6n27ikm4XDTCUELiHjvA+V8=; b=c6JkY9X8CqqqfBEm2vG62ebFii72uYvcAjLJNM0/umJGZPmlMFe4A3nCSPdRiWE2A2 rfysmeRMKZLOebAE7fbfwYb7wcshczDVa8pSvWC+BP//4q1QAstnZ7GKeJPeh+UBvkhh 1M1h5fv88CsNc7djTkMBkKPpidaeP6kOiAFw4umLoxyN7/XxPTihn1Bo4/eP7VhwtQCe QrFE29mc/lPqW3PAoxMxYfsvlAaArF8JvJjGFLu26iA8a8IegeHc8mjWtTPemrM0K3ak Qtg70ek2lwHFcpFwh8+C6ixzO+/9NpsQb5p/Hkc3Ve4Qfp/XgGAFx2KJlVfmbesTDg9A QNCA== X-Gm-Message-State: AIVw113FygUi8mWwnkO/K2se38bX86I4jipmzYYe/7SBSr/kAG9dk2Yc /omyXinHCGcs6Sv/+v5khg== X-Received: by 10.84.214.143 with SMTP id j15mr30944130pli.40.1500308518370; Mon, 17 Jul 2017 09:21:58 -0700 (PDT) Received: from xeon-e3 (76-14-207-240.or.wavecable.com. [76.14.207.240]) by smtp.gmail.com with ESMTPSA id o4sm34027807pga.47.2017.07.17.09.21.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 17 Jul 2017 09:21:58 -0700 (PDT) Date: Mon, 17 Jul 2017 09:21:51 -0700 From: Stephen Hemminger To: Andrew Rybchenko Cc: , Stephen Hemminger Message-ID: <20170717092151.507a9ecb@xeon-e3> In-Reply-To: References: <20170714183027.16021-1-stephen@networkplumber.org> <20170714183027.16021-2-stephen@networkplumber.org> <0ce78069-6517-aaf1-cfe9-165192a4cc5f@solarflare.com> <20170717085853.3949de50@xeon-e3> 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 16:21:59 -0000 On Mon, 17 Jul 2017 19:12:01 +0300 Andrew Rybchenko wrote: > On 07/17/2017 06:58 PM, Stephen Hemminger wrote: > > 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. > > Many thanks. May be it would be useful in the comment as well. Maybe one of the original developers could clarify. It would be cleaner just to do rte_atomcic64_set(), it might just be a leftover semantic from Linux/BSD/??? where the original developer was looking. > > >>> + */ > >>> + 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. > > I meant that dev_link value may change after old value saved in original > patch, > but before cmpset which actually replaces dev_link value here. As the result > two _rte_eth_link_update() run in parallel changing to the same value > may return > "changes done", but actually only one did the job. > I'm not sure if it is really important here, since requirements are not > clear. Since there is no locking here. There can not be a guarantee of ordering possible. The only guarantee is that the set of values (duplex, speed, flags) is consistent. I.e one caller wins, the streams don't get crossed.