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

[David Marchand <david.marchand@redhat.com>]

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