From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f182.google.com (mail-wr0-f182.google.com [209.85.128.182]) by dpdk.org (Postfix) with ESMTP id C1864532C for ; Fri, 30 Jun 2017 14:24:50 +0200 (CEST) Received: by mail-wr0-f182.google.com with SMTP id c11so203599586wrc.3 for ; Fri, 30 Jun 2017 05:24:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=x2KQ66lgQ3PcOUVzCtGGgKNr3+6CIK5zm9EtiabnUwo=; b=t44UH/bHdq5xoKax1n2BwqghUC17Ih3qfE9XKvbwMKocoMjj6fyimeTQh3fCKlAoPu RvOLTqAyXY0yuYXITjNplS+b1ve8NK8iVzl4TjWoE353pOQb6G/k+4uM0Y6++MqvPZHw 87w/FIGoYjwm4+Z9MUVaZUJVDziho9Sar42DNxeh2POf9LRyqw8ycqhAOHZ8FrMttPwb I3qptMifmqki+Xsl3uPSaw8RbdnQEOaiyZoylb0nCJ3Eh1tg11PB8i4pd9/FHgFy83cr vcnAdCj/0dDDkWBxU309RiqcRwhe/h/0Pbyl0UQ9ZxHNMARuQVoe4Y1e9ZaVgUYYUuAE wGhQ== 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=x2KQ66lgQ3PcOUVzCtGGgKNr3+6CIK5zm9EtiabnUwo=; b=Tl5Nh8nVDkg7bAmWPCS6U5AicdSw7t8XPMSvawQwAbwtUS+W977+A6K/IPMNRD0UTk GtVvzpkPhSYKYcT5U7FArX0HkLKEpZFbCx25hhlJ8BCvvE3Bkwy4FCP6kUPLwtaVgjbf cU0Ls41RP3YXRGs2VSA0Y2eSlULsRi3jclKOi5G24/znC4Blw3kFUNOP49CpYgmx4y9N 9ZNiUfqHz9UbUigc3I6MLevASLq+4G7XzsX++vr6Wt/noGIi/pKY3Rchfepp5/SLK55e ZGuYTsqXIn+8H5cXa8dbl3Jngwy8AwXraGmWhAKVfgOAYKtN3bLdISJE/yYgXxJhUbr+ y4VA== X-Gm-Message-State: AKS2vOxgZR+6CZPzMt/iz6hxZXZ8v284dB07zcUbMGKz/gcMfd9QA54R uA1UWYNj7U/AYhuX/qU= X-Received: by 10.223.182.172 with SMTP id j44mr20559324wre.122.1498825490358; Fri, 30 Jun 2017 05:24:50 -0700 (PDT) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id s65sm2076180wmb.7.2017.06.30.05.24.41 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 30 Jun 2017 05:24:41 -0700 (PDT) Date: Fri, 30 Jun 2017 14:24:38 +0200 From: Olivier Matz To: Bruce Richardson Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com Message-ID: <20170630142438.293c8e06@platinum> In-Reply-To: <20170630113227.GG14776@bricha3-MOBL3.ger.corp.intel.com> References: <20170607133620.275801-1-bruce.richardson@intel.com> <20170607133620.275801-2-bruce.richardson@intel.com> <20170630114046.5c6eb8bb@platinum> <20170630113227.GG14776@bricha3-MOBL3.ger.corp.intel.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/5] ring: allow rings with non power-of-2 sizes 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: Fri, 30 Jun 2017 12:24:51 -0000 On Fri, 30 Jun 2017 12:32:27 +0100, Bruce Richardson wrote: > On Fri, Jun 30, 2017 at 11:40:46AM +0200, Olivier Matz wrote: > > Hi Bruce, > > > > Few comments inline. > > > > On Wed, 7 Jun 2017 14:36:16 +0100, Bruce Richardson wrote: > > > The rte_rings traditionally have only supported having ring sizes as powers > > > of 2, with the actual usable space being the size - 1. In some cases, for > > > example, with an eventdev where we want to precisely control queue depths > > > for latency, we need to allow ring sizes which are not powers of two so we > > > add in an additional ring capacity value to allow that. For existing rings, > > > this value will be size-1, i.e. the same as the mask, but if the new > > > EXACT_SZ flag is passed on ring creation, the ring will have exactly the > > > usable space requested, although the underlying memory size may be bigger. > > > > > > Signed-off-by: Bruce Richardson > > > > Specifying the exact wanted size looks to be a better API than the current > > one, which is to give the power of two above the wanted value, knowing there > > will be only size-1 elements available. > > > > What would you think about deprecating ring init/creation without this > > flag in a few versions? Then, later, we could remove this flag and > > the new behavior would become the default. > > > > I'm not really sure it's necessary. For the vast majority of cases what > we have now is fine, and it ensures that we maximise the ring space. I'd > tend to keep the new behaviour for those cases where we really need it. > > It's a trade off between what we hide and expose: > * current scheme hides the fact that you get one entry less than you ask > for, but the memory space is as expected. > * new scheme gives you exactly the entries you ask for, but hides the > fact that you could be using up to double the memory space for the > ring. Yes, having 2 different behavior is precisely what I don't really like. Giving the number of entries the user asks for is straightforward, which is a good thing for an API. And hiding the extra consumed memory looks acceptable, this can be documented. That said, if we decide to deprecate it, there's no need to do it right now. > > > @@ -845,69 +857,63 @@ rte_ring_dequeue(struct rte_ring *r, void **obj_p) > > > } > > > > > > /** > > > - * Test if a ring is full. > > > + * Return the number of entries in a ring. > > > * > > > * @param r > > > * A pointer to the ring structure. > > > * @return > > > - * - 1: The ring is full. > > > - * - 0: The ring is not full. > > > + * The number of entries in the ring. > > > */ > > > -static inline int > > > -rte_ring_full(const struct rte_ring *r) > > > +static inline unsigned > > > +rte_ring_count(const struct rte_ring *r) > > > { > > > uint32_t prod_tail = r->prod.tail; > > > uint32_t cons_tail = r->cons.tail; > > > - return ((cons_tail - prod_tail - 1) & r->mask) == 0; > > > + return (prod_tail - cons_tail) & r->mask; > > > } > > > > When used on a mc/mp ring, this function can now return a value > > which is higher than the ring capacity. Even if this function is > > not really accurate when used while the ring is use, I think we > > should maximize the result with r->capacity. > > > > This will also avoid rte_ring_free_count() to return a overflowed > > value. > > > > How does it return an overflowing value? I think in the MP/MC case the > tests made each time around the loop for cmpset should ensure we never > overflow. Here we are reading r->prod.tail and r->cons.tail without synchronization, nor memory barriers. So basically, there is no guarantee that the values are consistent. Before, the returned value could be wrong, but always in the interval [0, mask]. Now, rte_ring_count() is still in the interval [0, mask], but the capacity of the ring is lower than mask. If rte_ring_count() returns mask, it would cause rte_ring_free_count() to return a value lower than 0 (overflowed since the result is unsigned). > > > @@ -916,7 +922,9 @@ rte_ring_free_count(const struct rte_ring *r) > > > * @param r > > > * A pointer to the ring structure. > > > * @return > > > - * The number of elements which can be stored in the ring. > > > + * The size of the data store used by the ring. > > > + * NOTE: this is not the same as the usable space in the ring. To query that > > > + * use ``rte_ring_get_capacity()``. > > > */ > > > static inline unsigned int > > > rte_ring_get_size(const struct rte_ring *r) > > > @@ -925,6 +933,20 @@ rte_ring_get_size(const struct rte_ring *r) > > > } > > > > > > /** > > > + * Return the number of elements which can be stored in the ring. > > > + * > > > + * @param r > > > + * A pointer to the ring structure. > > > + * @return > > > + * The usable size of the ring. > > > + */ > > > +static inline unsigned int > > > +rte_ring_get_capacity(const struct rte_ring *r) > > > +{ > > > + return r->capacity; > > > +} > > > + > > > +/** > > > * Dump the status of all rings on the console > > > * > > > * @param f > > > > I think the users of rte_ring_get_size() use this API to get the > > number of elements that can fit in the ring. The patch changes the > > semantic of this function (even if the result is the same when > > EXACT_SZ is not passed). This is the same for ring->size. > > > > Wouldn't it be better to rename all current occurences of "size" as > > "storage_size", and introduce a new "size", which is what you call > > "capacity". > > > > I'm asking the question but I'm not that convinced myself... > > nevertheless I find storage_size/size less confusing than > > size/capacity. > > > I went back and forward on this myself a bit too. However, the reason I > went for this approach is purely one of backwards compatibility. For > legacy apps unaware of the new apps, there is no change in behaviour or > return value for the size function. For apps that are aware of this > change there is the new function provided. > If we did change size to be storage size, then you could end up getting > off-by-one errors in apps that haven't been updated. OK. Olivier