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 143A1A00E6 for ; Tue, 11 Jun 2019 10:08:06 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 22D131C282; Tue, 11 Jun 2019 10:08:06 +0200 (CEST) Received: from mail-vk1-f193.google.com (mail-vk1-f193.google.com [209.85.221.193]) by dpdk.org (Postfix) with ESMTP id 8276A1C281 for ; Tue, 11 Jun 2019 10:08:04 +0200 (CEST) Received: by mail-vk1-f193.google.com with SMTP id s16so2310286vke.7 for ; Tue, 11 Jun 2019 01:08:04 -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=HKn6FDMO9WCKWzer3tw//CH/YCBdkD6NU0f8ZyGmb1k=; b=AyGZG+lxi0lE/EJwNmtOF3HlkKr4z5v20BjYoqE0uijsn/M/dmiaie+fdZNLTsH8Ue WM6GabK/EKxWZWFIKurmbDmkf8Qd+D1fOFf2bt0cSsIxxAvBTbDCZNJu4k1M+GF7f2z2 q4YHd+DVq5/OENDMsSp+yySvuAGCRBDhGGEdLIvNPhz8lDppdxx1EkQ2yw9TPydEzzOD CKBWqJi3GG0gqnsNsYJCJ4pLcpK9hqy01hCoy0FCvxXEecNrtRBKY9ztUYnWIf/glswp uLrLSiExHjyLBsZj2JmCrp5ZBeTZ1Mgx1psHiNwHOkf2NI2nlI7tcLVEcnCiKZxvweSl jLwg== X-Gm-Message-State: APjAAAW4fCNUpkw03KYkBExPXCziK7T8MwJ/bNgjt7FNn6EJZp/Jd/Pl aJAh3jENG9k3Il4sq+lGbGJrgy0pbJxcddIIBIojFg== X-Google-Smtp-Source: APXvYqzSxUqvCTVe/1liglxKEdcTB3uYzEmD8uZVS7owM84aEWgdzGpdvi3vbLm8ybIfoZwdI2xvcbPCLU79Aqp1Qhw= X-Received: by 2002:a1f:144:: with SMTP id 65mr5750823vkb.53.1560240483669; Tue, 11 Jun 2019 01:08:03 -0700 (PDT) MIME-Version: 1.0 References: <1559661921-8821-1-git-send-email-arnon@qwilt.com> <1560150509-22146-1-git-send-email-arnon@qwilt.com> In-Reply-To: <1560150509-22146-1-git-send-email-arnon@qwilt.com> From: David Marchand Date: Tue, 11 Jun 2019 10:07:52 +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 v5] eal: do not panic on shared memory init 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 Mon, Jun 10, 2019 at 9:09 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. > > 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 > -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 Thanks Arnon! -- David Marchand