DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] mempool: accept user flags only
@ 2021-10-18  8:26 David Marchand
  2021-10-18  8:35 ` Olivier Matz
  2021-10-18  8:37 ` Andrew Rybchenko
  0 siblings, 2 replies; 10+ messages in thread
From: David Marchand @ 2021-10-18  8:26 UTC (permalink / raw)
  To: dev
  Cc: Dmitry Kozlyuk, Olivier Matz, Andrew Rybchenko, Ray Kinsella,
	Stephen Hemminger

As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
internally.
This flag is not supposed to be requested from an application and would
probably result in an incorrect behavior if an application did pass it.

Other internal flags may be introduced later.

Rework the check and export a mask of valid user flags for use in the
unit test.

Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")

Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_mempool.c   |  8 ++++----
 lib/mempool/rte_mempool.c | 11 ++---------
 lib/mempool/rte_mempool.h |  9 +++++++++
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
index 66bc8d86b7..ba05742f76 100644
--- a/app/test/test_mempool.c
+++ b/app/test/test_mempool.c
@@ -205,15 +205,15 @@ static int test_mempool_creation_with_exceeded_cache_size(void)
 	return 0;
 }
 
-static int test_mempool_creation_with_unknown_flag(void)
+static int test_mempool_creation_with_invalid_flags(void)
 {
 	struct rte_mempool *mp_cov;
 
-	mp_cov = rte_mempool_create("test_mempool_unknown_flag", MEMPOOL_SIZE,
+	mp_cov = rte_mempool_create("test_mempool_invalid_flags", MEMPOOL_SIZE,
 		MEMPOOL_ELT_SIZE, 0, 0,
 		NULL, NULL,
 		NULL, NULL,
-		SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG << 1);
+		SOCKET_ID_ANY, ~MEMPOOL_VALID_USER_FLAGS);
 
 	if (mp_cov != NULL) {
 		rte_mempool_free(mp_cov);
@@ -653,7 +653,7 @@ test_mempool(void)
 	if (test_mempool_creation_with_exceeded_cache_size() < 0)
 		GOTO_ERR(ret, err);
 
-	if (test_mempool_creation_with_unknown_flag() < 0)
+	if (test_mempool_creation_with_invalid_flags() < 0)
 		GOTO_ERR(ret, err);
 
 	if (test_mempool_same_name_twice_creation() < 0)
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 607419ccaf..7f92d79c89 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -777,13 +777,6 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
 	rte_free(cache);
 }
 
-#define MEMPOOL_KNOWN_FLAGS (MEMPOOL_F_NO_SPREAD \
-	| MEMPOOL_F_NO_CACHE_ALIGN \
-	| MEMPOOL_F_SP_PUT \
-	| MEMPOOL_F_SC_GET \
-	| MEMPOOL_F_POOL_CREATED \
-	| MEMPOOL_F_NO_IOVA_CONTIG \
-	)
 /* create an empty mempool */
 struct rte_mempool *
 rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
@@ -828,8 +821,8 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
 		return NULL;
 	}
 
-	/* enforce no unknown flag is passed by the application */
-	if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
+	/* enforce only user flags are passed by the application */
+	if ((flags & ~MEMPOOL_VALID_USER_FLAGS) != 0) {
 		rte_errno = EINVAL;
 		return NULL;
 	}
diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 88bcbc51ef..d2bf2843f7 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -258,6 +258,15 @@ struct rte_mempool {
 #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
 #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
 
+/**
+ * This macro lists all the mempool flags an application may request.
+ */
+#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
+	| MEMPOOL_F_NO_CACHE_ALIGN \
+	| MEMPOOL_F_SP_PUT \
+	| MEMPOOL_F_SC_GET \
+	| MEMPOOL_F_NO_IOVA_CONTIG \
+	)
 /**
  * @internal When debug is enabled, store some statistics.
  *
-- 
2.23.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:26 [dpdk-dev] [PATCH] mempool: accept user flags only David Marchand
@ 2021-10-18  8:35 ` Olivier Matz
  2021-10-20  7:53   ` David Marchand
  2021-10-18  8:37 ` Andrew Rybchenko
  1 sibling, 1 reply; 10+ messages in thread
From: Olivier Matz @ 2021-10-18  8:35 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Dmitry Kozlyuk, Andrew Rybchenko, Ray Kinsella, Stephen Hemminger

On Mon, Oct 18, 2021 at 10:26:35AM +0200, David Marchand wrote:
> As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> internally.
> This flag is not supposed to be requested from an application and would
> probably result in an incorrect behavior if an application did pass it.
> 
> Other internal flags may be introduced later.
> 
> Rework the check and export a mask of valid user flags for use in the
> unit test.
> 
> Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> 
> Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:26 [dpdk-dev] [PATCH] mempool: accept user flags only David Marchand
  2021-10-18  8:35 ` Olivier Matz
@ 2021-10-18  8:37 ` Andrew Rybchenko
  2021-10-18  8:58   ` Olivier Matz
  2021-10-19 13:42   ` David Marchand
  1 sibling, 2 replies; 10+ messages in thread
From: Andrew Rybchenko @ 2021-10-18  8:37 UTC (permalink / raw)
  To: David Marchand, Olivier Matz
  Cc: Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On 10/18/21 11:26 AM, David Marchand wrote:
> As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> internally.
> This flag is not supposed to be requested from an application and would
> probably result in an incorrect behavior if an application did pass it.
> 
> Other internal flags may be introduced later.
> 
> Rework the check and export a mask of valid user flags for use in the
> unit test.
> 
> Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> 
> Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_mempool.c   |  8 ++++----
>  lib/mempool/rte_mempool.c | 11 ++---------
>  lib/mempool/rte_mempool.h |  9 +++++++++
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
> index 66bc8d86b7..ba05742f76 100644
> --- a/app/test/test_mempool.c
> +++ b/app/test/test_mempool.c
> @@ -205,15 +205,15 @@ static int test_mempool_creation_with_exceeded_cache_size(void)
>  	return 0;
>  }
>  
> -static int test_mempool_creation_with_unknown_flag(void)
> +static int test_mempool_creation_with_invalid_flags(void)
>  {
>  	struct rte_mempool *mp_cov;
>  
> -	mp_cov = rte_mempool_create("test_mempool_unknown_flag", MEMPOOL_SIZE,
> +	mp_cov = rte_mempool_create("test_mempool_invalid_flags", MEMPOOL_SIZE,
>  		MEMPOOL_ELT_SIZE, 0, 0,
>  		NULL, NULL,
>  		NULL, NULL,
> -		SOCKET_ID_ANY, MEMPOOL_F_NO_IOVA_CONTIG << 1);
> +		SOCKET_ID_ANY, ~MEMPOOL_VALID_USER_FLAGS);
>  
>  	if (mp_cov != NULL) {
>  		rte_mempool_free(mp_cov);
> @@ -653,7 +653,7 @@ test_mempool(void)
>  	if (test_mempool_creation_with_exceeded_cache_size() < 0)
>  		GOTO_ERR(ret, err);
>  
> -	if (test_mempool_creation_with_unknown_flag() < 0)
> +	if (test_mempool_creation_with_invalid_flags() < 0)
>  		GOTO_ERR(ret, err);
>  
>  	if (test_mempool_same_name_twice_creation() < 0)
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index 607419ccaf..7f92d79c89 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -777,13 +777,6 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
>  	rte_free(cache);
>  }
>  
> -#define MEMPOOL_KNOWN_FLAGS (MEMPOOL_F_NO_SPREAD \
> -	| MEMPOOL_F_NO_CACHE_ALIGN \
> -	| MEMPOOL_F_SP_PUT \
> -	| MEMPOOL_F_SC_GET \
> -	| MEMPOOL_F_POOL_CREATED \
> -	| MEMPOOL_F_NO_IOVA_CONTIG \
> -	)
>  /* create an empty mempool */
>  struct rte_mempool *
>  rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> @@ -828,8 +821,8 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
>  		return NULL;
>  	}
>  
> -	/* enforce no unknown flag is passed by the application */
> -	if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
> +	/* enforce only user flags are passed by the application */
> +	if ((flags & ~MEMPOOL_VALID_USER_FLAGS) != 0) {
>  		rte_errno = EINVAL;
>  		return NULL;
>  	}
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index 88bcbc51ef..d2bf2843f7 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -258,6 +258,15 @@ struct rte_mempool {
>  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
>  
> +/**
> + * This macro lists all the mempool flags an application may request.
> + */
> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \

I think RTE_ prefix is missing here since it is in a public
header now.

> +	| MEMPOOL_F_NO_CACHE_ALIGN \
> +	| MEMPOOL_F_SP_PUT \
> +	| MEMPOOL_F_SC_GET \
> +	| MEMPOOL_F_NO_IOVA_CONTIG \
> +	)
>  /**
>   * @internal When debug is enabled, store some statistics.
>   *
> 

Should we make a patch to add defines with RTE_ prefix and
add a deprecation for old flags without RTE_ prefix?

Olivier, what do you think? If you have time to care about it,
it would be great. (I'm unfamiliar with coccinelle yet).

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:37 ` Andrew Rybchenko
@ 2021-10-18  8:58   ` Olivier Matz
  2021-10-18  9:05     ` Andrew Rybchenko
  2021-10-19 13:42   ` David Marchand
  1 sibling, 1 reply; 10+ messages in thread
From: Olivier Matz @ 2021-10-18  8:58 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: David Marchand, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On Mon, Oct 18, 2021 at 11:37:32AM +0300, Andrew Rybchenko wrote:
> On 10/18/21 11:26 AM, David Marchand wrote:
> > As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> > internally.
> > This flag is not supposed to be requested from an application and would
> > probably result in an incorrect behavior if an application did pass it.
> > 
> > Other internal flags may be introduced later.
> > 
> > Rework the check and export a mask of valid user flags for use in the
> > unit test.
> > 
> > Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> > 
> > Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

(...)

> > --- a/lib/mempool/rte_mempool.h
> > +++ b/lib/mempool/rte_mempool.h
> > @@ -258,6 +258,15 @@ struct rte_mempool {
> >  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
> >  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
> >  
> > +/**
> > + * This macro lists all the mempool flags an application may request.
> > + */
> > +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
> 
> I think RTE_ prefix is missing here since it is in a public
> header now.

