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 D6D2CA0096 for ; Wed, 5 Jun 2019 09:50:21 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E64C15424; Wed, 5 Jun 2019 09:50:20 +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 7ECAC4F91 for ; Wed, 5 Jun 2019 09:50:19 +0200 (CEST) Received: by mail-vs1-f66.google.com with SMTP id l20so15183106vsp.3 for ; Wed, 05 Jun 2019 00:50:19 -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=kGUAk74c3elN0MdUxBImrQcPHaKL6/g3obFrD4cn0Eg=; b=HR9lPrv1a9CEdAQ/S/K15292ypARCHy9z8DUS8ac3glTXrQNYXdu4birItsXkI+Kxd HlBXWYmhrrh/vGi7iNLtC84Wnvt2tUj0DOVOdwiuZ6T7ffeVYOEEACXGVbQdcwP2fC2n 7gcW+VQAY5DpT8OkKx+OplwGc4Qs3tM7wPi26eyJk6O9ls3NoT6wtUeSNvE+AC0VQkqe pqXbnXE3TEMc1aQ/6OLXjZcw0kBr8M/fwWsUJnFx76TsbeTx4vb3B15xa4NgljdNa0iG 1p2QxkfLM2JzCnnr9HE3Y7FgLA7FllKIDkPYxwPpfYEKJI1cCUWk1tlPl0ISPyG8uLPM y46Q== X-Gm-Message-State: APjAAAWvjHPuuacv1oElhI6g0Z0WeqwDTlEy0Q68vHs7lzFlas5tXZM2 YNYnhk0h37eOsgbLutqnxQFWJTDxRGZo7qdeNPqACA== X-Google-Smtp-Source: APXvYqy7afWn5sMt3nAv86OeDUDSm+Hkmr3C6gLDu29zgEQmmkvLUmdX+W9mfe9mS0tsQAY8PTJSI+LAj/p3CO9hmNI= X-Received: by 2002:a67:2781:: with SMTP id n123mr19550327vsn.141.1559721018791; Wed, 05 Jun 2019 00:50:18 -0700 (PDT) MIME-Version: 1.0 References: <1559546368-32263-1-git-send-email-arnon@qwilt.com> <1559661921-8821-1-git-send-email-arnon@qwilt.com> In-Reply-To: <1559661921-8821-1-git-send-email-arnon@qwilt.com> From: David Marchand Date: Wed, 5 Jun 2019 09:50:08 +0200 Message-ID: To: Arnon Warshavsky Cc: dev , Thomas Monjalon , Stephen Hemminger , Bruce Richardson Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v4] 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" On Tue, Jun 4, 2019 at 5:45 PM 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. > All we care about in this patch are the panics wrt the shared configuration init. Can the commit title and description refer to this ? > Signed-off-by: Arnon Warshavsky > --- > > 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 > > lib/librte_eal/freebsd/eal/eal.c | 59 ++++++++++++++++++-------- > lib/librte_eal/linux/eal/eal.c | 91 > +++++++++++++++++++++++++++------------- > 2 files changed, 103 insertions(+), 47 deletions(-) > > diff --git a/lib/librte_eal/freebsd/eal/eal.c > b/lib/librte_eal/freebsd/eal/eal.c > index c6ac902..4b4db3b 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,79 @@ 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_id = -1; > I bet you did not compile FreeBSD :-). + 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 " > + mem_cfg_fd = -1; > + RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another > primary " > "process running?\n", pathname); > Indent. Not relevant to this patch, but I find it odd to truncate before trying to take the lock. Might be something to look at some day, anyway.. :-) + 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; > We failed to mmap. Since eal init is going to fail, we should not leave this fd open. } > 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); > At this point, we are in a secondary process. We don't need to keep this fd open which is fine. But at least for consistency, we should reset it to -1. - 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 +325,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 */ > diff --git a/lib/librte_eal/linux/eal/eal.c > b/lib/librte_eal/linux/eal/eal.c > index 1613996..1ec264f 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,37 @@ 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"); > + RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config\n"); > + return -1; > Same comment than FreeBSD, mmap failed, we can close/reset mem_cfg_fd. } > memcpy(rte_mem_cfg_addr, &early_mem_config, > sizeof(early_mem_config)); > rte_config.mem_config = rte_mem_cfg_addr; > @@ -353,10 +362,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 +374,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; > + } > We can close/reset mem_cfg_fd but in the error path _only_: we are going to reuse it in rte_eal_config_reattach. > 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 +420,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]" > - " - 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); > Let's move this close and add a reset to -1 before the branch. > rte_config.mem_config = mem_config; > + > + return 0; > } > > /* Detect if we are a primary or a secondary process */ > @@ -461,26 +482,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 +1026,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; > + } > This hunk is missing for FreeBSD. > if (rte_eal_intr_init() < 0) { > rte_eal_init_alert("Cannot init interrupt-handling > thread"); > -- > 1.8.3.1 > > -- David Marchand