From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id 5F9DD11D9 for ; Tue, 31 Jan 2017 14:27:24 +0100 (CET) Received: by mail-wm0-f54.google.com with SMTP id v77so93414824wmv.0 for ; Tue, 31 Jan 2017 05:27:24 -0800 (PST) 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=ggcafIpA4cBh07GTHcWEerlUlI1Q+p9ZQgG04hrnfLE=; b=jMxLSVwamnjbCdOy9mPm8Xv/gpNmqotC1c0Rt2q5Vp34P7ZkrC9y2xV9jbJYcIouIb BPVlsbjh8pcktEcSgYmsMbvyyzGi3BS36OXycc16VZIhPjn2NnytK31WNDqddHSxWcxI ghI5BlodpYIpv+Rfukq7Bv1QuikySlcOOQsSymfwlVks+U3wxOwrtnlsSkcYE8lqOUuv lOWyGI5YcMhMDSaM43NvfolUqc0F4Xb0lYO+fRNyzXhZyihO05Ems02vnkMNVjn0rSuX LMvml0YalQ9qsjP3r4YOIozZtWCJ5k+yitxSeFok/P51DCqQMo5iM29R/p3T9wRkE8KO 1ikw== 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=ggcafIpA4cBh07GTHcWEerlUlI1Q+p9ZQgG04hrnfLE=; b=rnzn4lVgeNro0280u8tbwksRxPT9jF3amPRwde5mRbOiwSHP+inKo1fw3aVWMJ7n1H ytCaP8YNkw5URWCSTsB1fkEkIPw8mBz0rd8FxZQ4COBvET0n8a30Rl6VH6NYZjwLmHHl RYnBP/O/OgOOq+G9Z/UKbTagk89JJsZBZ22NJFHlP2swjJ8Aufcx7mW5QZi92c1UYNBU in2gV80O3xLi6XtxktKGQXIm48JtOgx66Fy787XZktktnJXn1HKxP5N2CrtjCOnHAXXe tMZTsUULw2OKgJ4VVuTkCtIPYu8JC7mYfHWfem+Q3VuxnoXQYOIWAIq9iilbwlxNrrNj dvxA== X-Gm-Message-State: AIkVDXLnrPToepSp8d/Rwz6oz257OuN9hrTWGiEpq3diQUBR02Je2kPhA4PU37HpQkYUOtna X-Received: by 10.223.139.2 with SMTP id n2mr13442938wra.67.1485869242980; Tue, 31 Jan 2017 05:27:22 -0800 (PST) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id 1sm23893442wmz.2.2017.01.31.05.27.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 31 Jan 2017 05:27:22 -0800 (PST) Date: Tue, 31 Jan 2017 14:27:18 +0100 From: Olivier Matz To: Bruce Richardson Cc: "Ananyev, Konstantin" , "Wiles, Keith" , "dev@dpdk.org" Message-ID: <20170131142718.25e6c876@platinum> In-Reply-To: <20170131121050.GA130444@bricha3-MOBL3.ger.corp.intel.com> References: <20170125121456.GA24344@bricha3-MOBL3.ger.corp.intel.com> <20170125142052.7989e0ec@glumotte.dev.6wind.com> <20170125135404.GA24352@bricha3-MOBL3.ger.corp.intel.com> <20170125144809.GA26936@bricha3-MOBL3.ger.corp.intel.com> <647B0F09-667B-41D2-A8E1-964F71D4C365@intel.com> <20170125165740.GA33248@bricha3-MOBL3.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583F10CD7E@irsmsx105.ger.corp.intel.com> <20170131115349.7efadb09@platinum> <20170131114142.GA130764@bricha3-MOBL3.ger.corp.intel.com> <20170131121050.GA130444@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] rte_ring features in use (or not) 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: Tue, 31 Jan 2017 13:27:24 -0000 On Tue, 31 Jan 2017 12:10:50 +0000, Bruce Richardson wrote: > On Tue, Jan 31, 2017 at 11:41:42AM +0000, Bruce Richardson wrote: > > On Tue, Jan 31, 2017 at 11:53:49AM +0100, Olivier Matz wrote: > > > On Wed, 25 Jan 2017 17:29:18 +0000, "Ananyev, Konstantin" > > > wrote: > > > > > > > Bonus question: > > > > > > > * Do we know how widely used the enq_bulk/deq_bulk > > > > > > > functions are? They are useful for unit tests, so they do > > > > > > > have uses, but I think it would be good if we harmonized > > > > > > > the return values between bulk and burst functions. Right > > > > > > > now: enq_bulk - only enqueues all elements or none. > > > > > > > Returns 0 for all, or negative error for none. > > > > > > > enq_burst - enqueues as many elements as possible. > > > > > > > Returns the number enqueued. > > > > > > > > > > > > I do use the apis in pktgen and the difference in return > > > > > > values has got me once. Making them common would be great, > > > > > > but the problem is > > > > > backward compat to old versions I would need to have an ifdef > > > > > in pktgen now. So it seems like we moved the problem to the > > > > > application. > > > > > > > > > > > > > > > > Yes, an ifdef would be needed, but how many versions of DPDK > > > > > back do you support? Could the ifdef be removed again after > > > > > say, 6 months? > > > > > > I would like to see the old API kept and a new API with the > > > > > > new behavior. I know it adds another API but one of the API > > > > > > would be nothing > > > > > more than wrapper function if not a macro. > > > > > > > > > > > > Would that be more reasonable then changing the ABI? > > > > > > > > > > Technically, this would be an API rather than ABI change, > > > > > since the functions are inlined in the code. However, it's > > > > > not the only API change I'm looking to make here - I'd like > > > > > to have all the functions start returning details of the > > > > > state of the ring, rather than have the watermarks facility. > > > > > If we add all new functions for this and keep the old ones > > > > > around, we are just increasing our maintenance burden. > > > > > > > > > > I'd like other opinions here. Do we see increasing the API > > > > > surface as the best solution, or are we ok to change the APIs > > > > > of a key library like the rings one? > > > > > > > > I am ok with changing API to make both _bulk and _burst return > > > > the same thing. Konstantin > > > > > > I agree that the _bulk() functions returning 0 or -err can be > > > confusing. But it has at least one advantage: it explicitly shows > > > that if user ask for N enqueues/dequeues, it will either get N or > > > 0, not something between. > > > > > > Changing the API of the existing _bulk() functions looks a bit > > > dangerous to me. There's probably a lot of code relying on the > > > ring API, and changing its behavior may break it. > > > > > > I'd prefer to deprecate the old _bulk and _burst functions, and > > > introduce a new api, maybe something like: > > > > > > rte_ring_generic_dequeue(ring, objs, n, behavior, flags) > > > -> return nb_objs or -err > > > > > Don't like the -err, since it's not a valid value that can be used > > e.g. in simple loops in the case that the user doesn't care about > > the exact reason for error. I prefer having zero returned on error, > > with rte_errno set appropriately, since then it is trivial for apps > > to ignore error values they don't care about. > > It also makes the APIs in a ring library consistent in that all > > will set rte_errno on error, rather than returning the error code. > > It's not right for rte_ring_create and rte_ring_lookup to return an > > error code since they return pointers, not integer values. My assumption was that functions returning an int should return an error instead of rte_errno. By the way, it's actually the same debate than http://dpdk.org/ml/archives/dev/2017-January/056546.html In that particular case, I'm not convinced that this code: ret = ring_dequeue(r, objs, n); if (ret == 0) { /* handle error in rte_errno */ return; } do_stuff_with_elements(objs, ret); Is better/faster/clearer than this one: ret = ring_dequeue(r, objs, n); if (ret <= 0) { /* handle error in ret */ return; } do_stuff_with_elements(objs, ret); In the first case, you could argue that the "if (ret)" part could be stripped if the app does not care about errors, but I think it's not efficient to call the next function with 0 object. Also, this if() does not necessarily adds a test since ring_dequeue() is inline. In the first case, ring_dequeue needs to write rte_errno in memory on error (because it's a global variable), even if the caller does not look at it. In the second case, it can stay in a register. > > > > As for deprecating the functions - I'm not sure about that. I think > > the names of the existing functions are ok, and should be kept. > > I've a new patchset of cleanups for rte_rings in the works. Let me > > try and finish that and send it out as an RFC and we'll see what > > you think then. > Sorry, I realised on re-reading this reply seemed overly negative, > sorry. haha, no problem :) > I can actually see the case for deprecating both sets of > functions to allow us to "start afresh". If we do so, are we as well > to just replace the whole library with a new one, e.g. rte_fifo, which > would allow us the freedom to keep e.g. functions with "burst" in the > name if we so wish? If might also allow an easier transition. Yes, that's also an option. My fear is about changing the API of such widely used functions, without triggering any compilation error because the prototypes stays the same.