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
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
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>
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
> -----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
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
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