From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Bruce Richardson <bruce.richardson@intel.com>,
Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes
Date: Thu, 19 Jan 2017 12:15:42 +0000 [thread overview]
Message-ID: <0b6b6630-ebc7-ff06-5d6c-caebacf36ad3@intel.com> (raw)
In-Reply-To: <20170119121002.GA1720@bricha3-MOBL3.ger.corp.intel.com>
On 1/19/2017 12:10 PM, Bruce Richardson wrote:
> On Tue, Jan 17, 2017 at 02:38:20PM +0100, Olivier Matz wrote:
>> Hi Bruce,
>>
>> Maybe it's worth checking the impact. The size check could be done only
>> once per bulk, so it may not cost that much.
>>
>> It's also possible to have a particular case for pointer size, and
>> use a memcpy for other sizes.
>>
>>
> <snip>
>> I think having a performance test showing storing the elt size in the
>> ring structure has a deep impact would help to reach a consensus
>> faster :)
>>
>>
> Hi Olivier,
>
> I did a quick prototype using a switch statement for three data element
> sizes: 8, 16, and 32 bytes. The performance difference was neglible to
> none. In most cases, with ring_perf_autotest on my system, there was a
> small degradation, of less than 1 cycle per packet, and a few were
> slightly faster, probably due to the natural variation in results
> between runs. I did not test with any memcpy calls in the datapath, all
> assignments were done using uint64_t's or vectors of the appropriate
> sizes.
>
> Therefore it looks like some kind of solution without macros and using a
> stored element size is possible. However, I think there is a third
> alternative too. It is outlined below as option 3.
>
> 1. Use macros as in original RFC
>
> 2. Update rte_ring like I did for tests described above so that
> create takes the size parameter, and the switch statment in
> enqueue and dequeue looks that up at runtime.
> This means that rte_ring becomes the type used for all
> transfers of all sizes. Also, enqueues/dequeue functions take
> void * or const void * obj_table parameters rather than void
> ** and void * const * obj_table.
> Downside, this would change the ring API and ABI, and the
> ring maintains no type information
>
> 3. Update rte_ring as above but rename it to rte_common_ring,
> and have the element size parameter passed to enqueue and
> dequeue functions too - allowing the compiler to optimise the
> switch out. Then we update the existing rte_ring to use the
> rte_common_ring calls, passing in sizeof(void *) as parameter
> to each common call. An event-ring type, or any other ring
> types can similarly be written using common ring code, and
> present the appropriate type information on enqueue/dequeue
> to the apps using them.
> Downside: more code to maintain, and more specialised APIs.
>
> Personally, because I like having type-specialised code, I prefer the
> third option. It also gives us the ability to change the common code
> without affecting the API/ABI of the rings [which could be updated later
> after a proper deprecation period, if we want].
+1 for third option.
>
> An example of a change I have in mind for this common code would be some
> rework around the watermarks support. While the watermarks support is
> useful, for the event_rings we actually need more information provided
> from enqueue. To that end, I would see the common_rings code changed so
> that the enqueue function returns an additional parameter of the
> amount of space left in the ring. This information is computed by the
> function anyway, and can therefore be efficiently returned by the calls.
> For sp_enqueue, this extra parameter would allow the app to know the
> minimum number of elements which can be successfully enqueued to the
> ring in a subsequent call. The existing rte_ring code can use the return
> value to calculate itself if the watermark is exceeded and return a
> value as it does now. Other ring types can then decide themselves if
> they want to provide watermark functionality, or what their API set
> would be - though it's probably best to keep the APIs consistent.
>
> Further thoughts?
> /Bruce
>
prev parent reply other threads:[~2017-01-19 12:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-11 15:05 Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 01/11] ring: add new typed ring header file Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 02/11] test: add new test file for typed rings Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 03/11] ring: add ring management functions to typed ring header Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 04/11] ring: make ring tailq variable public Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 05/11] ring: add user-specified typing to typed rings Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 06/11] ring: use existing power-of-2 function Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 07/11] ring: allow multiple typed rings in the same unit Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 08/11] app/pdump: remove duplicate macro definition Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 09/11] ring: make existing rings reuse the typed ring definitions Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 10/11] ring: reuse typed rings management functions Bruce Richardson
2017-01-11 15:05 ` [dpdk-dev] [RFC PATCH 11/11] ring: reuse typed ring enqueue and dequeue functions Bruce Richardson
2017-01-13 14:23 ` [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes Olivier Matz
2017-01-13 15:00 ` Bruce Richardson
2017-01-17 13:38 ` Olivier Matz
2017-01-18 11:09 ` Bruce Richardson
2017-01-19 12:10 ` Bruce Richardson
2017-01-19 12:15 ` Ferruh Yigit [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0b6b6630-ebc7-ff06-5d6c-caebacf36ad3@intel.com \
--to=ferruh.yigit@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).