From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19936430EE; Thu, 24 Aug 2023 10:06:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AA792410EE; Thu, 24 Aug 2023 10:06:05 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 2047F40EE1 for ; Thu, 24 Aug 2023 10:06:04 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 4546D205ED; Thu, 24 Aug 2023 10:06:00 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [RFC] lib/st_ring: add single thread ring Date: Thu, 24 Aug 2023 10:05:58 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87B2B@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [RFC] lib/st_ring: add single thread ring Thread-Index: AQHZ1AJIj5UOZIDmtU+IXup+eth44a/1vMRwgANPOHA= References: <20230821060420.3509667-1-honnappa.nagarahalli@arm.com> <98CBD80474FA8B44BF855DF32C47DC35D87B22@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Honnappa Nagarahalli" , , , Cc: , "Ruifeng Wang" , "Aditya Ambadipudi" , "Wathsala Wathawana Vithanage" , "nd" , "nd" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Tuesday, 22 August 2023 07.47 >=20 > > From: Morten Br=F8rup > > 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 > > > --- > > > > 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. 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. >=20 > 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. >=20 > > > > 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: >=20 > /** 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 */ > }; >=20 > 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() >=20 > > > > 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.)