[dpdk-dev] [PATCH v3] eal: remove non-thread panic calls from init sequence

David Marchand david.marchand at redhat.com
Mon Jun 3 10:14:40 CEST 2019


Hello,

Thanks for looking at this.

First round of review, there is still something fishy about mem_cfg_fd in
secondary process implementation for Linux to me.
Maybe Anatoly can review this patch as well.


On Mon, Jun 3, 2019 at 9:21 AM Arnon Warshavsky <arnon at qwilt.com> wrote:

> This patch changes some void functions to return a value,
> so that the init sequence may tear down orderly
> instead of calling panic.
>

The part below can go after a --- marker, this is more a comment for the
work in progress rather than something to put in this patch commitlog.

The calls for launching core messaging threads were left in tact
> in all 3 eal implementations.
> This should be addressed in a different patchset.
> There are still cases where the PMDs or message threads
> may call panic with no context for the user.
> For these I will submit a patch suggesting a callback registration,
> allowing the user to register a context to be called
> in case of a panic that takes place outside the init sequence.
>

Not sure I understand what you want to do, but at least this patch is a
first step.


> Signed-off-by: Arnon Warshavsky <arnon at qwilt.com>
> ---
>
> -v2 fix freebsd compilation
> -v3 fix freebsd compilation, haste from the devil
>
>  lib/librte_eal/freebsd/eal/eal.c | 61 +++++++++++++++++++--------
>  lib/librte_eal/linux/eal/eal.c   | 89
> ++++++++++++++++++++++++++++------------
>  2 files changed, 106 insertions(+), 44 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index c6ac902..04a1033 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -215,7 +215,7 @@ enum rte_iova_mode
>   * We also don't lock the whole file, so that in future we can use
> read-locks
>   * on other parts, e.g. memzones, to detect if there are running secondary
>   * processes. */
> -static void
> +static int
>  rte_eal_config_create(void)
>  {
>         void *rte_mem_cfg_addr;
> @@ -224,60 +224,77 @@ enum rte_iova_mode
>         const char *pathname = eal_runtime_config_path();
>
>         if (internal_config.no_shconf)
> -               return;
> +               return 0;
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       pathname);
>

Fix indent please, and in subsequent uses of RTE_LOG in this patch.


