[v5] eal: do not panic on shared memory init

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

Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
---

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.

-v2 fix freebsd compilation
-v3 fix freebsd compilation, haste from the devil
-v4 fix comments by David
-v5 fix more comments by david. rename patch

 lib/librte_eal/freebsd/eal/eal.c |  69 ++++++++++++++++++--------
 lib/librte_eal/linux/eal/eal.c   | 101 +++++++++++++++++++++++++++------------
 2 files changed, 120 insertions(+), 50 deletions(-)
  

On Mon, Jun 10, 2019 at 9:09 AM Arnon Warshavsky <arnon@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.
>
> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> ---
>
> 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.
>
> -v2 fix freebsd compilation
> -v3 fix freebsd compilation, haste from the devil
> -v4 fix comments by David
> -v5 fix more comments by david. rename patch
>
>  lib/librte_eal/freebsd/eal/eal.c |  69 ++++++++++++++++++--------
>  lib/librte_eal/linux/eal/eal.c   | 101
> +++++++++++++++++++++++++++------------
>  2 files changed, 120 insertions(+), 50 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c
> b/lib/librte_eal/freebsd/eal/eal.c
> index c6ac902..f18f08e 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,82 @@ 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);
> +                       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);
> +               mem_cfg_fd = -1;
> +               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 "
> -                               "process running?\n", pathname);
> +               mem_cfg_fd = -1;
> +               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");
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               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");
> +       mem_cfg_fd = -1;
> +       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 +328,29 @@ 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)
> +                       return -1;
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       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 */
> @@ -655,7 +683,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");
> diff --git a/lib/librte_eal/linux/eal/eal.c
> b/lib/librte_eal/linux/eal/eal.c
> index 1613996..224ba90 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,29 +321,41 @@ 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);
> +               mem_cfg_fd = -1;
> +               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 "
> -                               "process running?\n", pathname);
> +               mem_cfg_fd = -1;
> +               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");
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               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 +365,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 +377,42 @@ 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) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               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;
> @@ -402,19 +424,26 @@ enum rte_iova_mode
>         mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE,
> MAP_SHARED,
>                         mem_cfg_fd, 0);
> +
> +       close(mem_cfg_fd);
> +       mem_cfg_fd = -1;
> +
>         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]"
> -                                 " - 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));
> +                       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);
> +                       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 +490,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)
> +                       return -1;
>                 eal_update_mem_config();
>                 break;
>         case RTE_PROC_SECONDARY:
> -               rte_eal_config_attach();
> +               if (rte_eal_config_attach() < 0)
> +                       return -1;
>                 rte_eal_mcfg_wait_complete(rte_config.mem_config);
> -               rte_eal_config_reattach();
> +               if (rte_eal_config_reattach() < 0)
> +                       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 +1034,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
>


Reviewed-by: David Marchand <david.marchand@redhat.com>

Thanks Arnon!
  

11/06/2019 10:07, David Marchand:
> On Mon, Jun 10, 2019 at 9:09 AM Arnon Warshavsky <arnon@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.
> >
> > Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> > ---
> >
> > 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.
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> Thanks Arnon!

Applied, thanks

For the record, below is the updated status of
rte_panic/rte_exit calls in libs:

librte_eal:

    int rte_eal_init
        rte_panic
        void eal_thread_init_master
            rte_panic
        -> internal change
    
    int rte_eal_remote_launch
            rte_panic
                -> add a return code
    
    eal_thread_loop
        rte_panic
            -> abort thread?
    eal_intr_thread_main
        rte_panic
            -> abort thread?
    
    int get_hugepage_dir
        rte_panic
            -> no public API
    
    uint64_t rte_get_timer_cycles
    uint64_t rte_get_hpet_hz
    uint64_t rte_get_hpet_cycles
        rte_panic
            -> return 0 / no API change
    
librte_mempool:
    void rte_mempool_*
        RTE_LIBRTE_MEMPOOL_DEBUG
            rte_panic
                -> debug assert

librte_mbuf:
    void rte_mbuf_sanity_check
        rte_panic
            -> debug assert

librte_lpm:
    RTE_LIBRTE_LPM_DEBUG
        VERIFY_DEPTH
            rte_panic

librte_acl:
    RTE_ACL_VERIFY
        rte_panic
            -> debug check? / no API change?

librte_sched:
    void debug_check_queue_slab
        rte_panic
            -> debug assert

void rte_metrics_init
        rte_exit
            -> API breakage

librte_kni:
    struct rte_kni *rte_kni_alloc
        void kni_fifo_init
            rte_panic
                -> no public API change

librte_eventdev:
    struct rte_eventdev *rte_event_pmd_vdev_init
        rte_panic
            -> no API change
    int rte_event_pmd_pci_probe
        rte_panic
            -> no API change