[PATCH 0/4] Small corrections in mempool

[PATCH 0/4] Small corrections in mempool

[Honnappa Nagarahalli]

1) Patches 1/4, 2/4 - Few small corrections in mempool API documentation.
2) Patch 3/4 - The API for checking 'lcore ID is valid' is trivial,
   but it is the right thing to do.
3) Patch 4/4 - IMO, the 'lcore ID is valid' check is not necessary to be
   done during run time. If it is not valid, there is something seriously
   wrong in the system already or it is a programming error. Given that
   it is a data plane function, it makes sense to remove it from run
   time check.

Honnappa Nagarahalli (4):
  mempool: clarify mempool cache flush API behavior
  mempool: clarify comments for mempool cache implementation
  eal: add API to check if lcore id is valid
  mempool: use lcore API to check if lcore ID is valid

 lib/eal/include/rte_lcore.h | 14 ++++++++++++++
 lib/mempool/rte_mempool.c   | 12 ++++++------
 lib/mempool/rte_mempool.h   | 13 ++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/4] mempool: clarify mempool cache flush API behavior

[Honnappa Nagarahalli]

Clarify that mempool cache flush API works with default mempool cache.
It is applications responsibility to validate that the cache
belongs to the specified mempool.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/mempool/rte_mempool.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 9f530db24b..009bd10215 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1326,10 +1326,14 @@ rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
 }
 
 /**
- * Flush a user-owned mempool cache to the specified mempool.
+ * Flush a mempool cache to the specified mempool.
+ *
+ * It is application's responsibility to validate that the mempool cache
+ * belongs to the specified mempool.
  *
  * @param cache
- *   A pointer to the mempool cache.
+ *   A pointer to the mempool cache. If NULL, default mempool cache
+ *   is used if configured.
  * @param mp
  *   A pointer to the mempool.
  */
-- 
2.25.1

RE: [PATCH 1/4] mempool: clarify mempool cache flush API behavior

[Morten Brørup]

> From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> Sent: Thursday, 9 March 2023 05.58
> 
> Clarify that mempool cache flush API works with default mempool cache.
> It is applications responsibility to validate that the cache
> belongs to the specified mempool.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---

Acked-by: Morten Brørup <mb@smartsharesystems.com>

[PATCH 2/4] mempool: clarify comments for mempool cache implementation

[Honnappa Nagarahalli]

Clarify that the mempool cache create and free API implementations
work on user owned mempool caches.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/mempool/rte_mempool.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index cf5dea2304..a46d855dbf 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -757,9 +757,9 @@ mempool_cache_init(struct rte_mempool_cache *cache, uint32_t size)
 }
 
 /*
- * Create and initialize a cache for objects that are retrieved from and
- * returned to an underlying mempool. This structure is identical to the
- * local_cache[lcore_id] pointed to by the mempool structure.
+ * Create and initialize a user owned cache for objects that are retrieved
+ * from and returned to an underlying mempool. This structure is identical
+ * to the local_cache[lcore_id] pointed to by the mempool structure.
  */
 struct rte_mempool_cache *
 rte_mempool_cache_create(uint32_t size, int socket_id)
@@ -786,9 +786,9 @@ rte_mempool_cache_create(uint32_t size, int socket_id)
 }
 
 /*
- * Free a cache. It's the responsibility of the user to make sure that any
- * remaining objects in the cache are flushed to the corresponding
- * mempool.
+ * Free a user owned cache. It's the responsibility of the user to make
+ * sure that any remaining objects in the cache are flushed to the
+ * corresponding mempool.
  */
 void
 rte_mempool_cache_free(struct rte_mempool_cache *cache)
-- 
2.25.1

RE: [PATCH 2/4] mempool: clarify comments for mempool cache implementation

[Morten Brørup]

> From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> Sent: Thursday, 9 March 2023 05.58
> 
> Clarify that the mempool cache create and free API implementations
> work on user owned mempool caches.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  lib/mempool/rte_mempool.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index cf5dea2304..a46d855dbf 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -757,9 +757,9 @@ mempool_cache_init(struct rte_mempool_cache *cache,
> uint32_t size)
>  }
> 
>  /*
> - * Create and initialize a cache for objects that are retrieved from and
> - * returned to an underlying mempool. This structure is identical to the
> - * local_cache[lcore_id] pointed to by the mempool structure.
> + * Create and initialize a user owned cache for objects that are retrieved
> + * from and returned to an underlying mempool. This structure is identical
> + * to the local_cache[lcore_id] pointed to by the mempool structure.
>   */
>  struct rte_mempool_cache *
>  rte_mempool_cache_create(uint32_t size, int socket_id)
> @@ -786,9 +786,9 @@ rte_mempool_cache_create(uint32_t size, int socket_id)
>  }
> 
>  /*
> - * Free a cache. It's the responsibility of the user to make sure that any
> - * remaining objects in the cache are flushed to the corresponding
> - * mempool.
> + * Free a user owned cache. It's the responsibility of the user to make
> + * sure that any remaining objects in the cache are flushed to the
> + * corresponding mempool.
>   */
>  void
>  rte_mempool_cache_free(struct rte_mempool_cache *cache)
> --
> 2.25.1
> 