I discussed about this offline with David. I was ok to omit the
RTE_ prefix for consistency.

> > +	| MEMPOOL_F_NO_CACHE_ALIGN \
> > +	| MEMPOOL_F_SP_PUT \
> > +	| MEMPOOL_F_SC_GET \
> > +	| MEMPOOL_F_NO_IOVA_CONTIG \
> > +	)
> >  /**
> >   * @internal When debug is enabled, store some statistics.
> >   *
> > 
> 
> Should we make a patch to add defines with RTE_ prefix and
> add a deprecation for old flags without RTE_ prefix?
> 
> Olivier, what do you think? If you have time to care about it,
> it would be great. (I'm unfamiliar with coccinelle yet).

I was joking with David about keeping some work for 2022 :)

I will try to have a look. As a side note, I was not that convinced by
coccinelle for simple replacements like this, because it does not
update the comments or documentation, it can (rarely) break the
indentation, it is slow, and the syntax is quite complex.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:58   ` Olivier Matz
@ 2021-10-18  9:05     ` Andrew Rybchenko
  2021-10-18 12:06       ` David Marchand
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Rybchenko @ 2021-10-18  9:05 UTC (permalink / raw)
  To: Olivier Matz
  Cc: David Marchand, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger,
	dev, Thomas Monjalon

On 10/18/21 11:58 AM, Olivier Matz wrote:
> On Mon, Oct 18, 2021 at 11:37:32AM +0300, Andrew Rybchenko wrote:
>> On 10/18/21 11:26 AM, David Marchand wrote:
>>> As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
>>> internally.
>>> This flag is not supposed to be requested from an application and would
>>> probably result in an incorrect behavior if an application did pass it.
>>>
>>> Other internal flags may be introduced later.
>>>
>>> Rework the check and export a mask of valid user flags for use in the
>>> unit test.
>>>
>>> Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
>>>
>>> Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> 
> (...)
> 
>>> --- a/lib/mempool/rte_mempool.h
>>> +++ b/lib/mempool/rte_mempool.h
>>> @@ -258,6 +258,15 @@ struct rte_mempool {
>>>  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>>>  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
>>>  
>>> +/**
>>> + * This macro lists all the mempool flags an application may request.
>>> + */
>>> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
>>
>> I think RTE_ prefix is missing here since it is in a public
>> header now.
> 
> I discussed about this offline with David. I was ok to omit the
> RTE_ prefix for consistency.
> 
>>> +	| MEMPOOL_F_NO_CACHE_ALIGN \
>>> +	| MEMPOOL_F_SP_PUT \
>>> +	| MEMPOOL_F_SC_GET \
>>> +	| MEMPOOL_F_NO_IOVA_CONTIG \
>>> +	)
>>>  /**
>>>   * @internal When debug is enabled, store some statistics.
>>>   *
>>>
>>
>> Should we make a patch to add defines with RTE_ prefix and
>> add a deprecation for old flags without RTE_ prefix?
>>
>> Olivier, what do you think? If you have time to care about it,
>> it would be great. (I'm unfamiliar with coccinelle yet).
> 
> I was joking with David about keeping some work for 2022 :)
> 
> I will try to have a look. As a side note, I was not that convinced by
> coccinelle for simple replacements like this, because it does not
> update the comments or documentation, it can (rarely) break the
> indentation, it is slow, and the syntax is quite complex.
> 

I dreamed to finish with renanamings in core libraries in 2021.
May be it is to naive :(

I see that rc1 is already delayed and adding a couple of
more patches could delay it even more. I'm ready to do it
without coccinelle today, so, it is a question to @Thomas and
@David if that late patch will be considered at all.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  9:05     ` Andrew Rybchenko
@ 2021-10-18 12:06       ` David Marchand
  2021-10-18 14:51         ` Andrew Rybchenko
  0 siblings, 1 reply; 10+ messages in thread
From: David Marchand @ 2021-10-18 12:06 UTC (permalink / raw)
  To: Andrew Rybchenko, Thomas Monjalon
  Cc: Olivier Matz, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On Mon, Oct 18, 2021 at 11:05 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
> >>> +/**
> >>> + * This macro lists all the mempool flags an application may request.
> >>> + */
> >>> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
> >>
> >> I think RTE_ prefix is missing here since it is in a public
> >> header now.
> >
> > I discussed about this offline with David. I was ok to omit the
> > RTE_ prefix for consistency.
> >
> >>> +   | MEMPOOL_F_NO_CACHE_ALIGN \
> >>> +   | MEMPOOL_F_SP_PUT \
> >>> +   | MEMPOOL_F_SC_GET \
> >>> +   | MEMPOOL_F_NO_IOVA_CONTIG \
> >>> +   )
> >>>  /**
> >>>   * @internal When debug is enabled, store some statistics.
> >>>   *
> >>>
> >>
> >> Should we make a patch to add defines with RTE_ prefix and
> >> add a deprecation for old flags without RTE_ prefix?
> >>
> >> Olivier, what do you think? If you have time to care about it,
> >> it would be great. (I'm unfamiliar with coccinelle yet).
> >
> > I was joking with David about keeping some work for 2022 :)
> >
> > I will try to have a look. As a side note, I was not that convinced by
> > coccinelle for simple replacements like this, because it does not
> > update the comments or documentation, it can (rarely) break the
> > indentation, it is slow, and the syntax is quite complex.
> >
>
> I dreamed to finish with renanamings in core libraries in 2021.

Sadly, you are not the only one.


> May be it is to naive :(
>
> I see that rc1 is already delayed and adding a couple of
> more patches could delay it even more. I'm ready to do it
> without coccinelle today, so, it is a question to @Thomas and
> @David if that late patch will be considered at all.

Thomas?

As usual, we must avoid unannounced API changes unless waived by Techboard.
I am not against it if we keep compatilibity (marking RTE_DEPRECATED
can wait next releases), and seeing how it only impacts a relatively
low number of places in DPDK.

Risks are mainly on compilation side, and conflicts with other patches
in mempool for rc1, but I can only think of Dmitry series and the
conflict is easy to solve.

It should be quick to do: when could you send such a patch?


What I have seen:

$ git grep -iE "(define|inline).*\<(|__)mempool" -- lib/mempool/*.h
lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_SHIFT_MAX
(sizeof(uintptr_t) * CHAR_BIT - 1)
lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_NUM_DEFAULT  1
^^^
bit-rot, both can be dropped.

lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_SPREAD      0x0001
lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_CACHE_ALIGN 0x0002 /**<
Do not align objs on cache lines.*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_SP_PUT         0x0004 /**<
Default put is "single-producer".*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_SC_GET         0x0008 /**<
Default get is "single-consumer".*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_POOL_CREATED   0x0010 /**<
Internal: pool is created. */
lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**<
Don't need IOVA contiguous objs. */
lib/mempool/rte_mempool.h:#define MEMPOOL_VALID_USER_FLAGS
(MEMPOOL_F_NO_SPREAD \

lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {
                   \
lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
lib/mempool/rte_mempool.h:#define MEMPOOL_HEADER_SIZE(mp, cs) \
lib/mempool/rte_mempool.h:static inline struct rte_mempool_objhdr
*__mempool_get_header(void *obj)
lib/mempool/rte_mempool.h:static inline struct rte_mempool_objtlr
*__mempool_get_trailer(void *obj)
lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
obj_table_const, n, free) \
lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
obj_table_const, n, free) do {} while(0)
lib/mempool/rte_mempool.h:#define
__mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
lib/mempool/rte_mempool.h:#define
__mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
^^
Internal stuff, but I would prefix in any case.

