* [PATCH] reorder: fix registration of dynamic field in mbuf
@ 2023-03-13 9:34 Volodymyr Fialko
2023-03-13 10:19 ` David Marchand
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-13 9:34 UTC (permalink / raw)
To: dev, Reshma Pattan, David Marchand, Andrew Rybchenko
Cc: jerinj, anoobj, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] reorder: fix registration of dynamic field in mbuf
2023-03-13 9:34 [PATCH] reorder: fix registration of dynamic field in mbuf Volodymyr Fialko
@ 2023-03-13 10:19 ` David Marchand
2023-03-13 13:08 ` [EXT] " Volodymyr Fialko
2023-03-13 13:04 ` [PATCH v2] " Volodymyr Fialko
2023-03-13 15:51 ` [PATCH] " Stephen Hemminger
2 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2023-03-13 10:19 UTC (permalink / raw)
To: Volodymyr Fialko, Reshma Pattan
Cc: dev, Andrew Rybchenko, jerinj, anoobj, Thomas Monjalon
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
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [EXT] Re: [PATCH] reorder: fix registration of dynamic field in mbuf
2023-03-13 10:19 ` David Marchand
@ 2023-03-13 13:08 ` Volodymyr Fialko
0 siblings, 0 replies; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-13 13:08 UTC (permalink / raw)
To: David Marchand, Reshma Pattan
Cc: dev, Andrew Rybchenko, Jerin Jacob Kollanukkaran, Anoob Joseph,
Thomas Monjalon
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-13 9:34 [PATCH] reorder: fix registration of dynamic field in mbuf Volodymyr Fialko
2023-03-13 10:19 ` David Marchand
@ 2023-03-13 13:04 ` Volodymyr Fialko
2023-03-15 15:29 ` Volodymyr Fialko
2023-03-16 15:55 ` David Marchand
2023-03-13 15:51 ` [PATCH] " Stephen Hemminger
2 siblings, 2 replies; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-13 13:04 UTC (permalink / raw)
To: dev, Reshma Pattan, David Marchand, Andrew Rybchenko
Cc: jerinj, anoobj, Volodymyr Fialko, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-13 13:04 ` [PATCH v2] " Volodymyr Fialko
@ 2023-03-15 15:29 ` Volodymyr Fialko
2023-03-15 15:43 ` David Marchand
2023-03-16 15:55 ` David Marchand
1 sibling, 1 reply; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-15 15:29 UTC (permalink / raw)
To: Volodymyr Fialko, dev, Reshma Pattan, David Marchand,
Andrew Rybchenko, Stephen Hemminger
Cc: Jerin Jacob Kollanukkaran, Anoob Joseph, stable
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>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-15 15:29 ` Volodymyr Fialko
@ 2023-03-15 15:43 ` David Marchand
2023-03-15 15:46 ` Pattan, Reshma
0 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2023-03-15 15:43 UTC (permalink / raw)
To: Volodymyr Fialko, Reshma Pattan
Cc: dev, Andrew Rybchenko, Stephen Hemminger,
Jerin Jacob Kollanukkaran, Anoob Joseph, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-15 15:43 ` David Marchand
@ 2023-03-15 15:46 ` Pattan, Reshma
2023-03-16 15:36 ` David Marchand
0 siblings, 1 reply; 12+ messages in thread
From: Pattan, Reshma @ 2023-03-15 15:46 UTC (permalink / raw)
To: David Marchand, Volodymyr Fialko
Cc: dev, Andrew Rybchenko, Stephen Hemminger,
Jerin Jacob Kollanukkaran, Anoob Joseph, stable
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-15 15:46 ` Pattan, Reshma
@ 2023-03-16 15:36 ` David Marchand
0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2023-03-16 15:36 UTC (permalink / raw)
To: Pattan, Reshma
Cc: Volodymyr Fialko, dev, Andrew Rybchenko, Stephen Hemminger,
Jerin Jacob Kollanukkaran, Anoob Joseph, stable, Mcnamara, John
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] reorder: fix registration of dynamic field in mbuf
2023-03-13 13:04 ` [PATCH v2] " Volodymyr Fialko
2023-03-15 15:29 ` Volodymyr Fialko
@ 2023-03-16 15:55 ` David Marchand
1 sibling, 0 replies; 12+ messages in thread
From: David Marchand @ 2023-03-16 15:55 UTC (permalink / raw)
To: Volodymyr Fialko
Cc: dev, Reshma Pattan, Andrew Rybchenko, jerinj, anoobj, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] reorder: fix registration of dynamic field in mbuf
2023-03-13 9:34 [PATCH] reorder: fix registration of dynamic field in mbuf Volodymyr Fialko
2023-03-13 10:19 ` David Marchand
2023-03-13 13:04 ` [PATCH v2] " Volodymyr Fialko
@ 2023-03-13 15:51 ` Stephen Hemminger
2023-03-13 17:29 ` [EXT] " Volodymyr Fialko
2 siblings, 1 reply; 12+ messages in thread
From: Stephen Hemminger @ 2023-03-13 15:51 UTC (permalink / raw)
To: Volodymyr Fialko
Cc: dev, Reshma Pattan, David Marchand, Andrew Rybchenko, jerinj, anoobj
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [EXT] Re: [PATCH] reorder: fix registration of dynamic field in mbuf
2023-03-13 15:51 ` [PATCH] " Stephen Hemminger
@ 2023-03-13 17:29 ` Volodymyr Fialko
0 siblings, 0 replies; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-13 17:29 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, Reshma Pattan, David Marchand, Andrew Rybchenko,
Jerin Jacob Kollanukkaran, Anoob Joseph
> -----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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] reorder: fix registration of dynamic field in mbuf
@ 2023-03-13 12:59 Volodymyr Fialko
0 siblings, 0 replies; 12+ messages in thread
From: Volodymyr Fialko @ 2023-03-13 12:59 UTC (permalink / raw)
To: dev, Reshma Pattan, David Marchand, Andrew Rybchenko
Cc: jerinj, anoobj, Volodymyr Fialko, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-16 15:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-13 9:34 [PATCH] reorder: fix registration of dynamic field in mbuf Volodymyr Fialko
2023-03-13 10:19 ` David Marchand
2023-03-13 13:08 ` [EXT] " Volodymyr Fialko
2023-03-13 13:04 ` [PATCH v2] " Volodymyr Fialko
2023-03-15 15:29 ` Volodymyr Fialko
2023-03-15 15:43 ` David Marchand
2023-03-15 15:46 ` Pattan, Reshma
2023-03-16 15:36 ` David Marchand
2023-03-16 15:55 ` David Marchand
2023-03-13 15:51 ` [PATCH] " Stephen Hemminger
2023-03-13 17:29 ` [EXT] " Volodymyr Fialko
2023-03-13 12:59 [PATCH v2] " Volodymyr Fialko
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).