[PATCH] reorder: fix registration of dynamic field in mbuf

[PATCH] reorder: fix registration of dynamic field in mbuf

[Volodymyr Fialko]

It's possible to initialize reorder buffer with user allocated memory via
rte_reorder_init() function. In such case rte_reorder_create() not required
and reorder dynamic field in rte_mbuf will be not registered.

Fixes: 01f3496695b5 ("reorder: switch sequence number to dynamic mbuf field")

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 lib/reorder/rte_reorder.c | 40 ++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/lib/reorder/rte_reorder.c b/lib/reorder/rte_reorder.c
index 6e029c9e02..a759a9c434 100644
--- a/lib/reorder/rte_reorder.c
+++ b/lib/reorder/rte_reorder.c
@@ -54,6 +54,28 @@ struct rte_reorder_buffer {
 static void
 rte_reorder_free_mbufs(struct rte_reorder_buffer *b);
 
+static int
+rte_reorder_dynf_register(void)
+{
+	int ret;
+
+	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
+		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
+		.size = sizeof(rte_reorder_seqn_t),
+		.align = __alignof__(rte_reorder_seqn_t),
+	};
+
+	if (rte_reorder_seqn_dynfield_offset > 0)
+		return 0;
+
+	ret = rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
+	if (ret < 0)
+		return ret;
+	rte_reorder_seqn_dynfield_offset = ret;
+
+	return 0;
+}
+
 struct rte_reorder_buffer *
 rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
 		const char *name, unsigned int size)
@@ -85,6 +107,12 @@ rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
 		rte_errno = EINVAL;
 		return NULL;
 	}
+	if (rte_reorder_dynf_register()) {
+		RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence"
+				      " number\n");
+		rte_errno = ENOMEM;
+		return NULL;
+	}
 
 	memset(b, 0, bufsize);
 	strlcpy(b->name, name, sizeof(b->name));
@@ -106,11 +134,6 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
 	struct rte_reorder_list *reorder_list;
 	const unsigned int bufsize = sizeof(struct rte_reorder_buffer) +
 					(2 * size * sizeof(struct rte_mbuf *));
-	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
-		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
-		.size = sizeof(rte_reorder_seqn_t),
-		.align = __alignof__(rte_reorder_seqn_t),
-	};
 
 	reorder_list = RTE_TAILQ_CAST(rte_reorder_tailq.head, rte_reorder_list);
 
@@ -128,10 +151,9 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
 		return NULL;
 	}
 
-	rte_reorder_seqn_dynfield_offset =
-		rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
-	if (rte_reorder_seqn_dynfield_offset < 0) {
-		RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence number\n");
+	if (rte_reorder_dynf_register()) {
+		RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence"
+				      " number\n");
 		rte_errno = ENOMEM;
 		return NULL;
 	}
-- 
2.34.1

Re: [PATCH] reorder: fix registration of dynamic field in mbuf

[David Marchand]

Hello,

On Mon, Mar 13, 2023 at 10:35 AM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> It's possible to initialize reorder buffer with user allocated memory via
> rte_reorder_init() function. In such case rte_reorder_create() not required
> and reorder dynamic field in rte_mbuf will be not registered.

Good catch.


>
> Fixes: 01f3496695b5 ("reorder: switch sequence number to dynamic mbuf field")

It seems worth backporting.
Cc: stable@dpdk.org

>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
>  lib/reorder/rte_reorder.c | 40 ++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/lib/reorder/rte_reorder.c b/lib/reorder/rte_reorder.c
> index 6e029c9e02..a759a9c434 100644
> --- a/lib/reorder/rte_reorder.c
> +++ b/lib/reorder/rte_reorder.c
> @@ -54,6 +54,28 @@ struct rte_reorder_buffer {
>  static void
>  rte_reorder_free_mbufs(struct rte_reorder_buffer *b);
>
> +static int
> +rte_reorder_dynf_register(void)
> +{
> +       int ret;
> +
> +       static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
> +               .name = RTE_REORDER_SEQN_DYNFIELD_NAME,
> +               .size = sizeof(rte_reorder_seqn_t),
> +               .align = __alignof__(rte_reorder_seqn_t),
> +       };
> +
> +       if (rte_reorder_seqn_dynfield_offset > 0)
> +               return 0;
> +
> +       ret = rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
> +       if (ret < 0)
> +               return ret;
> +       rte_reorder_seqn_dynfield_offset = ret;
> +
> +       return 0;
> +}