+                       return -1;
> +               }
>         }
>
>         retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config));
>         if (retval < 0){
>                 close(mem_cfg_fd);
>

When we close this fd, we need to reset the variable pointing to it to -1
or subsequent call would reuse an incorrect mem_cfg_fd.
The problem is general in both linux and freebsd eal.c but something is
still not clear to me in how we are supposed to keep this fd opened or not
in normal successfull init case.


-               rte_panic("Cannot resize '%s' for rte_mem_config\n",
> pathname);
> +               RTE_LOG(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                               pathname);
>
+               return -1;
>         }
>
>         retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock);
>         if (retval < 0){
>                 close(mem_cfg_fd);
> -               rte_exit(EXIT_FAILURE, "Cannot create lock on '%s'. Is
> another primary "
> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
>                                 "process running?\n", pathname);
>
+               return -1;
>         }
>
>         rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
>                                 PROT_READ | PROT_WRITE, MAP_SHARED,
> mem_cfg_fd, 0);
>
>         if (rte_mem_cfg_addr == MAP_FAILED){
> -               rte_panic("Cannot mmap memory for rte_config\n");
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>         }
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
> sizeof(early_mem_config));
>         rte_config.mem_config = rte_mem_cfg_addr;
> +
> +       return 0;
>  }
>
>  /* attach to an existing shared memory config */
> -static void
> +static int
>  rte_eal_config_attach(void)
>  {
>         void *rte_mem_cfg_addr;
>         const char *pathname = eal_runtime_config_path();
>
>         if (internal_config.no_shconf)
> -               return;
> +               return 0;
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       pathname);
>
+                       return -1;
> +               }
>         }
>
>         rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
>                                 PROT_READ | PROT_WRITE, MAP_SHARED,
> mem_cfg_fd, 0);
>         close(mem_cfg_fd);
> -       if (rte_mem_cfg_addr == MAP_FAILED)
> -               rte_panic("Cannot mmap memory for rte_config\n");
> +       if (rte_mem_cfg_addr == MAP_FAILED) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                               errno, strerror(errno));
>
+               return -1;
> +       }
>
>         rte_config.mem_config = rte_mem_cfg_addr;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -306,23 +323,33 @@ enum rte_proc_type_t
>  }
>
>  /* Sets up rte_config structure with the pointer to shared memory
> config.*/
> -static void
> +static int
>  rte_config_init(void)
>  {
>         rte_config.process_type = internal_config.process_type;
>
>         switch (rte_config.process_type){
>         case RTE_PROC_PRIMARY:
> -               rte_eal_config_create();
> +               if (rte_eal_config_create() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to create config\n");
>

All error paths in rte_eal_config_create() have a log.
Adding one more here won't help and we have another one coming with the
rte_eal_init_alert later.

This comment goes for linux changes as well.


+                       return -1;
> +               }
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to attach config\n");
>

Idem no log needed.


+                       return -1;
> +               }
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> -               rte_panic("Invalid process type\n");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                               rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* display usage */
> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 1613996..57abc4f 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -300,7 +300,7 @@ enum rte_iova_mode
>   * We also don't lock the whole file, so that in future we can use
> read-locks
>   * on other parts, e.g. memzones, to detect if there are running secondary
>   * processes. */
> -static void
> +static int
>  rte_eal_config_create(void)
>  {
>         void *rte_mem_cfg_addr;
> @@ -309,7 +309,7 @@ enum rte_iova_mode
>         const char *pathname = eal_runtime_config_path();
>
>         if (internal_config.no_shconf)
> -               return;
> +               return 0;
>
>         /* map the config before hugepage address so that we don't waste a
> page */
>         if (internal_config.base_virtaddr != 0)
> @@ -321,28 +321,35 @@ enum rte_iova_mode
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0600);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       pathname);
> +                       return -1;
> +               }
>         }
>
>         retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config));
>         if (retval < 0){
>                 close(mem_cfg_fd);
> -               rte_panic("Cannot resize '%s' for rte_mem_config\n",
> pathname);
> +               RTE_LOG(ERR, EAL, "Cannot resize '%s' for
> rte_mem_config\n",
> +                               pathname);
> +               return -1;
>         }
>
>         retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock);
>         if (retval < 0){
>                 close(mem_cfg_fd);
> -               rte_exit(EXIT_FAILURE, "Cannot create lock on '%s'. Is
> another primary "
> +               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another
> primary "
>                                 "process running?\n", pathname);
> +               return -1;
>         }
>
>         rte_mem_cfg_addr = mmap(rte_mem_cfg_addr,
> sizeof(*rte_config.mem_config),
>                                 PROT_READ | PROT_WRITE, MAP_SHARED,
> mem_cfg_fd, 0);
>
>         if (rte_mem_cfg_addr == MAP_FAILED){
> -               rte_panic("Cannot mmap memory for rte_config\n");
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n");
> +               return -1;
>         }
>         memcpy(rte_mem_cfg_addr, &early_mem_config,
> sizeof(early_mem_config));
>         rte_config.mem_config = rte_mem_cfg_addr;
> @@ -353,10 +360,11 @@ enum rte_iova_mode
>
>         rte_config.mem_config->dma_maskbits = 0;
>
> +       return 0;
>  }
>
>  /* attach to an existing shared memory config */
> -static void
> +static int
>  rte_eal_config_attach(void)
>  {
>         struct rte_mem_config *mem_config;
> @@ -364,33 +372,40 @@ enum rte_iova_mode
>         const char *pathname = eal_runtime_config_path();
>
>         if (internal_config.no_shconf)
> -               return;
> +               return 0;
>
>         if (mem_cfg_fd < 0){
>                 mem_cfg_fd = open(pathname, O_RDWR);
> -               if (mem_cfg_fd < 0)
> -                       rte_panic("Cannot open '%s' for rte_mem_config\n",
> pathname);
> +               if (mem_cfg_fd < 0) {
> +                       RTE_LOG(ERR, EAL, "Cannot open '%s' for
> rte_mem_config\n",
> +                                       pathname);
> +                       return -1;
> +               }
>         }
>
>         /* map it as read-only first */
>         mem_config = (struct rte_mem_config *) mmap(NULL,
> sizeof(*mem_config),
>                         PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
> -       if (mem_config == MAP_FAILED)
> -               rte_panic("Cannot mmap memory for rte_config! error %i
> (%s)\n",
> -                         errno, strerror(errno));
> +       if (mem_config == MAP_FAILED) {
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                               errno, strerror(errno));
> +               return -1;
> +       }
>
>         rte_config.mem_config = mem_config;
> +
> +       return 0;
>  }
>
>  /* reattach the shared config at exact memory location primary process
> has it */
> -static void
> +static int
>  rte_eal_config_reattach(void)
>  {
>         struct rte_mem_config *mem_config;
>         void *rte_mem_cfg_addr;
>
>         if (internal_config.no_shconf)
> -               return;
> +               return 0;
>
>         /* save the address primary process has mapped shared config to */
>         rte_mem_cfg_addr = (void *) (uintptr_t)
> rte_config.mem_config->mem_cfg_addr;
> @@ -403,18 +418,22 @@ enum rte_iova_mode
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
>                         mem_cfg_fd, 0);
>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
> -               if (mem_config != MAP_FAILED)
> +               if (mem_config != MAP_FAILED) {
>                         /* errno is stale, don't use */
> -                       rte_panic("Cannot mmap memory for rte_config at
> [%p], got [%p]"
> +                       RTE_LOG(ERR, EAL, "Cannot mmap memory for
> rte_config at [%p], got [%p]"
>                                   " - please use '--base-virtaddr'
> option\n",
>                                   rte_mem_cfg_addr, mem_config);
> -               else
> -                       rte_panic("Cannot mmap memory for rte_config!
> error %i (%s)\n",
> -                                 errno, strerror(errno));
> +                       return -1;
> +               }
> +               RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config!
> error %i (%s)\n",
> +                         errno, strerror(errno));
> +               return -1;
>         }
>         close(mem_cfg_fd);
>
>         rte_config.mem_config = mem_config;
> +
> +       return 0;
>  }
>
>  /* Detect if we are a primary or a secondary process */
> @@ -461,26 +480,39 @@ enum rte_proc_type_t
>  }
>
>  /* Sets up rte_config structure with the pointer to shared memory
> config.*/
> -static void
> +static int
>  rte_config_init(void)
>  {
>         rte_config.process_type = internal_config.process_type;
>
>         switch (rte_config.process_type){
>         case RTE_PROC_PRIMARY:
> -               rte_eal_config_create();
> +               if (rte_eal_config_create() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to create config\n");
> +                       return -1;
> +               }
>                 eal_update_mem_config();
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to attach config\n");
> +                       return -1;
> +               }
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> -               rte_eal_config_reattach();
> +               if (rte_eal_config_reattach() < 0) {
> +                       RTE_LOG(ERR, EAL, "Failed to reattach config\n");
> +                       return -1;
> +               }
>                 eal_update_internal_config();
>                 break;
>         case RTE_PROC_AUTO:
>         case RTE_PROC_INVALID:
> -               rte_panic("Invalid process type\n");
> +               RTE_LOG(ERR, EAL, "Invalid process type %d\n",
> +                               rte_config.process_type);
> +               return -1;
>         }
> +
> +       return 0;
>  }
>
>  /* Unlocks hugepage directories that were locked by
> eal_hugepage_info_init */
> @@ -998,7 +1030,10 @@ static void rte_eal_init_alert(const char *msg)
>                 return -1;
>         }
>
> -       rte_config_init();
> +       if (rte_config_init() < 0) {
> +               rte_eal_init_alert("Cannot init config");
> +               return -1;
> +       }
>
>         if (rte_eal_intr_init() < 0) {
>                 rte_eal_init_alert("Cannot init interrupt-handling
> thread");
> --
> 1.8.3.1
>
>


-- 
David Marchand


More information about the dev mailing list