RE: MLX5 PMD access ring library private data

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >
> >     On 2023/8/18 12:30, Honnappa Nagarahalli wrote:
> >
> >
> >
> >
> >
> >             -----Original Message-----
> >
> >             From: Jack Min<jackmin@nvidia.com>
> > <mailto:jackmin@nvidia.com>
> >
> >             Sent: Thursday, August 17, 2023 9:32 PM
> >
> >             To: Stephen Hemminger<stephen@networkplumber.org>
> > <mailto:stephen@networkplumber.org>; Honnappa
> >
> >             Nagarahalli<Honnappa.Nagarahalli@arm.com>
> > <mailto:Honnappa.Nagarahalli@arm.com>
> >
> >             Cc:dev@dpdk.org; Matan Azrad<matan@nvidia.com>
> > <mailto:matan@nvidia.com>;
> >
> >             viacheslavo@nvidia.com; Tyler
> > Retzlaff<roretzla@linux.microsoft.com>
> > <mailto:roretzla@linux.microsoft.com>;
> >
> >             Wathsala Wathawana Vithanage<wathsala.vithanage@arm.com>
> > <mailto:wathsala.vithanage@arm.com>; nd
> >
> >             <nd@arm.com>  <mailto:nd@arm.com>
> >
> >             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<Honnappa.Nagarahalli@arm.com>
> <mailto:Honnappa.Nagarahalli@arm.com>  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).
> >
> >     */[Honnappa]/* May be I was not clear in my recommendation. What I
> >     am saying is, you could call ‘rte_ring_free_count’ to check if you
> >     have enough space on the cache ring. If there is not enough space
> >     you can enqueue the new objects on the global ring. Pseudo code below:
> >
> >     If (rte_ring_free_count(cache_ring) > n) {
> >
> >                  <enqueue n objects on cache ring>
> >
> >     } else {
> >
> >                  <enqueue n objects on global ring>
> >
> >     }
> >
> > Hey,
> >
> > Then next n objects will still enqueue into global ring, not into
> > cache , right? ( we enqueue nnnn objects continually)
> >
> > Our requirement is like this:
> >
> > if (rte_ring_free_count(cache_ring) > 0) {
> >
> >          <enqueue this object on cache ring>
> >
> > } else { /* cache is full */
> >
> >       <enqueue this object into global ring>
> >
> >      <move the latest n objects into global ring too>
> >
> > */[Honnappa] /*Understood. IMO, this is a unique requirement. Ring
> > library does not support this and is not designed for this. As per the
> > guidelines and past agreements, accessing structure members in ring
> > structures is not allowed.
> >
> Alright. Now I'm aware of this.
> 
> Do we have a document about this? I probably overlooked it...
Not sure if this is documented, we can add it to coding guidelines.

> 
> > I think a simple implementation like [1] would suffice your needs.
> >
> > [1]
> > https://patches.dpdk.org/project/dpdk/patch/20230821060420.3509667-1-
> h
> > onnappa.nagarahalli@arm.com/
> >
> > }
> >
> Yes, mostly.
> 
> It will be better if we have a "zero-copy" version, like:
> rte_st_ring_dequeue_zc_at_head_burst_elem_start()
Yes, will add
> 
> -Jack