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 8BC81A0096 for ; Wed, 5 Jun 2019 17:43:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D8B081BB65; Wed, 5 Jun 2019 17:43:56 +0200 (CEST) Received: from mail-it1-f193.google.com (mail-it1-f193.google.com [209.85.166.193]) by dpdk.org (Postfix) with ESMTP id 0912E1BB52 for ; Wed, 5 Jun 2019 17:43:54 +0200 (CEST) Received: by mail-it1-f193.google.com with SMTP id h9so4232337itk.3 for ; Wed, 05 Jun 2019 08:43:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qwilt-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=SjCkgxSvx9aX5nP8gfjt2f6rvFDJpJm4rvbfEFMEUYU=; b=MlQxUD9fJCMo/KxqLvmiM0kDJlvUIpOdh3OjVJ2jH9a+LNN2h7eCW5johgZCjYctF5 nsrvrPSf74goUQrf+X+q+i64csU8MiwKZh5o4+6ZsTGIBUKjx4oxQUB5mgMHfl01eVk3 rjExX8sBXnKfkvh6uk8Y8DYILQySqtH6OkE5V+SgoiG3bJbV2KB0ockJu6M4skNGX7Fj S9/N//a6Tk/c+m2YgVZXEM1pL5Njubcoe8rhKxPYfhUvHM0LV8BTg9goYE9euUZj/fGJ NMoZxSG7cCEg06B83yytDhgiGjE5aniqAAx9oAQEG50bB6XTz9q8IfFyLWfuzWEqawOm TzDA== 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=SjCkgxSvx9aX5nP8gfjt2f6rvFDJpJm4rvbfEFMEUYU=; b=gz5hMOl6GKTgTVBgSsAOsTdTHyiK2c7Vk1k68g+TQLqj+Y68buTVJImnMPkjXmU7eL ENyM0lkSLri/hEPlHIKVH88a1itsmxDPW9Qxwz9HCmNudY/r2xo8UiBD9Csvb82zxWrN v3msDHozTxTvoNTYrLr0tbNjrbcXEbLPvpsFIcSphSGw5iOlnJ9QDJrbyWaJqt5wBsdh JD5+Koz8Eb8miAMeRYupchjikMW0A0Rf3GTODnaKEj239nbYR8rhDxM6hNx4gyzEaljk UcbT2MLr8vnjWJBhm9t/ja3+VHIPKe1+Jt2sIIJNZwFcr46ZY+fF3dtFYHr9ocMs8ksu zIiQ== X-Gm-Message-State: APjAAAVtazrNNk2cdNoMgvoLFtUpetGWsUNem18svdbVsHjjX3JwD58O rVt7g2g922l+3TcBPGfhV1+y3YX3GDL8zx88Jjnm0w== X-Google-Smtp-Source: APXvYqyJTUXkh+saRjQYe0sEZaUsAIq7ldEim4r9MJy5zMIUHnT+OeNZA/LAxIQbF1mNq9MWxqa7gJ+OzXtVNrVbklc= X-Received: by 2002:a24:1dd5:: with SMTP id 204mr28642334itj.180.1559749434102; Wed, 05 Jun 2019 08:43:54 -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: From: Arnon Warshavsky Date: Wed, 5 Jun 2019 18:43:43 +0300 Message-ID: To: David Marchand 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 Wed, Jun 5, 2019 at 10:50 AM David Marchand wrote: > > > 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 ? > Will remove the non-thread from the title. Can you be more specific/ offer your view per the description? > > >> Signed-off-by: Arnon Warshavsky >> --- >> >> The calls for launching core messaging threads were left in tact >> in all 3 eal implementations. >> >> > 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 :-). > Yup...Not only haste is from the devil..Getting myself a fbsd vm now.. > > + 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. > Ack > > 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. > Ok. In general I tried to stick with previous functionality by just replacing the panic with the exit path, rather than improve it, but this one is indeed straight forward. > >> + } >> } >> >> 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. > Ack. Same as above > > >> 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. > > Ack > > >> + } >> > > 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. > Ack > > >> - rte_config_init(); >> + if (rte_config_init() < 0) { >> + rte_eal_init_alert("Cannot init config"); >> + return -1; >> + } >> > > This hunk is missing for FreeBSD. > > mmmmm , not enough coffee in the process :-/ > thanks