These comments should probably be updated in the function descriptions in rte_mempool.h too, so they go into the auto-generated documentation.

Re: [PATCH 2/4] mempool: clarify comments for mempool cache implementation

[Stephen Hemminger]

On Wed, 7 Jun 2023 12:10:01 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> > Sent: Thursday, 9 March 2023 05.58
> > 
> > Clarify that the mempool cache create and free API implementations
> > work on user owned mempool caches.
> > 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Kamalakshitha Aligeri <kamalakshitha.aligeri@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  lib/mempool/rte_mempool.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index cf5dea2304..a46d855dbf 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -757,9 +757,9 @@ mempool_cache_init(struct rte_mempool_cache *cache,
> > uint32_t size)
> >  }
> > 
> >  /*
> > - * Create and initialize a cache for objects that are retrieved from and
> > - * returned to an underlying mempool. This structure is identical to the
> > - * local_cache[lcore_id] pointed to by the mempool structure.
> > + * Create and initialize a user owned cache for objects that are retrieved
> > + * from and returned to an underlying mempool. This structure is identical
> > + * to the local_cache[lcore_id] pointed to by the mempool structure.
> >   */
> >  struct rte_mempool_cache *
> >  rte_mempool_cache_create(uint32_t size, int socket_id)
> > @@ -786,9 +786,9 @@ rte_mempool_cache_create(uint32_t size, int socket_id)
> >  }
> > 
> >  /*
> > - * Free a cache. It's the responsibility of the user to make sure that any
> > - * remaining objects in the cache are flushed to the corresponding
> > - * mempool.
> > + * Free a user owned cache. It's the responsibility of the user to make
> > + * sure that any remaining objects in the cache are flushed to the
> > + * corresponding mempool.
> >   */
> >  void
> >  rte_mempool_cache_free(struct rte_mempool_cache *cache)
> > --
> > 2.25.1
> >   
> 
> These comments should probably be updated in the function descriptions in rte_mempool.h too, so they go into the auto-generated documentation.
> 

Agree with Morten on this.
The comments in rte_mempool.h should be updated, and the comments in rte_mempool.c are redundant and can be removed.
Best to have docbook comment as the part which describes the function.

[PATCH 3/4] eal: add API to check if lcore id is valid

[Honnappa Nagarahalli]

Simple API to check if the lcore ID does not exceed the
maximum number of lcores configured.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/eal/include/rte_lcore.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 6a355e9986..cf99919a02 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -38,6 +38,20 @@ enum rte_lcore_role_t {
 	ROLE_NON_EAL,
 };
 