We don't need this helper (see my comment below, for
rte_reorder_create), you can simply move this block to
rte_reorder_init().


> +
>  struct rte_reorder_buffer *
>  rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
>                 const char *name, unsigned int size)
> @@ -85,6 +107,12 @@ rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
>                 rte_errno = EINVAL;
>                 return NULL;
>         }
> +       if (rte_reorder_dynf_register()) {
> +               RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence"
> +                                     " number\n");
> +               rte_errno = ENOMEM;

I think returning this new errno code is fine from a ABI pov.
An application would have to check for NULL return code in any case
and can't act differently based on rte_errno value.

However, this is a small change to the rte_reorder_init API, so it
needs some update, see:

 * @return
 *   The initialized reorder buffer instance, or NULL on error
 *   On error case, rte_errno will be set appropriately:
 *    - EINVAL - invalid parameters



> +               return NULL;
> +       }
>
>         memset(b, 0, bufsize);
>         strlcpy(b->name, name, sizeof(b->name));
> @@ -106,11 +134,6 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
>         struct rte_reorder_list *reorder_list;
>         const unsigned int bufsize = sizeof(struct rte_reorder_buffer) +
>                                         (2 * size * sizeof(struct rte_mbuf *));
> -       static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
> -               .name = RTE_REORDER_SEQN_DYNFIELD_NAME,
> -               .size = sizeof(rte_reorder_seqn_t),
> -               .align = __alignof__(rte_reorder_seqn_t),
> -       };
>
>         reorder_list = RTE_TAILQ_CAST(rte_reorder_tailq.head, rte_reorder_list);
>
> @@ -128,10 +151,9 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
>                 return NULL;
>         }
>
> -       rte_reorder_seqn_dynfield_offset =
> -               rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
> -       if (rte_reorder_seqn_dynfield_offset < 0) {
> -               RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence number\n");
> +       if (rte_reorder_dynf_register()) {
> +               RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence"
> +                                     " number\n");

All rte_reorder_buffer objects need to go through rte_reorder_init().
You can check rte_reorder_init() return code.


>                 rte_errno = ENOMEM;
>                 return NULL;
>         }
> --
> 2.34.1
>


-- 
David Marchand

RE: [EXT] Re: [PATCH] reorder: fix registration of dynamic field in mbuf

[Volodymyr Fialko]

> All rte_reorder_buffer objects need to go through rte_reorder_init().
> You can check rte_reorder_init() return code.
Hi David,
I agree with all comments, however there is one catch with locks.
Both reorder lib and mbuf dynamic field are using `rte_mcfg_tailq` read/write lock for synchronization, so
I moved alloc+init before lock+insert to avoid deadlocking in v2. Let me know if you see any issues with this approach.

[PATCH v2] reorder: fix registration of dynamic field in mbuf

[Volodymyr Fialko]

It's possible to initialize reorder buffer with user allocated memory via
rte_reorder_init() function. In such case rte_reorder_create() is not
required and reorder dynamic field in rte_mbuf will not be registered.

Both reorder lib and mbuf dynamic field are using `rte_mcfg_tailq`
read/write lock for synchronization, to avoid deadlocking move reorder
buffer initialization before queue insertion.

Fixes: 01f3496695b5 ("reorder: switch sequence number to dynamic mbuf field")
Cc: stable@dpdk.org

Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
---
 lib/reorder/rte_reorder.c | 94 +++++++++++++++++++++++++--------------
 lib/reorder/rte_reorder.h |  1 +
 2 files changed, 61 insertions(+), 34 deletions(-)

