On 2023/8/18 12:30, Honnappa Nagarahalli wrote: > >> -----Original Message----- >> From: Jack Min >> Sent: Thursday, August 17, 2023 9:32 PM >> To: Stephen Hemminger; Honnappa >> Nagarahalli >> Cc:dev@dpdk.org; Matan Azrad; >> viacheslavo@nvidia.com; Tyler Retzlaff; >> Wathsala Wathawana Vithanage; nd >> >> Subject: Re: MLX5 PMD access ring library private data >> >> On 2023/8/17 22:06, Stephen Hemminger wrote: >>> On Thu, 17 Aug 2023 05:06:20 +0000 >>> Honnappa Nagarahalli wrote: >>> >>>> Hi Matan, Viacheslav, >>>> Tyler pointed out that the function >> __mlx5_hws_cnt_pool_enqueue_revert is accessing the ring private structure >> members (prod.head and prod.tail) directly. Even though ' struct rte_ring' is a >> public structure (mainly because the library provides inline functions), the >> structure members are considered private to the ring library. So, this needs to >> be corrected. >>>> It looks like the function __mlx5_hws_cnt_pool_enqueue_revert is trying >> to revert things that were enqueued. It is not clear to me why this >> functionality is required. Can you provide the use case for this? We can >> discuss possible solutions. >>> How can reverting be thread safe? Consumer could have already looked at >> them? >> >> Hey, >> >> In our case, this ring is SC/SP, only accessed by one thread >> (enqueue/dequeue/revert). > You could implement a more simpler and more efficient (For ex: such an implementation would not need any atomic operations, would require less number of cache lines) ring for this. > Is this function being used in the dataplane? Yes,  we can have our own version of ring (no atomic operations) but basic operation are still as same as rte_ring. Since rte ring has been well-designed and tested sufficiently, so there is no strong reason to re-write a new simple version of it until today :) > >> The scenario we have "revert" is: >> >>  We use ring to manager our HW objects (counter in this case) and for each >> core (thread) has "cache" (a SC/SP ring) for sake of performance. >> >> 1. Get objects from "cache" firstly, if cache is empty, we fetch a bulk of free >> objects from global ring into cache. >> >> 2. Put (free) objects also into "cache" firstly, if cache is full, we flush a bulk of >> objects into global ring in order to make some rooms in cache. >> >> However, this HW object cannot be immediately reused after free. It needs >> time to be reset and then can be used again. >> >> So when we flush cache, we want to keep the first enqueued objects still stay >> there because they have more chance already be reset than the latest >> enqueued objects. >> >> Only flush recently enqueued objects back into global ring, act as "LIFO" >> behavior. >> >> This is why we require "revert" enqueued objects. > You could use 'rte_ring_free_count' API before you enqueue to check for available space. Only when cache is full (rte_ring_free_count() is zero), we revert X objects. If there is still  one free slot we will not trigger revert (flush). > >> -Jack >>