+/**
+ * Check if the lcore ID is valid
+ *
+ * @param lcore_id
+ *   The identifier of the lcore.
+ *
+ * @return
+ *   True if the given lcore ID is between 0 and RTE_MAX_LCORE-1.
+ */
+static inline int rte_lcore_id_is_valid(unsigned int lcore_id)
+{
+	return (lcore_id < RTE_MAX_LCORE);
+}
+
 /**
  * Get a lcore's role.
  *
-- 
2.25.1

RE: [PATCH 3/4] eal: add API to check if lcore id is valid

[Morten Brørup]

> From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> Sent: Thursday, 9 March 2023 05.58
> 
> Simple API to check if the lcore ID does not exceed the
> maximum number of lcores configured.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  lib/eal/include/rte_lcore.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
> index 6a355e9986..cf99919a02 100644
> --- a/lib/eal/include/rte_lcore.h
> +++ b/lib/eal/include/rte_lcore.h
> @@ -38,6 +38,20 @@ enum rte_lcore_role_t {
>  	ROLE_NON_EAL,
>  };
> 
> +/**
> + * Check if the lcore ID is valid
> + *
> + * @param lcore_id
> + *   The identifier of the lcore.
> + *
> + * @return
> + *   True if the given lcore ID is between 0 and RTE_MAX_LCORE-1.
> + */
> +static inline int rte_lcore_id_is_valid(unsigned int lcore_id)
> +{
> +	return (lcore_id < RTE_MAX_LCORE);
> +}
> +
>  /**
>   * Get a lcore's role.
>   *
> --
> 2.25.1
> 

Isn't LCORE_ID_ANY considered valid in some contexts?

I don't think this function adds any value; it only makes the lcore_id interpretation more opaque. Having this comparison, i.e. (lcore_id < RTE_MAX_LCORE), directly in the source code makes it more easily readable than a call to this function.

So, NAK from me.

Re: [PATCH 3/4] eal: add API to check if lcore id is valid

[Stephen Hemminger]

On Wed, 7 Jun 2023 12:19:00 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Honnappa Nagarahalli [mailto:honnappa.nagarahalli@arm.com]
> > Sent: Thursday, 9 March 2023 05.58
> > 
> > Simple API to check if the lcore ID does not exceed the
> > maximum number of lcores configured.
> > 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

What about cases where passed lcore set is sparse?
In that case, lcore would not be valid in API calls.

[PATCH 4/4] mempool: use lcore API to check if lcore ID is valid

[Honnappa Nagarahalli]

Use lcore API to check if the lcore ID is valid. The runtime
check does not add much value. Hence use assert to validate
the lcore ID.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/mempool/rte_mempool.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 009bd10215..00c5aa961b 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1314,10 +1314,9 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache);
 static __rte_always_inline struct rte_mempool_cache *
 rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
 {
-	if (mp->cache_size == 0)
-		return NULL;
+	RTE_ASSERT(rte_lcore_id_is_valid(lcore_id));
 
-	if (lcore_id >= RTE_MAX_LCORE)
+	if (mp->cache_size == 0)
 		return NULL;
 
 	rte_mempool_trace_default_cache(mp, lcore_id,
-- 
2.25.1

RE: [PATCH 4/4] mempool: use lcore API to check if lcore ID is valid

[Konstantin Ananyev]

> 
> Use lcore API to check if the lcore ID is valid. The runtime
> check does not add much value.

From my perspective it adds a perfect value:
Only threads with valid lcore id have their own default mempool cache.

> Hence use assert to validate
> the lcore ID.

Wonder why?
What's wrong for the thread to try to get default mempool cache?
That would change existing behavior and in general seems wrong to me.  
So I am strongly opposed.
 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  lib/mempool/rte_mempool.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 009bd10215..00c5aa961b 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -1314,10 +1314,9 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache);
>  static __rte_always_inline struct rte_mempool_cache *
>  rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
>  {
> -	if (mp->cache_size == 0)
> -		return NULL;
> +	RTE_ASSERT(rte_lcore_id_is_valid(lcore_id));
> 
> -	if (lcore_id >= RTE_MAX_LCORE)
> +	if (mp->cache_size == 0)
>  		return NULL;
> 
>  	rte_mempool_trace_default_cache(mp, lcore_id,
> --
> 2.25.1
> 

RE: [PATCH 4/4] mempool: use lcore API to check if lcore ID is valid

[Honnappa Nagarahalli]

<snip>
> 
> 
> 
> >
> > Use lcore API to check if the lcore ID is valid. The runtime check
> > does not add much value.
> 
> From my perspective it adds a perfect value:
> Only threads with valid lcore id have their own default mempool cache.
The threads would call 'rte_lcore_id()' to return their lcore_id. This ensures the lcore_id is valid already. Why do we need to check it again in rte_mempool_default_cache? Why would a thread use an incorrect lcore_id?

> 
> > Hence use assert to validate
> > the lcore ID.
> 
> Wonder why?
> What's wrong for the thread to try to get default mempool cache?
What are the cases where a thread does not know that it is not an EAL thread and call rte_mempool_default_cache with a random lcore_id?
Since, this API is called in the data plane, it makes sense to remove any additional validations.

> That would change existing behavior and in general seems wrong to me.
Agree on the change in existing behavior. We can discuss this once we agree/disagree on the above.

> So I am strongly opposed.
> 
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >  lib/mempool/rte_mempool.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> > index 009bd10215..00c5aa961b 100644
> > --- a/lib/mempool/rte_mempool.h
> > +++ b/lib/mempool/rte_mempool.h
> > @@ -1314,10 +1314,9 @@ rte_mempool_cache_free(struct
> rte_mempool_cache
> > *cache);  static __rte_always_inline struct rte_mempool_cache *
> > rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
> > {
> > -	if (mp->cache_size == 0)
> > -		return NULL;
> > +	RTE_ASSERT(rte_lcore_id_is_valid(lcore_id));
> >
> > -	if (lcore_id >= RTE_MAX_LCORE)
> > +	if (mp->cache_size == 0)
> >  		return NULL;
> >
> >  	rte_mempool_trace_default_cache(mp, lcore_id,
> > --
> > 2.25.1
> >

RE: [PATCH 4/4] mempool: use lcore API to check if lcore ID is valid

[Konstantin Ananyev]

> 
> <snip>
> >
> >
> >
> > >
> > > Use lcore API to check if the lcore ID is valid. The runtime check
> > > does not add much value.
> >
> > From my perspective it adds a perfect value:
> > Only threads with valid lcore id have their own default mempool cache.
> The threads would call 'rte_lcore_id()' to return their lcore_id. This ensures the lcore_id is valid already.
> Why do we need to check it
> again in rte_mempool_default_cache? Why would a thread use an incorrect lcore_id?

rte_lcore_id() will just return you value of per-thread variable: RTE_PER_LCORE(_lcore_id).
Without any checking. For non-eal tthreads this value would be UINT32_MAX.
 
> >
> > > Hence use assert to validate
> > > the lcore ID.
> >
> > Wonder why?
> > What's wrong for the thread to try to get default mempool cache?
> What are the cases where a thread does not know that it is not an EAL thread and call rte_mempool_default_cache with a random
> lcore_id?
> Since, this API is called in the data plane, it makes sense to remove any additional validations.

Why is that?
I believe this API have to be generic and safe to call from any thread.
Let's look for example at:
*/
static __rte_always_inline void
rte_mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
                     unsigned int n)
{
        struct rte_mempool_cache *cache;
        cache = rte_mempool_default_cache(mp, rte_lcore_id());
        rte_mempool_trace_put_bulk(mp, obj_table, n, cache);
        rte_mempool_generic_put(mp, obj_table, n, cache);
} 

