From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 3A4973B5 for ; Tue, 14 Feb 2017 09:32:23 +0100 (CET) Received: by mail-wm0-f42.google.com with SMTP id t18so12162085wmt.0 for ; Tue, 14 Feb 2017 00:32:23 -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=Cq2sXSegMYZp8plE3TGDh8Rufk4d4b6izRR9gdJVpr0=; b=a4oxfh8GEiNSCRJUF3JSGBhZQNN+7P+tQ4zPmD4rKc1IpJZ0Vp8mFJyhcvoeqz9dUy xdMsUouH52bhhrDdoyBXsGaQX4oJ2YRA3vZAciMRItk4Ax6/zoYB/MWmNpFy3DeZ6pGF 1r8pgj3+Xb2VROZTWgX+a34Wf7ibvvzLVEvkfdLZujPRaJtiXwaUT0hkstdRBcdsaF2j yL8PpXUTzhOoYCevPaqGE8g+92E2DJfDgEDRVnuJtn2KpF0d631NN+9hatuj8Es8uqzo 2ro6z84NYCAswtv7oKO4VCprvI35cKwNFbHrKwkpcDwGWP27FSmR6TkLMu8kHfIONRbR cRpQ== 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=Cq2sXSegMYZp8plE3TGDh8Rufk4d4b6izRR9gdJVpr0=; b=mrisA26s/BX2V/4/W6OmwElG3tqdoiSQAzjJga9ctDlTo97v5DsGZPkYO2JsFLyh11 OGDsprADfAcCi+NWYxHp6Wrl34qqTxUVcBcMasYZ1aV/N9OgTle39P87GqtRxPB/BmWC YzE98Ipga0MTXfZD+iibIMqK9zh+97VnzBWxTHgbsH5g2BPE+9P9wgG4UPfHcUU7wKGA pvPszuShpuOMFKqoEJ0xbLo3ZzgMIqW50ZE5SISSnFl2POUpVAH9SBqHjGNKFxcA5icI RxUUDbhRg3lZ9oalxePwBPLL6AJZ2JFktAPRL4Sv024aZKMeMYRwWSSW8gnhEBg3SBzi P/zw== X-Gm-Message-State: AMke39mn8fgftThrfqVnWIrjZA2GLtK4syrGZ1HZXjOY/9XGXOw8rW0pZQVQjNewVslxZpzv X-Received: by 10.28.174.208 with SMTP id x199mr2189339wme.107.1487061142880; Tue, 14 Feb 2017 00:32: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 r19sm16964528wrr.44.2017.02.14.00.32.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 14 Feb 2017 00:32:22 -0800 (PST) Date: Tue, 14 Feb 2017 09:32:20 +0100 From: Olivier Matz To: Bruce Richardson Cc: thomas.monjalon@6wind.com, keith.wiles@intel.com, konstantin.ananyev@intel.com, stephen@networkplumber.org, dev@dpdk.org Message-ID: <20170214093220.5d2c30e7@platinum> In-Reply-To: <1486476777-24768-1-git-send-email-bruce.richardson@intel.com> References: <20170125121456.GA24344@bricha3-MOBL3.ger.corp.intel.com> <1486476777-24768-1-git-send-email-bruce.richardson@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 RFCv3 00/19] ring cleanup and generalization 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, 14 Feb 2017 08:32:23 -0000 Hi Bruce, On Tue, 7 Feb 2017 14:12:38 +0000, Bruce Richardson wrote: > This patchset make a set of, sometimes non-backward compatible, > cleanup changes to the rte_ring code in order to improve it. The > resulting code is shorter*, since the existing functions are > restructured to reduce code duplication, as well as being more > consistent in behaviour. The specific changes made are explained in > each patch which makes that change. > > Key incompatibilities: > * The biggest, and probably most controversial change is that to the > enqueue and dequeue APIs. The enqueue/deq burst and bulk functions > have their function prototypes changed so that they all return an > additional parameter, indicating the size of next call which is > guaranteed to succeed. In case on enq, this is the number of > available slots on the ring, and in case of deq, it is the number of > objects which can be pulled. As well as this, the return value from > the bulk functions have been changed to make them compatible with the > burst functions. In all cases, the functions to enq/deq a set of objs > now return the number of objects processed, 0 or N, in the case of > bulk functions, 0, N or any value in between in the case of the burst > ones. [Due to the extra parameter, the compiler will flag all > instances of the function to allow the user to also change the return > value logic at the same time] > * The parameters to the single object enq/deq functions have not been > changed. Because of that, the return value is also unmodified - as > the compiler cannot automatically flag this to the user. > > Potential further cleanups: > * To a certain extent the rte_ring structure has gone from being a > whole ring structure, including a "ring" element itself, to just > being a header which can be reused, along with the head/tail update > functions to create new rings. For now, the enqueue code works by > assuming that the ring data goes immediately after the header, but > that can be changed to allow specialised ring implementations to put > additional metadata of their own after the ring header. I didn't see > this as being needed right now, but it may be worth considering for a > V1 patchset. > * There are 9 enqueue functions and 9 dequeue functions in > rte_ring.h. I suspect not all of those are used, so personally I > would consider dropping the functions to enqueue/dequeue a single > value using single or multi semantics, i.e. drop > rte_ring_sp_enqueue > rte_ring_mp_enqueue > rte_ring_sc_dequeue > rte_ring_mc_dequeue > That would still leave a single enqueue and dequeue function for > working with a single object at a time. > * It should be possible to merge the head update code for enqueue and > dequeue into a single function. The key difference between the two > is the calculation of how far the index can be moved. I felt that the > functions for moving the head index are sufficiently complicated > with many parameters to them already, that trying to merge in more > code would impede readability. However, if so desired this change can > be made at a later stage without affecting ABI or API. > > PERFORMANCE: > I've run performance autotests on a couple of (Intel) platforms. > Looking particularly at the core-2-core results, which I expect are > the main ones of interest, the performance after this patchset is a > few cycles per packet faster in my testing. I'm hoping it should be > at least neutral perf-wise. > > REQUEST FOR FEEDBACK: > * Are all of these changes worth making? I've quickly browsed all the patches. I think yes, we should do it: it brings a good cleanup, removing features we don't need, restructuring the code, and also adding the feature you need :) > * Should they be made in existing ring code, or do we look to provide > a new fifo library to completely replace the ring one? I think it's ok to have it in the existing code. Breaking the ABI is never suitable, but I think having 2 libs would be even more confusing. > * How does the implementation of new ring types using this code > compare vs that of the previous RFCs? I prefer this version, especially compared to the first RFC. Thanks for this big rework. I'll dive into the patches a do a more exhaustive review soon. Regards, Olivier