lib/mempool/rte_mempool.h:#define MEMPOOL_REGISTER_OPS(ops)
                 \


-- 
David Marchand


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18 12:06       ` David Marchand
@ 2021-10-18 14:51         ` Andrew Rybchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Rybchenko @ 2021-10-18 14:51 UTC (permalink / raw)
  To: David Marchand, Thomas Monjalon
  Cc: Olivier Matz, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On 10/18/21 3:06 PM, David Marchand wrote:
> On Mon, Oct 18, 2021 at 11:05 AM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>> +/**
>>>>> + * This macro lists all the mempool flags an application may request.
>>>>> + */
>>>>> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
>>>>
>>>> I think RTE_ prefix is missing here since it is in a public
>>>> header now.
>>>
>>> I discussed about this offline with David. I was ok to omit the
>>> RTE_ prefix for consistency.
>>>
>>>>> +   | MEMPOOL_F_NO_CACHE_ALIGN \
>>>>> +   | MEMPOOL_F_SP_PUT \
>>>>> +   | MEMPOOL_F_SC_GET \
>>>>> +   | MEMPOOL_F_NO_IOVA_CONTIG \
>>>>> +   )
>>>>>  /**
>>>>>   * @internal When debug is enabled, store some statistics.
>>>>>   *
>>>>>
>>>>
>>>> Should we make a patch to add defines with RTE_ prefix and
>>>> add a deprecation for old flags without RTE_ prefix?
>>>>
>>>> Olivier, what do you think? If you have time to care about it,
>>>> it would be great. (I'm unfamiliar with coccinelle yet).
>>>
>>> I was joking with David about keeping some work for 2022 :)
>>>
>>> I will try to have a look. As a side note, I was not that convinced by
>>> coccinelle for simple replacements like this, because it does not
>>> update the comments or documentation, it can (rarely) break the
>>> indentation, it is slow, and the syntax is quite complex.
>>>
>>
>> I dreamed to finish with renanamings in core libraries in 2021.
> 
> Sadly, you are not the only one.
> 
> 
>> May be it is to naive :(
>>
>> I see that rc1 is already delayed and adding a couple of
>> more patches could delay it even more. I'm ready to do it
>> without coccinelle today, so, it is a question to @Thomas and
>> @David if that late patch will be considered at all.
> 
> Thomas?
> 
> As usual, we must avoid unannounced API changes unless waived by Techboard.
> I am not against it if we keep compatilibity (marking RTE_DEPRECATED
> can wait next releases), and seeing how it only impacts a relatively
> low number of places in DPDK.
> 
> Risks are mainly on compilation side, and conflicts with other patches
> in mempool for rc1, but I can only think of Dmitry series and the
> conflict is easy to solve.
> 
> It should be quick to do: when could you send such a patch?

Done.

> 
> 
> What I have seen:
> 
> $ git grep -iE "(define|inline).*\<(|__)mempool" -- lib/mempool/*.h
> lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_SHIFT_MAX
> (sizeof(uintptr_t) * CHAR_BIT - 1)
> lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_NUM_DEFAULT  1
> ^^^
> bit-rot, both can be dropped.
> 
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_SPREAD      0x0001
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_CACHE_ALIGN 0x0002 /**<
> Do not align objs on cache lines.*/
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_SP_PUT         0x0004 /**<
> Default put is "single-producer".*/
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_SC_GET         0x0008 /**<
> Default get is "single-consumer".*/
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_POOL_CREATED   0x0010 /**<
> Internal: pool is created. */
> lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**<
> Don't need IOVA contiguous objs. */
> lib/mempool/rte_mempool.h:#define MEMPOOL_VALID_USER_FLAGS
> (MEMPOOL_F_NO_SPREAD \
> 
> lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {
>                    \
> lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
> lib/mempool/rte_mempool.h:#define MEMPOOL_HEADER_SIZE(mp, cs) \
> lib/mempool/rte_mempool.h:static inline struct rte_mempool_objhdr
> *__mempool_get_header(void *obj)
> lib/mempool/rte_mempool.h:static inline struct rte_mempool_objtlr
> *__mempool_get_trailer(void *obj)
> lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
> obj_table_const, n, free) \
> lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
> obj_table_const, n, free) do {} while(0)
> lib/mempool/rte_mempool.h:#define
> __mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
> lib/mempool/rte_mempool.h:#define
> __mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
> ^^
> Internal stuff, but I would prefix in any case.
> 
> lib/mempool/rte_mempool.h:#define MEMPOOL_REGISTER_OPS(ops)
>                  \

