From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 780238DA6 for ; Thu, 26 Apr 2018 18:07:51 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1996E407574E; Thu, 26 Apr 2018 16:07:51 +0000 (UTC) Received: from ktraynor.remote.csb (unknown [10.36.118.53]) by smtp.corp.redhat.com (Postfix) with ESMTP id CD42D2029296; Thu, 26 Apr 2018 16:07:48 +0000 (UTC) To: Arnon Warshavsky , thomas@monjalon.net, anatoly.burakov@intel.com, wenzhuo.lu@intel.com, declan.doherty@intel.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com, ferruh.yigit@intel.com Cc: dev@dpdk.org References: <1524663944-30376-11-git-send-email-arnon@qwilt.com> <1524723664-30510-1-git-send-email-arnon@qwilt.com> <1524723664-30510-10-git-send-email-arnon@qwilt.com> From: Kevin Traynor Organization: Red Hat Message-ID: <621e7964-0450-a66e-cbf3-4fbc3a5ad723@redhat.com> Date: Thu, 26 Apr 2018 17:07:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <1524723664-30510-10-git-send-email-arnon@qwilt.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 26 Apr 2018 16:07:51 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 26 Apr 2018 16:07:51 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'ktraynor@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v9 09/10] eal: replace rte_panic instances in 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: , X-List-Received-Date: Thu, 26 Apr 2018 16:07:51 -0000 On 04/26/2018 07:21 AM, Arnon Warshavsky wrote: > Change some local functions return type from void to int. > This change does not break ABI as the functions are internal. > Panic thrown from threads was not handled in this patch > > Signed-off-by: Arnon Warshavsky > Acked-by: Anatoly Burakov > --- > lib/librte_eal/bsdapp/eal/eal.c | 71 ++++++++++++++++++++------- > lib/librte_eal/linuxapp/eal/eal.c | 101 ++++++++++++++++++++++++++------------ > 2 files changed, 121 insertions(+), 51 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c > index 10d8dc0..c958ed9 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -151,7 +151,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; > @@ -160,60 +160,83 @@ 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, 0660); > - if (mem_cfg_fd < 0) > - rte_panic("Cannot open '%s' for rte_mem_config\n", pathname); > + if (mem_cfg_fd < 0) { > + RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n", > + __func__, 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(CRIT, EAL, "%s(): Cannot resize '%s' for rte_mem_config\n", > + __func__, 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(CRIT, EAL, "%s(): Cannot create lock on '%s'. Is another primary process running?\n", > + __func__, 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(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n", > + __func__); > + 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(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n", > + __func__, 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) { > + mem_cfg_fd = -1; > + RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n", > + __func__); > + return -1; > + } > > rte_config.mem_config = rte_mem_cfg_addr; > + > + return 0; > } > > /* Detect if we are a primary or a secondary process */ > @@ -237,23 +260,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()) > + return -1; > break; > case RTE_PROC_SECONDARY: > - rte_eal_config_attach(); > + if (rte_eal_config_attach()) > + 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"); > + default: > + RTE_LOG(CRIT, EAL, "%s(): Invalid process type %d\n", > + __func__, rte_config.process_type); > + return -1; > } > + return 0; > } > > /* display usage */ > @@ -552,7 +581,11 @@ static void rte_eal_init_alert(const char *msg) > return -1; > } > > - rte_config_init(); > + if (rte_config_init() != 0) { > + rte_eal_init_alert("Failed to init configuration"); > + rte_errno = EFAULT; > + return -1; > + } > > /* Put mp channel init before bus scan so that we can init the vdev > * bus through mp channel in the secondary process before the bus scan. > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index 200e879..c0d704b 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -160,7 +160,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; > @@ -169,7 +169,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) > @@ -179,30 +179,42 @@ enum rte_iova_mode > else > rte_mem_cfg_addr = NULL; > > - if (mem_cfg_fd < 0){ > + if (mem_cfg_fd < 0) { > mem_cfg_fd = open(pathname, O_RDWR | O_CREAT, 0660); > - if (mem_cfg_fd < 0) > - rte_panic("Cannot open '%s' for rte_mem_config\n", pathname); > + if (mem_cfg_fd < 0) { > + RTE_LOG(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n", > + __func__, pathname); > + return -1; > + } > } > > retval = ftruncate(mem_cfg_fd, sizeof(*rte_config.mem_config)); > - if (retval < 0){ > + if (retval < 0) { > close(mem_cfg_fd); > - rte_panic("Cannot resize '%s' for rte_mem_config\n", pathname); > + mem_cfg_fd = -1; > + RTE_LOG(CRIT, EAL, "%s(): Cannot resize '%s' for rte_mem_config\n", > + __func__, pathname); > + return -1; > } > > retval = fcntl(mem_cfg_fd, F_SETLK, &wr_lock); > - if (retval < 0){ > + 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(CRIT, EAL, "%s(): Cannot create lock on '%s'.Is another primary process running?\n", > + __func__, 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"); > + if (rte_mem_cfg_addr == MAP_FAILED) { > + RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config\n", > + __func__); > + 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; > @@ -211,10 +223,11 @@ enum rte_iova_mode > * processes could later map the config into this exact location */ > rte_config.mem_config->mem_cfg_addr = (uintptr_t) rte_mem_cfg_addr; > > + return 0; > } > > /* attach to an existing shared memory config */ > -static void > +static int > rte_eal_config_attach(void) > { > struct rte_mem_config *mem_config; > @@ -222,33 +235,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){ > + 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(CRIT, EAL, "%s(): Cannot open '%s' for rte_mem_config\n", > + __func__, 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(CRIT, EAL, "%s(): Cannot mmap memory for rte_config! error %i (%s)\n", > + __func__, 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; > @@ -263,16 +285,20 @@ enum rte_iova_mode > if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) { > 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); > + RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n", > + __func__, rte_mem_cfg_addr, mem_config); > else > - rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n", > - errno, strerror(errno)); > + RTE_LOG(CRIT, EAL, "%s(): Cannot mmap memory for rte_config! error %i (%s)\n", > + __func__, errno, strerror(errno)); > + close(mem_cfg_fd); > + mem_cfg_fd = -1; > + return -1; > } > close(mem_cfg_fd); > > rte_config.mem_config = mem_config; > + > + return 0; > } > > /* Detect if we are a primary or a secondary process */ > @@ -296,24 +322,32 @@ 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()) > + return -1; > break; > case RTE_PROC_SECONDARY: > - rte_eal_config_attach(); > + if (rte_eal_config_attach()) > + return -1; > rte_eal_mcfg_wait_complete(rte_config.mem_config); > - rte_eal_config_reattach(); > + if (rte_eal_config_reattach()) > + return -1; > break; > case RTE_PROC_AUTO: > case RTE_PROC_INVALID: > - rte_panic("Invalid process type\n"); > + default: > + RTE_LOG(CRIT, EAL, "%s(): Invalid process type %d\n", > + __func__, rte_config.process_type); > + return -1; > } > + > + return 0; > } > > /* Unlocks hugepage directories that were locked by eal_hugepage_info_init */ > @@ -833,6 +867,9 @@ static void rte_eal_init_alert(const char *msg) > > rte_srand(rte_rdtsc()); > > + if (rte_config_init() != 0) > + return -1; > + I'm confused, is this deliberate? there is now two rte_config_init() calls. Aaron's comments on v4 about rte_eal_init_alert() are missed? and it's missing the clear once and error number like the other returns. > if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0) { > rte_eal_init_alert("Cannot init logging."); > rte_errno = ENOMEM; >