Right now it is perfectly valid to invoke it from any thread.
With the change you propose invoking mempool_put() from non-EAL thread
will cause either a crash or silent memory corruption.
 
> > That would change existing behavior and in general seems wrong to me.
> Agree on the change in existing behavior. We can discuss this once we agree/disagree on the above.
> 
> > So I am strongly opposed.
> >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > >  lib/mempool/rte_mempool.h | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> > > index 009bd10215..00c5aa961b 100644
> > > --- a/lib/mempool/rte_mempool.h
> > > +++ b/lib/mempool/rte_mempool.h
> > > @@ -1314,10 +1314,9 @@ rte_mempool_cache_free(struct
> > rte_mempool_cache
> > > *cache);  static __rte_always_inline struct rte_mempool_cache *
> > > rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id)
> > > {
> > > -	if (mp->cache_size == 0)
> > > -		return NULL;
> > > +	RTE_ASSERT(rte_lcore_id_is_valid(lcore_id));
> > >
> > > -	if (lcore_id >= RTE_MAX_LCORE)
> > > +	if (mp->cache_size == 0)
> > >  		return NULL;
> > >
> > >  	rte_mempool_trace_default_cache(mp, lcore_id,
> > > --
> > > 2.25.1
> > >

Re: [PATCH 0/4] Small corrections in mempool

[Thomas Monjalon]

Any update about this series?

09/03/2023 05:57, Honnappa Nagarahalli:
> 1) Patches 1/4, 2/4 - Few small corrections in mempool API documentation.
> 2) Patch 3/4 - The API for checking 'lcore ID is valid' is trivial,
>    but it is the right thing to do.
> 3) Patch 4/4 - IMO, the 'lcore ID is valid' check is not necessary to be
>    done during run time. If it is not valid, there is something seriously
>    wrong in the system already or it is a programming error. Given that
>    it is a data plane function, it makes sense to remove it from run
>    time check.
> 
> Honnappa Nagarahalli (4):
>   mempool: clarify mempool cache flush API behavior
>   mempool: clarify comments for mempool cache implementation
>   eal: add API to check if lcore id is valid
>   mempool: use lcore API to check if lcore ID is valid
> 
>  lib/eal/include/rte_lcore.h | 14 ++++++++++++++
>  lib/mempool/rte_mempool.c   | 12 ++++++------
>  lib/mempool/rte_mempool.h   | 13 ++++++++-----
>  3 files changed, 28 insertions(+), 11 deletions(-)