Re: [dpdk-dev] [PATCH v8 1/3] lib/lpm: integrate RCU QSBR

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

Hello Ruifeng,

On Thu, Jul 9, 2020 at 10:03 AM Ruifeng Wang <ruifeng.wang@arm.com> wrote:
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index 38ab512a4..4fbf5b6df 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2010-2014 Intel Corporation
> + * Copyright(c) 2020 Arm Limited
>   */
>
>  #include <string.h>
> @@ -39,6 +40,17 @@ enum valid_flag {
>         VALID
>  };
>
> +/** @internal LPM structure. */
> +struct __rte_lpm {
> +       /* LPM metadata. */
> +       struct rte_lpm lpm;
> +
> +       /* RCU config. */
> +       struct rte_rcu_qsbr *v;         /* RCU QSBR variable. */
> +       enum rte_lpm_qsbr_mode rcu_mode;/* Blocking, defer queue. */
> +       struct rte_rcu_qsbr_dq *dq;     /* RCU QSBR defer queue. */
> +};
> +
>  /* Macro to enable/disable run-time checks. */
>  #if defined(RTE_LIBRTE_LPM_DEBUG)
>  #include <rte_debug.h>
> @@ -122,6 +134,7 @@ rte_lpm_create(const char *name, int socket_id,
>                 const struct rte_lpm_config *config)
>  {
>         char mem_name[RTE_LPM_NAMESIZE];
> +       struct __rte_lpm *internal_lpm = NULL;

Nit: internal_lpm does not need to be initialised to NULL.


>         struct rte_lpm *lpm = NULL;
>         struct rte_tailq_entry *te;
>         uint32_t mem_size, rules_size, tbl8s_size;
> @@ -140,12 +153,6 @@ rte_lpm_create(const char *name, int socket_id,
>
>         snprintf(mem_name, sizeof(mem_name), "LPM_%s", name);
>
> -       /* Determine the amount of memory to allocate. */
> -       mem_size = sizeof(*lpm);
> -       rules_size = sizeof(struct rte_lpm_rule) * config->max_rules;
> -       tbl8s_size = (sizeof(struct rte_lpm_tbl_entry) *
> -                       RTE_LPM_TBL8_GROUP_NUM_ENTRIES * config->number_tbl8s);
> -
>         rte_mcfg_tailq_write_lock();
>
>         /* guarantee there's no existing */
> @@ -161,6 +168,12 @@ rte_lpm_create(const char *name, int socket_id,
>                 goto exit;
>         }
>
> +       /* Determine the amount of memory to allocate. */
> +       mem_size = sizeof(*internal_lpm);
> +       rules_size = sizeof(struct rte_lpm_rule) * config->max_rules;
> +       tbl8s_size = (sizeof(struct rte_lpm_tbl_entry) *
> +                       RTE_LPM_TBL8_GROUP_NUM_ENTRIES * config->number_tbl8s);
> +
>         /* allocate tailq entry */
>         te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
>         if (te == NULL) {
> @@ -170,22 +183,23 @@ rte_lpm_create(const char *name, int socket_id,
>         }
>
>         /* Allocate memory to store the LPM data structures. */
> -       lpm = rte_zmalloc_socket(mem_name, mem_size,
> +       internal_lpm = rte_zmalloc_socket(mem_name, mem_size,
>                         RTE_CACHE_LINE_SIZE, socket_id);
> -       if (lpm == NULL) {
> +       if (internal_lpm == NULL) {
>                 RTE_LOG(ERR, LPM, "LPM memory allocation failed\n");
>                 rte_free(te);
>                 rte_errno = ENOMEM;
>                 goto exit;
>         }
>
> +       lpm = &internal_lpm->lpm;

From this point...

>         lpm->rules_tbl = rte_zmalloc_socket(NULL,
>                         (size_t)rules_size, RTE_CACHE_LINE_SIZE, socket_id);
>
>         if (lpm->rules_tbl == NULL) {
>                 RTE_LOG(ERR, LPM, "LPM rules_tbl memory allocation failed\n");
> -               rte_free(lpm);
> -               lpm = NULL;
> +               rte_free(internal_lpm);
> +               internal_lpm = NULL;

... lpm is set to &internal_lpm->lpm and will be returned by jumping
to the exit label.
So freeing internal_lpm is necessary, but the lpm variable must be set
to NULL too.


>                 rte_free(te);
>                 rte_errno = ENOMEM;
>                 goto exit;
> @@ -197,8 +211,8 @@ rte_lpm_create(const char *name, int socket_id,
>         if (lpm->tbl8 == NULL) {
>                 RTE_LOG(ERR, LPM, "LPM tbl8 memory allocation failed\n");
>                 rte_free(lpm->rules_tbl);
> -               rte_free(lpm);
> -               lpm = NULL;
> +               rte_free(internal_lpm);
> +               internal_lpm = NULL;

Ditto.


>                 rte_free(te);
>                 rte_errno = ENOMEM;
>                 goto exit;

-- 
David Marchand