From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f47.google.com (mail-it0-f47.google.com [209.85.214.47]) by dpdk.org (Postfix) with ESMTP id 1DC061C01 for ; Wed, 25 Apr 2018 11:33:53 +0200 (CEST) Received: by mail-it0-f47.google.com with SMTP id 186-v6so18582650itu.0 for ; Wed, 25 Apr 2018 02:33:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qwilt-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=p/3NcAni9EoWa6xZ7e+0qz150P3EPzNvh1dl3niU3i8=; b=dDHiRb9BCmGsgaILJ5kL/yvPChev3za83nGe7D2N0Hm/G0wP/lWZfXggdpQ6ljQ7/U CenpKZxwVqa5G1iP6cfFM7SdqlGXkrf3c/eQiLk0tzhdvyeXm0a/be9HgRmx71W9rele Ouy2tu01LHIJoVRkK+xjk19gjB1c6ZwIdifWqsom3nu09yY/GbjCC4o72lM0TeXVRbEa RWmko5/32DgTdTAtlulEkmkKl8dedZ8MrR07rRNjcLw3wKPSeETs85c5raschxvv+lBD uaXco3PZ+8ZvLHwKtL8asAkahHW3snBfINUvzODStriVZuAAcxkW0wQHoAVsDyOxb3ja lfLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=p/3NcAni9EoWa6xZ7e+0qz150P3EPzNvh1dl3niU3i8=; b=TQ//bo1aF8u0UYUtAEPD5lAQ7k0huN3FdA6r7hd/GDzzpUIvgazpO/JUCBLD02Dtam rxvMQYQ6tedNX3Zn0E7OXWNmTY4NMGeDv9Ra2zIIgXTKtOZZXRq2WsdQ44bpkotLqPI/ 4+s8ZFEv2GGZILltWtz1h74KbBnk+dKKazmUwHvAlEYAk1DE4+XhcFRuHhRuUXHWJm0m qrJLfR2NplVUASk8FMdb+R5KMCKzxT0/KjssnTk6HOgJaa7zatGKMIREyN7i+Z39roX5 mOY67TQAWYmM9d5TarXPbrRoJWjXMR/Ax7vbHhETLkhexM4Ft8bc+w5tZRSfTGdT/FzE DspA== X-Gm-Message-State: ALQs6tBfk+QDu95DtEsWwZkvmgGymSGxhRdwZBFOBJl/rmfesGmAhRGI aN4kyWwQRUoTe00x4jg3ketSTbXRk5AGMLiTl0GqPA== X-Google-Smtp-Source: AIpwx490sOQXMLQsJQCSY9fW5NkFbJRUiLCQUCHFDZYxXbUqngv9PHf2AFG+nJG5bO1qBtbuA/KSoNLl25fB6ApLh5Y= X-Received: by 2002:a24:8d85:: with SMTP id w127-v6mr19098545itd.22.1524648832468; Wed, 25 Apr 2018 02:33:52 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.142.145 with HTTP; Wed, 25 Apr 2018 02:33:52 -0700 (PDT) In-Reply-To: References: <1524608213-2080-1-git-send-email-arnon@qwilt.com> <1524608213-2080-11-git-send-email-arnon@qwilt.com> From: Arnon Warshavsky Date: Wed, 25 Apr 2018 12:33:52 +0300 Message-ID: To: "Burakov, Anatoly" Cc: Thomas Monjalon , "Lu, Wenzhuo" , "Doherty, Declan" , jerin.jacob@caviumnetworks.com, Bruce Richardson , "Yigit, Ferruh" , dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v7 10/11] 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: Wed, 25 Apr 2018 09:33:54 -0000 <...> > > + if (rte_config_init() != 0) { >> + rte_eal_init_alert("Failed to init configuration"); >> + rte_errno = EFAULT; >> + return -1; >> + } >> + >> + if (rte_mp_channel_init() < 0) { >> + rte_eal_init_alert("failed to init mp channel\n"); >> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >> + rte_errno = EFAULT; >> + return -1; >> + } >> + } >> > > ^^^ this change looks unintended. Rebase artifact? > > + >> /* in secondary processes, memory init may allocate additional >> fbarrays >> * not present in primary processes, so to avoid any potential >> issues, >> * initialize memzones first. >> @@ -671,6 +712,7 @@ static void rte_eal_init_alert(const char *msg) >> */ >> if (pipe(lcore_config[i].pipe_master2slave) < 0) >> rte_panic("Cannot create pipe\n"); >> + >> if (pipe(lcore_config[i].pipe_slave2master) < 0) >> rte_panic("Cannot create pipe\n"); >> > > ^^^ this looks unintended as well. > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c >> b/lib/librte_eal/linuxapp/eal/eal.c >> index 5b23bf0..54adaec 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; >> > > <...> > > } >> 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__); >> + return -1; >> } >> > > I think you forgot to close mem_cfg_fd and set it to -1 in case of error > here. > > memcpy(rte_mem_cfg_addr, &early_mem_config, >> sizeof(early_mem_config)); >> rte_config.mem_config = rte_mem_cfg_addr; >> @@ -211,10 +221,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; >> } >> >> > > <...> > > /* 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) { >> + mem_cfg_fd = -1; >> > > Forgot close() here, i think. > > + 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 */ >> > > <...> > > + if (rte_config_init() != 0) >> + return -1; >> + >> if (rte_eal_log_init(logid, internal_config.syslog_facility) < >> 0) { >> rte_eal_init_alert("Cannot init logging."); >> rte_errno = ENOMEM; >> @@ -914,6 +946,7 @@ static void rte_eal_init_alert(const char *msg) >> */ >> if (pipe(lcore_config[i].pipe_master2slave) < 0) >> rte_panic("Cannot create pipe\n"); >> + >> if (pipe(lcore_config[i].pipe_slave2master) < 0) >> > > Again, looks like unintended whitespace change. > > rte_panic("Cannot create pipe\n"); >> >> > > Thanks Anatoly