Many thanks for the grep. I'd lost something without it.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:37 ` Andrew Rybchenko
  2021-10-18  8:58   ` Olivier Matz
@ 2021-10-19 13:42   ` David Marchand
  2021-10-19 18:17     ` Andrew Rybchenko
  1 sibling, 1 reply; 10+ messages in thread
From: David Marchand @ 2021-10-19 13:42 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Olivier Matz, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On Mon, Oct 18, 2021 at 10:37 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
> > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> > index 88bcbc51ef..d2bf2843f7 100644
> > --- a/lib/mempool/rte_mempool.h
> > +++ b/lib/mempool/rte_mempool.h
> > @@ -258,6 +258,15 @@ struct rte_mempool {
> >  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
> >  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
> >
> > +/**
> > + * This macro lists all the mempool flags an application may request.
> > + */
> > +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
>
> I think RTE_ prefix is missing here since it is in a public
> header now.
>
> > +     | MEMPOOL_F_NO_CACHE_ALIGN \
> > +     | MEMPOOL_F_SP_PUT \
> > +     | MEMPOOL_F_SC_GET \
> > +     | MEMPOOL_F_NO_IOVA_CONTIG \
> > +     )
> >  /**
> >   * @internal When debug is enabled, store some statistics.
> >   *
> >
>
> Should we make a patch to add defines with RTE_ prefix and
> add a deprecation for old flags without RTE_ prefix?
>
> Olivier, what do you think? If you have time to care about it,
> it would be great. (I'm unfamiliar with coccinelle yet).

Andrew,

RTE_ prefixing subject put aside, are you ok with this patch?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-19 13:42   ` David Marchand
@ 2021-10-19 18:17     ` Andrew Rybchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Rybchenko @ 2021-10-19 18:17 UTC (permalink / raw)
  To: David Marchand
  Cc: Olivier Matz, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger, dev

On 10/19/21 4:42 PM, David Marchand wrote:
> On Mon, Oct 18, 2021 at 10:37 AM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
>>> index 88bcbc51ef..d2bf2843f7 100644
>>> --- a/lib/mempool/rte_mempool.h
>>> +++ b/lib/mempool/rte_mempool.h
>>> @@ -258,6 +258,15 @@ struct rte_mempool {
>>>   #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>>>   #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
>>>
>>> +/**
>>> + * This macro lists all the mempool flags an application may request.
>>> + */
>>> +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
>>
>> I think RTE_ prefix is missing here since it is in a public
>> header now.
>>
>>> +     | MEMPOOL_F_NO_CACHE_ALIGN \
>>> +     | MEMPOOL_F_SP_PUT \
>>> +     | MEMPOOL_F_SC_GET \
>>> +     | MEMPOOL_F_NO_IOVA_CONTIG \
>>> +     )
>>>   /**
>>>    * @internal When debug is enabled, store some statistics.
>>>    *
>>>
>>
>> Should we make a patch to add defines with RTE_ prefix and
>> add a deprecation for old flags without RTE_ prefix?
>>
>> Olivier, what do you think? If you have time to care about it,
>> it would be great. (I'm unfamiliar with coccinelle yet).
> 
> Andrew,
> 
> RTE_ prefixing subject put aside, are you ok with this patch?
> 
> 

Yes,

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] mempool: accept user flags only
  2021-10-18  8:35 ` Olivier Matz
