[v2] reorder: fix registration of dynamic field in mbuf
Checks
Commit Message
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(-)
Comments
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.
> -----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.
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.
@@ -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;
}
@@ -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,