From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 5BEFF2E8A for ; Fri, 13 Jan 2017 16:01:05 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga103.jf.intel.com with ESMTP; 13 Jan 2017 07:00:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,221,1477983600"; d="scan'208";a="52891455" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.61]) by fmsmga005.fm.intel.com with SMTP; 13 Jan 2017 07:00:54 -0800 Received: by (sSMTP sendmail emulation); Fri, 13 Jan 2017 15:00:54 +0000 Date: Fri, 13 Jan 2017 15:00:54 +0000 From: Bruce Richardson To: Olivier Matz Cc: dev@dpdk.org Message-ID: <20170113150053.GA193796@bricha3-MOBL3.ger.corp.intel.com> References: <1484147125-5948-1-git-send-email-bruce.richardson@intel.com> <20170113152334.5efda35a@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170113152334.5efda35a@platinum> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.7.1 (2016-10-04) Subject: Re: [dpdk-dev] [RFC PATCH 00/11] generalise rte_ring to allow different datatypes 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: Fri, 13 Jan 2017 15:01:05 -0000 On Fri, Jan 13, 2017 at 03:23:34PM +0100, Olivier Matz wrote: > Hi Bruce, > > On Wed, 11 Jan 2017 15:05:14 +0000, Bruce Richardson > 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 > > #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, 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 > 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. 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? 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. Regards, /Bruce