diff --git a/lib/reorder/rte_reorder.c b/lib/reorder/rte_reorder.c
index 6e029c9e02..66d2cc07b7 100644
--- a/lib/reorder/rte_reorder.c
+++ b/lib/reorder/rte_reorder.c
@@ -60,6 +60,11 @@ rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
 {
 	const unsigned int min_bufsize = sizeof(*b) +
 					(2 * size * sizeof(struct rte_mbuf *));
+	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
+		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
+		.size = sizeof(rte_reorder_seqn_t),
+		.align = __alignof__(rte_reorder_seqn_t),
+	};
 
 	if (b == NULL) {
 		RTE_LOG(ERR, REORDER, "Invalid reorder buffer parameter:"
@@ -86,6 +91,14 @@ rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
 		return NULL;
 	}
 
+	rte_reorder_seqn_dynfield_offset = rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
+	if (rte_reorder_seqn_dynfield_offset < 0) {
+		RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence"
+				      " number, rte_errno: %i\n", rte_errno);
+		rte_errno = ENOMEM;
+		return NULL;
+	}
+
 	memset(b, 0, bufsize);
 	strlcpy(b->name, name, sizeof(b->name));
 	b->memsize = bufsize;
@@ -98,21 +111,45 @@ rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
 	return b;
 }
 
+/*
+ * Insert new entry into global list.
+ * Returns pointer to already inserted entry if such exists, or to newly inserted one.
+ */
+static struct rte_tailq_entry*
+rte_reorder_entry_insert(struct rte_tailq_entry *new_te)
+{
+	struct rte_reorder_list *reorder_list;
+	struct rte_reorder_buffer *b, *nb;
+	struct rte_tailq_entry *te;
+
+	rte_mcfg_tailq_write_lock();
+
+	reorder_list = RTE_TAILQ_CAST(rte_reorder_tailq.head, rte_reorder_list);
+	/* guarantee there's no existing */
+	TAILQ_FOREACH(te, reorder_list, next) {
+		b = (struct rte_reorder_buffer *) te->data;
+		nb = (struct rte_reorder_buffer *) new_te->data;
+		if (strncmp(nb->name, b->name, RTE_REORDER_NAMESIZE) == 0)
+			break;
+	}
+
+	if (te == NULL) {
+		TAILQ_INSERT_TAIL(reorder_list, new_te, next);
+		te = new_te;
+	}
+
+	rte_mcfg_tailq_write_unlock();
+
+	return te;
+}
+
 struct rte_reorder_buffer*
 rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
 {
 	struct rte_reorder_buffer *b = NULL;
-	struct rte_tailq_entry *te;
-	struct rte_reorder_list *reorder_list;
+	struct rte_tailq_entry *te, *te_inserted;
 	const unsigned int bufsize = sizeof(struct rte_reorder_buffer) +
 					(2 * size * sizeof(struct rte_mbuf *));
-	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
-		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
-		.size = sizeof(rte_reorder_seqn_t),
-		.align = __alignof__(rte_reorder_seqn_t),
-	};
-
-	reorder_list = RTE_TAILQ_CAST(rte_reorder_tailq.head, rte_reorder_list);
 
 	/* Check user arguments. */
 	if (!rte_is_power_of_2(size)) {
@@ -128,32 +165,12 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
 		return NULL;
 	}
 
