DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "david.marchand@redhat.com" <david.marchand@redhat.com>,
	"jielong.zjl@antfin.com" <jielong.zjl@antfin.com>,
	nd <nd@arm.com>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/9] ring: introduce RTS ring mode
Date: Tue, 14 Apr 2020 13:18:44 +0000	[thread overview]
Message-ID: <BYAPR11MB330165888F01F69C5DCC05AC9ADA0@BYAPR11MB3301.namprd11.prod.outlook.com> (raw)
In-Reply-To: <AM6PR08MB4644F50A3D536F7D1E6B054598DE0@AM6PR08MB4644.eurprd08.prod.outlook.com>

> > > >
> > > > +#ifdef ALLOW_EXPERIMENTAL_API
> > > > +#include <rte_ring_rts.h>
> > > > +#endif
> > > > +
> > > >  /**
> > > >   * Enqueue several objects on a ring.
> > > >   *
> > > > @@ -484,8 +520,21 @@ static __rte_always_inline unsigned int
> > > > rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table,
> > > >  		      unsigned int n, unsigned int *free_space)  {
> > > > -	return __rte_ring_do_enqueue(r, obj_table, n,
> > > > RTE_RING_QUEUE_FIXED,
> > > > -			r->prod.sync_type, free_space);
> > > > +	switch (r->prod.sync_type) {
> > > > +	case RTE_RING_SYNC_MT:
> > > > +		return rte_ring_mp_enqueue_bulk(r, obj_table, n, free_space);
> > > > +	case RTE_RING_SYNC_ST:
> > > > +		return rte_ring_sp_enqueue_bulk(r, obj_table, n, free_space);
> > > Have you validated if these affect the performance for the existing APIs?
> >
> > I run ring_pmd_perf_autotest
> > (AFAIK, that's the only one of our perf tests that calls
> > rte_ring_enqueue/dequeue), and didn't see any real difference in perf
> > numbers.
> >
> > > I am also wondering why should we support these new modes in the legacy
> > APIs?
> >
> > Majority of DPDK users still do use legacy API, and I am not sure all of them
> > will be happy to switch to _elem_ one manually.
> > Plus I can't see how we can justify that after let say:
> > rte_ring_init(ring, ..., RING_F_MP_HTS_ENQ | RING_F_MC_HTS_DEQ); returns
> > with success valid call to rte_ring_enqueue(ring,...) should fail.
> Agree, I think the only way right now is through documentation.
> 
> >
> > > I think users should move to use rte_ring_xxx_elem APIs. If users want to
> > use RTS/HTS it will be a good time for them to move to new APIs.
> >
> > If they use rte_ring_enqueue/dequeue all they have to do - just change flags
> > in ring_create/ring_init call.
> > With what you suggest - they have to change every
> > rte_ring_enqueue/dequeue to rte_ring_elem_enqueue/dequeue.
> > That's much bigger code churn.
> But these are just 1 to 1 mapped.  I would think, there are not a whole lot of them in the application code, may be ~10 lines?

I suppose it depends a lot on particular user app.
My preference not to force users to do extra changes in their code.
If we can add new functionality while keeping existing API, why not to do it?
Less disturbance for users seems a good thing to me.

> I think the bigger factor for the user here is the algorithm changes in rte_ring library. Bigger effort for the users would be testing rather than
> code changes in the applications.
> >
> > > They anyway have to test their code for RTS/HTS, might as well make the
> > change to new APIs and test both.
> > > It will be less code to maintain for the community as well.
> >
> > That's true, right now there is a lot of duplication between _elem_ and legacy
> > code.
> >  Actually the only real diff between them - actual copying of the objects.
> >  But I thought we are going to deal with that, just by changing one day all
> > legacy API to wrappers around _elem_ calls, i.e something like:
> >
> > static __rte_always_inline unsigned int
> > rte_ring_enqueue_bulk(struct rte_ring *r, void * const *obj_table,
> >                       unsigned int n, unsigned int *free_space) {
> > 	return  rte_ring_enqueue_elem_bulk(r, obj_table, sizeof(uintptr_t), n,
> > free_space); }
> >
> > That way users will switch to new API automatically, without any extra effort
> > for them, and we will be able to remove legacy code.
> > Do you have some other thoughts here how to deal with this legacy/elem
> > conversion?
> Yes, that is what I was thinking, but had not considered any addition of new APIs.
> But, I am wondering if we should look at deprecation?

You mean to deprecate existing  legacy API?
rte_ring_enqueue/dequeue_bulk, etc?
I don't think we need to deprecate it at all.
As long as we'll have _elem_  functions called underneath there would be one implementation anyway,
and we can leave them forever, so users wouldn't need to change their existing code at all. 

> If we decide to deprecate, it would be good to avoid making the users of RTS/HTS do
> the work twice (once to make the switch to RTS/HTS and then another to _elem_ APIs).
> 
> One thing we can do is to implement the wrappers you mentioned above for RTS/HTS now.

That's a very good point.
 It will require some re-org to allow rte_ring.h to include rte_ring_elem.h,
but I think it is doable, will try to make these changes in v4. 

> I also it is worth considering to switch to these
> wrappers 20.05 so that come 20.11, we have a code base that has gone through couple of releases' testing.

You mean wrappers for existing legacy API (MP/MC, SP/SC modes)?
It is probably too late to make such changes in 20.05, probably early 20.08 is a good time for that.  

> 
> <snip>
> 
> > > > +
> > > > +#endif /* _RTE_RING_RTS_ELEM_H_ */
> > > > diff --git a/lib/librte_ring/rte_ring_rts_generic.h
> > > > b/lib/librte_ring/rte_ring_rts_generic.h
> > > > new file mode 100644
> > > > index 000000000..f88460d47
> > > > --- /dev/null
> > > > +++ b/lib/librte_ring/rte_ring_rts_generic.h
> > > I do not know the benefit to providing the generic version. Do you know
> > why this was done in the legacy APIs?
> >
> > I think at first we had generic API only, then later C11 was added.
> > As I remember, C11 one on IA was measured as a bit slower then generic,
> > so it was decided to keep both.
> >
> > > If there is no performance difference between generic and C11 versions,
> > should we just skip the generic version?
> > > The oldest compiler in CI are GCC 4.8.5 and Clang 3.4.2 and C11 built-ins
> > are supported earlier than these compiler versions.
> > > I feel the code is growing exponentially in rte_ring library and we should try
> > to cut non-value-ass code/APIs aggressively.
> >
> > I'll check is there perf difference for RTS and HTS between generic and C11
> > versions on IA.
> > Meanwhile please have a proper look at C11 implementation, I am not that
> > familiar with C11 atomics yet.
> ok
> 
> > If there would be no problems with it and no noticeable diff in performance -
> > I am ok to have for RTS/HTS modes C11 version only.