@ 2021-10-20  7:53   ` David Marchand
  0 siblings, 0 replies; 10+ messages in thread
From: David Marchand @ 2021-10-20  7:53 UTC (permalink / raw)
  To: Olivier Matz, Andrew Rybchenko
  Cc: dev, Dmitry Kozlyuk, Ray Kinsella, Stephen Hemminger

On Mon, Oct 18, 2021 at 10:36 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Mon, Oct 18, 2021 at 10:26:35AM +0200, David Marchand wrote:
> > As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> > internally.
> > This flag is not supposed to be requested from an application and would
> > probably result in an incorrect behavior if an application did pass it.
> >
> > Other internal flags may be introduced later.
> >
> > Rework the check and export a mask of valid user flags for use in the
> > unit test.
> >
> > Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> >
> > Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Rebased on top of mempool cleanup series, and applied.
Thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-10-20  7:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18  8:26 [dpdk-dev] [PATCH] mempool: accept user flags only David Marchand
2021-10-18  8:35 ` Olivier Matz
2021-10-20  7:53   ` David Marchand
2021-10-18  8:37 ` Andrew Rybchenko
2021-10-18  8:58   ` Olivier Matz
2021-10-18  9:05     ` Andrew Rybchenko
2021-10-18 12:06       ` David Marchand
2021-10-18 14:51         ` Andrew Rybchenko
2021-10-19 13:42   ` David Marchand
2021-10-19 18:17     ` Andrew Rybchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).