-	rte_reorder_seqn_dynfield_offset =
-		rte_mbuf_dynfield_register(&reorder_seqn_dynfield_desc);
-	if (rte_reorder_seqn_dynfield_offset < 0) {
-		RTE_LOG(ERR, REORDER, "Failed to register mbuf field for reorder sequence number\n");
-		rte_errno = ENOMEM;
-		return NULL;
-	}
-
-	rte_mcfg_tailq_write_lock();
-
-	/* guarantee there's no existing */
-	TAILQ_FOREACH(te, reorder_list, next) {
-		b = (struct rte_reorder_buffer *) te->data;
-		if (strncmp(name, b->name, RTE_REORDER_NAMESIZE) == 0)
-			break;
-	}
-	if (te != NULL)
-		goto exit;
-
 	/* allocate tailq entry */
 	te = rte_zmalloc("REORDER_TAILQ_ENTRY", sizeof(*te), 0);
 	if (te == NULL) {
 		RTE_LOG(ERR, REORDER, "Failed to allocate tailq entry\n");
 		rte_errno = ENOMEM;
-		b = NULL;
-		goto exit;
+		return NULL;
 	}
 
 	/* Allocate memory to store the reorder buffer structure. */
@@ -162,14 +179,23 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size)
 		RTE_LOG(ERR, REORDER, "Memzone allocation failed\n");
 		rte_errno = ENOMEM;
 		rte_free(te);
+		return NULL;
 	} else {
-		rte_reorder_init(b, bufsize, name, size);
+		if (rte_reorder_init(b, bufsize, name, size) == NULL) {
+			rte_free(b);
+			rte_free(te);
+			return NULL;
+		}
 		te->data = (void *)b;
-		TAILQ_INSERT_TAIL(reorder_list, te, next);
 	}
 
-exit:
-	rte_mcfg_tailq_write_unlock();
+	te_inserted = rte_reorder_entry_insert(te);
+	if (te_inserted != te) {
+		rte_free(b);
+		rte_free(te);
+		return te_inserted->data;
+	}
+
 	return b;
 }
 
diff --git a/lib/reorder/rte_reorder.h b/lib/reorder/rte_reorder.h
index cc95015fa6..9a5998f2f6 100644
--- a/lib/reorder/rte_reorder.h
+++ b/lib/reorder/rte_reorder.h
@@ -82,6 +82,7 @@ rte_reorder_create(const char *name, unsigned socket_id, unsigned int size);
  *   The initialized reorder buffer instance, or NULL on error
  *   On error case, rte_errno will be set appropriately:
  *    - EINVAL - invalid parameters
