From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Mattias Rönnblom" <hofors@lysator.liu.se>,
"Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com>,
jackmin@nvidia.com, konstantin.v.ananyev@yandex.ru
Cc: <dev@dpdk.org>, "Ruifeng Wang" <Ruifeng.Wang@arm.com>,
"Aditya Ambadipudi" <Aditya.Ambadipudi@arm.com>,
"Wathsala Wathawana Vithanage" <wathsala.vithanage@arm.com>,
"nd" <nd@arm.com>
Subject: RE: [RFC] lib/st_ring: add single thread ring
Date: Thu, 24 Aug 2023 13:22:06 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87B2D@smartserver.smartshare.dk> (raw)
In-Reply-To: <a5217d24-045e-2c48-ee7c-f08cf32c6c43@lysator.liu.se>
> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Thursday, 24 August 2023 12.53
>
> On 2023-08-24 10:05, Morten Brørup wrote:
> >> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> >> Sent: Tuesday, 22 August 2023 07.47
> >>
> >>> From: Morten Brørup <mb@smartsharesystems.com>
> >>> Sent: Monday, August 21, 2023 2:37 AM
> >>>
> >>>> From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> >>>> Sent: Monday, 21 August 2023 08.04
> >>>>
> >>>> Add a single thread safe and multi-thread unsafe ring data structure.
> >>>> This library provides an simple and efficient alternative to multi-
> >>>> thread safe ring when multi-thread safety is not required.
> >>>>
> >>>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>>> ---
> >>>
> >>> Good idea.
> >>>
> >>> However, I prefer it to be implemented in the ring lib as one more ring
> >> type.
> >>> That would also give us a lot of the infrastructure (management functions,
> >>> documentation and tests) for free.
> >> IMO, the current code for rte_ring seems complex with C11 and generic
> >> implementations, APIs for pointer objects vs APIs for flexible element size
> >> etc. I did not want to introduce one more flavor and make it more complex.
> >
> > From the user perspective, I think one more ring flavor is less complex
> than an entirely separate (very similar) library with its own set of (very
> similar) APIs.
> >
> > I agree that the ring lib has grown somewhat over-engineered, but please
> don't use that as an argument for making the same-thread ring a separate lib.
> >
>
> What's being proposed is a double-ended queue, not a ring (in the DPDK
> sense).
>
> If you want to Swiss army knifify the rte_ring further and make it a
> deque, then rte_stack should scrapped as well, since it's will become
> just a subset of the new rte_ring_now_really_a_deque.
OK. I accept that argument for not hacking it into the ring lib.
Then I will suggest that the new "deque" library should be designed with multi-threading in mind, like its two sibling libs (ring and stack). This makes it easier to use, and leaves it open for expansion to other flavors in the future.
It is perfectly acceptable that the first version only supports the same-thread deque flavor, and only the same-thread specialized APIs are exposed. I don't require any APIs or implementations supporting single-threaded (individual producer/consumer threads) or multi-threaded flavors, I only request that the design and API resemble those of its two sibling libraries. (And if there are no use cases for multi-threading flavors, they might never be added to this lib.)
>
> > On the other hand: If the addition of an optimized same-thread ring flavor
> would require too many invasive modifications of the existing ring lib, I
> would accept that as an argument for not adding it as another ring flavor to
> the existing ring lib.
> >
> >> The requirements are different as well. For ex: single thread ring needs
> APIs
> >> for dequeuing and enqueuing at both ends of the ring which is not
> applicable
> >> to existing RTE ring.
> >
> > Yes, I will address this topic at the end of this mail.
> >
> >>
> >> But, I see how the existing infra can be reused easily.
> >
> > This also goes for future infrastructure. I doubt that new infrastructure
> added to the ring lib will also be added to the same-thread ring lib... for
> reference, consider the PMDs containing copy-pasted code from the mempool
> lib... none of the later improvements of the mempool lib were implemented in
> those PMDs.
> >
> > In essence, I think this lib overlaps the existing ring lib too much to
> justify making it a separate lib.
> >
> >>
> >>>
> >>> The ring lib already has performance-optimized APIs for single-consumer
> and
> >>> single-producer use, rte_ring_sc_dequeue_bulk() and
> >>> rte_ring_sp_enqueue_burst(). Similar performance-optimized APIs for
> single-
> >>> thread use could be added: rte_ring_st_dequeue_bulk() and
> >>> rte_ring_st_enqueue_burst().
> >> Yes, the names look fine.
> >> Looking through the code. We have the sync type enum:
> >>
> >> /** prod/cons sync types */
> >> enum rte_ring_sync_type {
> >> RTE_RING_SYNC_MT, /**< multi-thread safe (default mode) */
> >> RTE_RING_SYNC_ST, /**< single thread only */
> >> RTE_RING_SYNC_MT_RTS, /**< multi-thread relaxed tail sync */
> >> RTE_RING_SYNC_MT_HTS, /**< multi-thread head/tail sync */
> >> };
> >>
> >> The type RTE_RING_SYNC_ST needs better explanation (not a problem). But,
> this
> >> name would have been ideal to use for single thread ring.
> >> This enum does not need to be exposed to the users. However, there are
> >> rte_ring_get_prod/cons_sync_type etc which seem to be exposed to the user.
> >> This all means, we need to have a sync type name RTE_RING_SYNC_MT_UNSAFE
> (any
> >> other better name?) which then affects API naming.
> >> rte_ring_mt_unsafe_dequeue_bulk?
> >
> > As always, naming is difficult.
> > The enum rte_ring_sync_type describes the producer and consumer
> independently, whereas this ring type uses the same thread for both producer
> and consumer.
> > I think we should avoid MT in the names for this variant. How about:
> >
> > RTE_RING_SYNC_STPC /**< same thread for both producer and consumer */
> >
> > And:
> >
> > rte_ring_spc_dequeue_bulk() and rte_ring_spc_enqueue_burst()
> >
> >>
> >>>
> >>> Regardless if added to the ring lib or as a separate lib, "reverse" APIs
> >> (for single-
> >>> thread use only) and zero-copy APIs can be added at any time later.
> >
> > As the only current use case for "reverse" (i.e. dequeue at tail, enqueue at
> head) APIs is for the same-thread ring flavor, we could start by adding only
> the specialized variants of the "reverse" APIs, rte_ring_spc_reverse_xxx(),
> and initially omit the generic rte_ring_reverse_xxx() APIs. (We need better
> names; I used "reverse" for explanation only.)
> >
next prev parent reply other threads:[~2023-08-24 11:22 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-21 6:04 Honnappa Nagarahalli
2023-08-21 7:37 ` Morten Brørup
2023-08-22 5:47 ` Honnappa Nagarahalli
2023-08-24 8:05 ` Morten Brørup
2023-08-24 10:52 ` Mattias Rönnblom
2023-08-24 11:22 ` Morten Brørup [this message]
2023-08-26 23:34 ` Honnappa Nagarahalli
2023-08-21 21:14 ` Mattias Rönnblom
2023-08-22 5:43 ` Honnappa Nagarahalli
2023-08-22 8:04 ` Mattias Rönnblom
2023-08-22 16:28 ` Honnappa Nagarahalli
2023-09-04 10:13 ` Konstantin Ananyev
2023-09-04 18:10 ` Honnappa Nagarahalli
2023-09-05 8:19 ` Konstantin Ananyev
2024-04-01 1:37 ` [PATCH v1 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-04-01 1:37 ` [PATCH v1 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-04-06 9:35 ` Morten Brørup
2024-04-24 13:42 ` [PATCH v2 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-04-24 13:42 ` [PATCH v2 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-04-24 15:16 ` Morten Brørup
2024-04-24 17:21 ` Patrick Robb
2024-04-25 7:43 ` Ali Alnubani
2024-04-24 23:28 ` Mattias Rönnblom
2024-05-02 20:19 ` [PATCH v3 0/2] deque: add multithread unsafe deque library Aditya Ambadipudi
2024-05-02 20:19 ` [PATCH v3 1/2] deque: add multi-thread unsafe double ended queue Aditya Ambadipudi
2024-05-02 20:19 ` [PATCH v3 2/2] deque: add unit tests for the deque library Aditya Ambadipudi
2024-05-02 20:29 ` [PATCH v3 0/2] deque: add multithread unsafe " Aditya Ambadipudi
2024-04-24 13:42 ` [PATCH v2 2/2] deque: add unit tests for the " Aditya Ambadipudi
2024-04-01 1:37 ` [PATCH v1 " Aditya Ambadipudi
2024-04-01 14:05 ` [PATCH v1 0/2] deque: add multithread unsafe " Stephen Hemminger
2024-04-01 22:28 ` Aditya Ambadipudi
2024-04-02 0:05 ` Tyler Retzlaff
2024-04-02 0:47 ` Stephen Hemminger
2024-04-02 1:35 ` Honnappa Nagarahalli
2024-04-02 2:00 ` Stephen Hemminger
2024-04-02 2:14 ` Honnappa Nagarahalli
2024-04-02 2:53 ` Stephen Hemminger
[not found] ` <PAVPR08MB9185DC373708CBD16A38EFA8EF3E2@PAVPR08MB9185.eurprd08.prod.outlook.com>
2024-04-02 4:20 ` Tyler Retzlaff
2024-04-02 23:44 ` Stephen Hemminger
2024-04-03 0:12 ` Honnappa Nagarahalli
2024-04-03 23:52 ` Variable name issues with codespell Stephen Hemminger
2024-04-02 4:20 ` [PATCH v1 0/2] deque: add multithread unsafe deque library Tyler Retzlaff
2024-04-03 16:50 ` Honnappa Nagarahalli
2024-04-03 17:46 ` Tyler Retzlaff
2024-04-02 6:05 ` Mattias Rönnblom
2024-04-02 15:25 ` Stephen Hemminger
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=98CBD80474FA8B44BF855DF32C47DC35D87B2D@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=Aditya.Ambadipudi@arm.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=dev@dpdk.org \
--cc=hofors@lysator.liu.se \
--cc=jackmin@nvidia.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=nd@arm.com \
--cc=wathsala.vithanage@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).