From what I see on my box, there is no much difference
in terms of performance between *generic* and *c11_mem* for RTS/HTS.
ring_stress_autotest for majority of cases shows ~1% diff,
in some cases c11 numbers are even a bit better.
So will keep c11 version only in v4.


  parent reply	other threads:[~2020-04-14 13:18 UTC|newest]

Thread overview: 146+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-24 11:35 [dpdk-dev] [RFC 0/6] New sync modes for ring Konstantin Ananyev
2020-02-24 11:35 ` [dpdk-dev] [RFC 1/6] test/ring: add contention stress test Konstantin Ananyev
2020-02-24 11:35 ` [dpdk-dev] [RFC 2/6] ring: rework ring layout to allow new sync schemes Konstantin Ananyev
2020-02-24 11:35 ` [dpdk-dev] [RFC 3/6] ring: introduce RTS ring mode Konstantin Ananyev
2020-02-24 11:35 ` [dpdk-dev] [RFC 4/6] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-02-24 11:35 ` [dpdk-dev] [RFC 5/6] ring: introduce HTS ring mode Konstantin Ananyev
2020-03-25 20:44   ` Honnappa Nagarahalli
2020-03-26 12:26     ` Ananyev, Konstantin
2020-02-24 11:35 ` [dpdk-dev] [RFC 6/6] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-02-24 16:59 ` [dpdk-dev] [RFC 0/6] New sync modes for ring Stephen Hemminger
2020-02-24 17:59   ` Jerin Jacob
2020-02-24 19:35     ` Stephen Hemminger
2020-02-24 20:52       ` Honnappa Nagarahalli
2020-02-25 11:45         ` Ananyev, Konstantin
2020-02-25 13:41       ` Ananyev, Konstantin
2020-02-26 16:53         ` Morten Brørup
2020-02-27 10:31         ` Jerin Jacob
2020-02-28  0:17           ` David Christensen
2020-03-20 16:45             ` Ananyev, Konstantin
2020-02-25  0:58     ` Honnappa Nagarahalli
2020-02-25 15:14       ` Ananyev, Konstantin
2020-03-25 20:43 ` Honnappa Nagarahalli
2020-03-26  1:50   ` Ananyev, Konstantin
2020-03-30 21:29     ` Honnappa Nagarahalli
2020-03-30 23:37       ` Honnappa Nagarahalli
2020-03-31 17:21         ` Ananyev, Konstantin
2020-03-31 16:43 ` [dpdk-dev] [PATCH v1 0/8] " Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 1/8] test/ring: add contention stress test Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 2/8] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 3/8] ring: introduce RTS ring mode Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 4/8] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 5/8] ring: introduce HTS ring mode Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 6/8] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 7/8] ring: introduce peek style API Konstantin Ananyev
2020-03-31 16:43   ` [dpdk-dev] [PATCH v1 8/8] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-02 22:09   ` [dpdk-dev] [PATCH v2 0/9] New sync modes for ring Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 1/9] test/ring: add contention stress test Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 2/9] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 3/9] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 4/9] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 5/9] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 6/9] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 7/9] ring: introduce peek style API Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 8/9] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-02 22:09     ` [dpdk-dev] [PATCH v2 9/9] ring: add C11 memory model for new sync modes Konstantin Ananyev
2020-04-03 17:42     ` [dpdk-dev] [PATCH v3 0/9] New sync modes for ring Konstantin Ananyev
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 1/9] test/ring: add contention stress test Konstantin Ananyev
2020-04-08  4:59         ` Honnappa Nagarahalli
2020-04-09 12:36           ` Ananyev, Konstantin
2020-04-09 13:00             ` Ananyev, Konstantin
2020-04-10 18:01               ` Honnappa Nagarahalli
2020-04-10 16:59             ` Honnappa Nagarahalli
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 2/9] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-08  4:59         ` Honnappa Nagarahalli
2020-04-09 13:39           ` Ananyev, Konstantin
2020-04-10 20:15             ` Honnappa Nagarahalli
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 3/9] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-04 17:27         ` Wang, Haiyue
2020-04-08  5:00         ` Honnappa Nagarahalli
2020-04-09 14:52           ` Ananyev, Konstantin
2020-04-10 23:10             ` Honnappa Nagarahalli
2020-04-13 14:29               ` David Marchand
2020-04-13 16:42                 ` Honnappa Nagarahalli
2020-04-14 13:47                   ` David Marchand
2020-04-14 15:57                     ` Honnappa Nagarahalli
2020-04-14 16:21                       ` Ananyev, Konstantin
2020-04-14 13:18               ` Ananyev, Konstantin [this message]
2020-04-14 15:58                 ` Honnappa Nagarahalli
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 4/9] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 5/9] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-13 23:27         ` Honnappa Nagarahalli
2020-04-14 16:12           ` Ananyev, Konstantin
2020-04-14 17:06             ` Honnappa Nagarahalli
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 6/9] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 7/9] ring: introduce peek style API Konstantin Ananyev
2020-04-14  3:45         ` Honnappa Nagarahalli
2020-04-14 16:47           ` Ananyev, Konstantin
2020-04-14 17:30             ` Honnappa Nagarahalli
2020-04-14 22:24               ` Ananyev, Konstantin
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 8/9] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-03 17:42       ` [dpdk-dev] [PATCH v3 9/9] ring: add C11 memory model for new sync modes Konstantin Ananyev
2020-04-04 14:16         ` [dpdk-dev] 回复:[PATCH " 周介龙
2020-04-14  4:28         ` [dpdk-dev] [PATCH " Honnappa Nagarahalli
2020-04-14 18:29           ` Ananyev, Konstantin
2020-04-15 20:28           ` Ananyev, Konstantin
2020-04-17 13:36       ` [dpdk-dev] [PATCH v4 0/9] New sync modes for ring Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 1/9] test/ring: add contention stress test Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 2/9] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 3/9] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 4/9] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 5/9] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 6/9] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 7/9] ring: introduce peek style API Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 8/9] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-17 13:36         ` [dpdk-dev] [PATCH v4 9/9] test/ring: add functional tests for new sync modes Konstantin Ananyev
2020-04-18 16:32         ` [dpdk-dev] [PATCH v5 0/9] New sync modes for ring Konstantin Ananyev
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 1/9] test/ring: add contention stress test Konstantin Ananyev
2020-04-19  2:30             ` Honnappa Nagarahalli
2020-04-19  8:03               ` David Marchand
2020-04-19 11:47                 ` Ananyev, Konstantin
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 2/9] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 3/9] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 4/9] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 5/9] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 6/9] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 7/9] ring: introduce peek style API Konstantin Ananyev
2020-04-19  2:31             ` Honnappa Nagarahalli
2020-04-19 18:32               ` Ananyev, Konstantin
2020-04-19 19:12                 ` Ananyev, Konstantin
2020-04-19 21:14                   ` Honnappa Nagarahalli
2020-04-19 22:41                     ` Ananyev, Konstantin
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 8/9] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-19  2:32             ` Honnappa Nagarahalli
2020-04-18 16:32           ` [dpdk-dev] [PATCH v5 9/9] test/ring: add functional tests for new sync modes Konstantin Ananyev
2020-04-19  2:32             ` Honnappa Nagarahalli
2020-04-19  2:32           ` [dpdk-dev] [PATCH v5 0/9] New sync modes for ring Honnappa Nagarahalli
2020-04-20 12:11           ` [dpdk-dev] [PATCH v6 00/10] " Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 01/10] test/ring: add contention stress test Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 02/10] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 03/10] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 04/10] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 05/10] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 06/10] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 07/10] ring: introduce peek style API Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 08/10] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 09/10] test/ring: add functional tests for new sync modes Konstantin Ananyev
2020-04-20 12:11             ` [dpdk-dev] [PATCH v6 10/10] doc: update ring guide Konstantin Ananyev
2020-04-20 13:47               ` David Marchand
2020-04-20 14:07                 ` Ananyev, Konstantin
2020-04-20 12:28             ` [dpdk-dev] [PATCH v7 00/10] New sync modes for ring Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 01/10] test/ring: add contention stress test Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 02/10] ring: prepare ring to allow new sync schemes Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 03/10] ring: introduce RTS ring mode Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 04/10] test/ring: add contention stress test for RTS ring Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 05/10] ring: introduce HTS ring mode Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 06/10] test/ring: add contention stress test for HTS ring Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 07/10] ring: introduce peek style API Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 08/10] test/ring: add stress test for MT peek API Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 09/10] test/ring: add functional tests for new sync modes Konstantin Ananyev
2020-04-20 12:28               ` [dpdk-dev] [PATCH v7 10/10] doc: update ring guide Konstantin Ananyev
2020-04-21 11:31               ` [dpdk-dev] [PATCH v7 00/10] New sync modes for ring David Marchand

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=BYAPR11MB330165888F01F69C5DCC05AC9ADA0@BYAPR11MB3301.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jielong.zjl@antfin.com \
    --cc=nd@arm.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).