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: Wed, 18 Jan 2017 11:09:17 +0000 [thread overview]
Message-ID: <20170118110916.GA48388@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,
>
> On Fri, 13 Jan 2017 15:00:54 +0000, Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > Hi Olivier,
> >
> > Thanks for the feedback.
> >
> > Yes, I thought about that parameterized sizes solution too, but I did
> > not pursue it primarily because I was worried about the performance
> > hits. It would mean that the actual copies of the data elements would
> > have to be done via memcpy calls - or switches based on size - rather
> > than assignments, as now. Given that all these calls to
> > enqueue/dequeue are inlined, that could really hurt performance, as
> > the size of the elements to be copied are unknown to the compiler at
> > compile time - as the size is stored in the struct, and not available
> > from the API call.
>
> 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.
>
I think if we go with this approach, just allowing sizes of 8/16/32
bytes may be the best, and we can optimize element assignments for those
particular sizes. I'd hold off on having other sizes beyond those until
such time as we have a concrete use case for it.
>
> > The compiler type-checking, I really like, being a
> > believer in having the compiler do as much work as possible for us,
> > but it is a nice-to-have rather than a mandatory requirement. :-)
> >
> > Am I right in assuming that the main issue that you see with the patch
> > is the use of macros may lead to problems with maintainability with
> > the code?
>
> Yes, from my experience, having unusual macros leads to loose time when
> trying to understand, use or change the code.
>
>
> > For me, while macros may not be the nicest solution to the problem:
> > * it does keep everything in rte_ring exactly as it was before - no
> > API and ABI issues
> > * it should be completely hidden from the end user - most applications
> > should never need to use the typed ring directly. Instead apps
> > should instead use rte_ring and rte_event_ring headers.
> > * The majority of the code is still regular C, and the macros don't
> > effect readability much IMHO. Also, it's comparatively rare that
> > there are changes being made to the ring library. [Though I have a few
> > follow-on ideas myself!].
> > * It gives us the maximum support from the compiler for type checking
> > and error reporting based on that
> >
> > This patchset is not for 17.02 so we have some time to consider our
> > options, though I would like some resolution on this early in the
> > 17.05 timeframe so as to reuse any solution inside any software
> > eventdevs we create.
>
>
> Yes, I hear your arguments. I don't have much to oppose except
> readability. Hmm the fact that init functions become static inline
> also bothers me a bit. All functions are static inline, so it closes the
> door to de-inline functions in the future.
Yes, it does close the door for now. However, I'd actually view this as
a positive since it eliminates problems of ABI compatibility. We can
freely change the internals of the ring from one release to the next, so
long as the API stays the same for compilation.
>
> 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 :)
>
I agree. I'll do some prototyping and see what the perf is like with
elt size in the ring structure. I'll also see what other alternative
approaches can be come up with here.
/Bruce
next prev parent reply other threads:[~2017-01-18 11:09 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 [this message]
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=20170118110916.GA48388@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).