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 843B5430EF; Thu, 24 Aug 2023 12:52:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 75ADD41140; Thu, 24 Aug 2023 12:52:49 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 1525C40150 for ; Thu, 24 Aug 2023 12:52:48 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 8E3191D83C for ; Thu, 24 Aug 2023 12:52:47 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 8D08A1D647; Thu, 24 Aug 2023 12:52:47 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -2.9 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 57B551D745; Thu, 24 Aug 2023 12:52:46 +0200 (CEST) Message-ID: Date: Thu, 24 Aug 2023 12:52:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [RFC] lib/st_ring: add single thread ring To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Honnappa Nagarahalli , jackmin@nvidia.com, konstantin.v.ananyev@yandex.ru Cc: dev@dpdk.org, Ruifeng Wang , Aditya Ambadipudi , Wathsala Wathawana Vithanage , nd References: <20230821060420.3509667-1-honnappa.nagarahalli@arm.com> <98CBD80474FA8B44BF855DF32C47DC35D87B22@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87B2B@smartserver.smartshare.dk> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87B2B@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 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 >>> 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. > 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. > 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.) >