+ *    - ENOMEM - not enough memory to register dynamic field
  */
 struct rte_reorder_buffer *
 rte_reorder_init(struct rte_reorder_buffer *b, unsigned int bufsize,
-- 
2.34.1

RE: [PATCH v2] reorder: fix registration of dynamic field in mbuf

[Volodymyr Fialko]

Hi,

A gentle reminder, please review and ack/comment.
Can we have this merged before RC3?

> -----Original Message-----
> From: Volodymyr Fialko <vfialko@marvell.com>
> Sent: Monday, March 13, 2023 2:04 PM
> To: dev@dpdk.org; Reshma Pattan <reshma.pattan@intel.com>; David Marchand
> <david.marchand@redhat.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Anoob Joseph <anoobj@marvell.com>; Volodymyr
> Fialko <vfialko@marvell.com>; stable@dpdk.org
> Subject: [PATCH v2] reorder: fix registration of dynamic field in mbuf
> 
> It's possible to initialize reorder buffer with user allocated memory via
> rte_reorder_init() function. In such case rte_reorder_create() is not required and reorder dynamic field in
> rte_mbuf will not be registered.
> 
> Both reorder lib and mbuf dynamic field are using `rte_mcfg_tailq` read/write lock for synchronization, to
> avoid deadlocking move reorder buffer initialization before queue insertion.
> 
> Fixes: 01f3496695b5 ("reorder: switch sequence number to dynamic mbuf field")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>
> ---
>  lib/reorder/rte_reorder.c | 94 +++++++++++++++++++++++++--------------
>  lib/reorder/rte_reorder.h |  1 +
>  2 files changed, 61 insertions(+), 34 deletions(-)
<snip>

Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf

[David Marchand]

On Wed, Mar 15, 2023 at 4:29 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> Hi,
>
> A gentle reminder, please review and ack/comment.
> Can we have this merged before RC3?

I did not go into depth this time, the fix seems ok.
Reshma, please review.


-- 
David Marchand

RE: [PATCH v2] reorder: fix registration of dynamic field in mbuf

[Pattan, Reshma]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> > A gentle reminder, please review and ack/comment.
> > Can we have this merged before RC3?
> 
> I did not go into depth this time, the fix seems ok.
> Reshma, please review.
> 
>

Hi David,

Unfortunately do not have a bandwidth to review this. I am ok if someone who already reviewed ACKs this.

Thanks,
Reshma

Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf

[David Marchand]

On Wed, Mar 15, 2023 at 4:47 PM Pattan, Reshma <reshma.pattan@intel.com> wrote:
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > > A gentle reminder, please review and ack/comment.
> > > Can we have this merged before RC3?
> >
> > I did not go into depth this time, the fix seems ok.
> > Reshma, please review.
> Unfortunately do not have a bandwidth to review this. I am ok if someone who already reviewed ACKs this.

That's a pity.

I don't think there is a huge amount of work on this library.
I would have expected one review every other month or so is acceptable.

The fix looks correct to me, nobody objects, I'll take it as is.


-- 
David Marchand

Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf

[David Marchand]

On Mon, Mar 13, 2023 at 2:05 PM Volodymyr Fialko <vfialko@marvell.com> wrote:
>
> It's possible to initialize reorder buffer with user allocated memory via
> rte_reorder_init() function. In such case rte_reorder_create() is not
> required and reorder dynamic field in rte_mbuf will not be registered.
>
> Both reorder lib and mbuf dynamic field are using `rte_mcfg_tailq`
> read/write lock for synchronization, to avoid deadlocking move reorder
> buffer initialization before queue insertion.
>
> Fixes: 01f3496695b5 ("reorder: switch sequence number to dynamic mbuf field")
> Cc: stable@dpdk.org
>
> Signed-off-by: Volodymyr Fialko <vfialko@marvell.com>

Applied, thanks Volodymyr.


-- 
David Marchand

Re: [PATCH] reorder: fix registration of dynamic field in mbuf

[Stephen Hemminger]

On Mon, 13 Mar 2023 10:34:50 +0100
Volodymyr Fialko <vfialko@marvell.com> wrote:

> +rte_reorder_dynf_register(void)
> +{
> +	int ret;
> +
> +	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
> +		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
> +		.size = sizeof(rte_reorder_seqn_t),
> +		.align = __alignof__(rte_reorder_seqn_t),
> +	};
> +

This does not need to be static, can just be on stack variable.

RE: [EXT] Re: [PATCH] reorder: fix registration of dynamic field in mbuf

[Volodymyr Fialko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, March 13, 2023 4:51 PM
> To: Volodymyr Fialko <vfialko@marvell.com>
> Cc: dev@dpdk.org; Reshma Pattan <reshma.pattan@intel.com>; David Marchand
> <david.marchand@redhat.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Anoob Joseph <anoobj@marvell.com>
> Subject: [EXT] Re: [PATCH] reorder: fix registration of dynamic field in mbuf
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Mon, 13 Mar 2023 10:34:50 +0100
> Volodymyr Fialko <vfialko@marvell.com> wrote:
> 
> > +rte_reorder_dynf_register(void)
> > +{
> > +	int ret;
> > +
> > +	static const struct rte_mbuf_dynfield reorder_seqn_dynfield_desc = {
> > +		.name = RTE_REORDER_SEQN_DYNFIELD_NAME,
> > +		.size = sizeof(rte_reorder_seqn_t),
> > +		.align = __alignof__(rte_reorder_seqn_t),
> > +	};
> > +
> 
> This does not need to be static, can just be on stack variable.

I agree, static is unnecessary here since the parameters will be copied to the internal storage during the register call.
So it can be on stack, but static sort of indicates/hints that this is one time initialization.
Also, static is present near every dynfield register in this codebase.
In fact, this patch simply moved the dynamic field declaration from create() to init() - static was already present.
Therefore, if the maintainers decide to remove static, it should be done in all other places too, it's not in scope of this patch.