From: Bruce Richardson <bruce.richardson@intel.com>
To: 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:10:02 +0000 [thread overview]
Message-ID: <20170119121002.GA1720@bricha3-MOBL3.ger.corp.intel.com> (raw)
In-Reply-To: <20170117143820.336b40d9@platinum>
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].
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
next prev parent reply other threads:[~2017-01-19 12:10 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 [this message]
2017-01-19 12:15 ` Ferruh Yigit
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=20170119121002.GA1720@bricha3-MOBL3.ger.corp.intel.com \
--to=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).