RE: [RFC] lib/st_ring: add single thread ring

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.)
> >