From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id F20D5A0096 for ; Mon, 3 Jun 2019 10:14:54 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 187B11B948; Mon, 3 Jun 2019 10:14:54 +0200 (CEST) Received: from mail-vs1-f66.google.com (mail-vs1-f66.google.com [209.85.217.66]) by dpdk.org (Postfix) with ESMTP id A3D96559A for ; Mon, 3 Jun 2019 10:14:52 +0200 (CEST) Received: by mail-vs1-f66.google.com with SMTP id l125so10638778vsl.13 for ; Mon, 03 Jun 2019 01:14:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vhtdf90sY9MUY7P+fEgL+ZRURrGUlnYheccC/2S6HHA=; b=I5yo+5ORzpzefrCrFTU5zpfWGzeJDDG2NJhWVUczhP9lI4MyRvNrKmUpnFDhXv8Yvh tdkxkiMBHvZBiBdS9BZIQ+xo6oXXfY6mWB7mOrpBb3ZWxMfQDY5YmugUKKLaSAgH3l/E eZREexsm4kUHaPiqzmgTWpxROSEZOuSYtZFtSf2JG0OwEiKqodV0pDWcIBaA/EY/u3uv 8rqlvLEHJYkFGMtH/tSW1+WtjgC5o6d6gArqmIRJpQTOVg4HzqVNPDHqnWXUw6AJn0R2 Iy2cFHcFdq8bGPAPX8r+7TM3WUK4f9d33R8lMArk74EE1+Pq/evD9QNeFCIwFkNCEpUY oyfg== X-Gm-Message-State: APjAAAVlb5Q7dt+rSShAMuohO6CUb+qmVtAlECKLOldfJOcfDf/zyDir tV8p2nIUSvnyT5tw5Z6Fy2uxKIm8Yx7R0Z9zDikW6A== X-Google-Smtp-Source: APXvYqxkfMhkfNKOi6Qb0ZwZQskgDYIIcF0K0vpszRIP4h7l8emgnE3SXHpPockOBVMUsXm7kky2cHpoi53iY6gGZLo= X-Received: by 2002:a67:1885:: with SMTP id 127mr5854697vsy.39.1559549691950; Mon, 03 Jun 2019 01:14:51 -0700 (PDT) MIME-Version: 1.0 References: <1559535816-23542-1-git-send-email-arnon@qwilt.com> <1559546368-32263-1-git-send-email-arnon@qwilt.com> In-Reply-To: <1559546368-32263-1-git-send-email-arnon@qwilt.com> From: David Marchand Date: Mon, 3 Jun 2019 10:14:40 +0200 Message-ID: To: Arnon Warshavsky Cc: dev , Thomas Monjalon , Stephen Hemminger , "Burakov, Anatoly" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v3] eal: remove non-thread panic calls from init sequence X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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 > --- > > -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