From: Olivier Matz <olivier.matz@6wind.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes
Date: Fri, 13 Jan 2017 15:23:34 +0100 [thread overview]
Message-ID: <20170113152334.5efda35a@platinum> (raw)
In-Reply-To: <1484147125-5948-1-git-send-email-bruce.richardson@intel.com>
Hi Bruce,
On Wed, 11 Jan 2017 15:05:14 +0000, Bruce Richardson
<bruce.richardson@intel.com> wrote:
> The rte_ring library in DPDK provides an excellent high-performance
> mechanism which can be used for passing pointers between cores and
> for other tasks such as buffering. However, it does have a number
> of limitations:
>
> * type information of pointers is lost, as it works with void pointers
> * typecasting is needed when using enqueue/dequeue burst functions,
> since arrays of other types cannot be automatically cast to void **
> * the data to be passed through the ring itself must be no bigger than
> a pointer
>
> While the first two limitations are an inconvenience, the final one is
> one that can prevent use of rte_rings in cases where their
> functionality is needed. The use-case which has inspired the patchset
> is that of eventdev. When working with rte_events, each event is a
> 16-byte structure consisting of a pointer and some metadata e.g.
> priority and type. For these events, what is passed around between
> cores is not pointers to events, but the events themselves. This
> makes existing rings unsuitable for use by applications working with
> rte_events, and also for use internally inside any software
> implementation of an eventdev.
>
> For rings to handle events or other similarly sized structures, e.g.
> NIC descriptors, etc., we then have two options - duplicate rte_ring
> code to create new ring implementations for each of those types, or
> generalise the existing code using macros so that the data type
> handled by each rings is a compile time paramter. This patchset takes
> the latter approach, and once applied would allow us to add an
> rte_event_ring type to DPDK using a header file containing:
>
> #define RING_TYPE struct rte_event
> #define RING_TYPE_NAME rte_event
> #include <rte_typed_ring.h>
> #undef RING_TYPE_NAME
> #undef RING_TYPE
>
> [NOTE: the event_ring is not defined in this set, since it depends on
> the eventdev implementation not present in the main tree]
>
> If we want to elimiate some of the typecasting on our code when
> enqueuing and dequeuing mbuf pointers, an rte_mbuf_ring type can be
> similarly created using the same number of lines of code.
>
> The downside of this generalisation is that the code for the rings now
> has far more use of macros in it. However, I do not feel that overall
> readability suffers much from this change, the since the changes are
> pretty much just search-replace onces. There should also be no ABI
> compatibility issues with this change, since the existing rte_ring
> structures remain the same.
I didn't dive deeply in the patches, just had a quick look. I
understand the need, and even if I really don't like the "#define +
#include" way to create a new specific ring (for readability,
grepability), that may be a solution to your problem.
I think using a similar approach than in sys/queue.h would be even
worse in terms of readability.
What do you think about the following approach?
- add a new elt_size in rte_ring structure
- update create/enqueue/dequeue/... functions to manage the elt size
- change:
rte_ring_enqueue_bulk(struct rte_ring *r,
void * const *obj_table, unsigned n)
to:
rte_ring_enqueue_bulk(struct rte_ring *r, void *obj_table,
unsigned n)
This relaxes the type for the API in the function. In the caller,
the type of obj_table would be:
- (void **) in case of a ring of pointers
- (uint8_t *) in case of a ring of uint8_t
- (struct rte_event *) in case of a ring of rte_event
...
I think (I have not tested it) it won't break compilation since
any type can be implicitly casted into a void *. Also, I'd say it
is possible to avoid breaking the ABI.
- deprecate or forbid calls to:
rte_ring_mp_enqueue(struct rte_ring *r, void *obj)
(and similar)
Because with a ring of pointers, obj is the pointer, passed by value.
For other types, we would need
rte_ring_mp_enqueue(struct rte_ring *r, <TYPE> obj)
Maybe we could consider using a macro here.
The drawbacks I see are:
- a dynamic elt_size may slightly decrease performance
- it still uses casts to (void *), so there is no type checking
Regards,
Olivier
next prev parent reply other threads:[~2017-01-13 14:23 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 ` Olivier Matz [this message]
2017-01-13 15:00 ` [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes 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
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=20170113152334.5efda35a@platinum \
--to=olivier.matz@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
/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).