From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8411DA0350; Thu, 25 Jun 2020 16:18:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B5A2FAAB7; Thu, 25 Jun 2020 16:18:18 +0200 (CEST) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by dpdk.org (Postfix) with ESMTP id 2BF445F69 for ; Thu, 25 Jun 2020 16:18:17 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 9A277580664; Thu, 25 Jun 2020 10:18:14 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Thu, 25 Jun 2020 10:18:14 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= 5pUc9WKkfUp8p3oC3k0DaijJoPq42oin2qPQR1/E3DQ=; b=rU1T1ZGvVo4DL3w7 Co3nsOYWNBcQ87R9FH1VlhQCj3Y95UIFW0gWafjnA3l1D66zn4MFlk7JWi/3s90s sJibuIPMd3/abzidOoblyHNzrBMVKGIEdlCa2pva5vdZZPGk/H4jVEEoCM3/jLeq U42RBDBKUhsIXrPvpMqfKjdXaVziF3/nydXl4tS7WHtRe3EB32n1ZqL52VapulHN v+9uFpXXTRe79VrP/3BxKOb+s+LIRcxR1+K83eSb53U6r1KF58HfeITYr/YKVqsA V+gTvIKrZV3MzV69pVxVb2+q6xD3S0ARQumj8yfam6apAFgPW/kw27CpYx7Jr7RI A4Xomw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=5pUc9WKkfUp8p3oC3k0DaijJoPq42oin2qPQR1/E3 DQ=; b=DaGV3AY/OV+EuEZLGZKBObOhii8ZJAwEmLb/s3z8HfTwEv/EYiRbjsxnn fr7Q1yCz6EomEXKiVcmBKO7Grovb4QECWeyTJQO/sF4Qoxh+Fig3SX7i/tHTJjdR tjAgfpTbFPmIbpF3fk/S5Nn4ap5AEP6cjF8/Y3lU82ZIO/+WHWvGIXuy8cygId1j oF2EU9zIhefteSOUBH6nheooiHrOM3A3zQkc8gx4SoxGAcMy2JneILNKS0DSXQv1 HW9jle47jYKA6CL8FwythIPpblOervUBws4YiX/GoEJ7ovC49mBBN9y1rtHNRJ5y 5ELB/J6ZzrYrbK0Fh7uafG3hwcx+A== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrudekledgjeejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecukfhppeejjedrudefgedrvddtfedrudekgeenucevlhhushhtvghruf hiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghl ohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 97C80328005E; Thu, 25 Jun 2020 10:18:12 -0400 (EDT) From: Thomas Monjalon To: Tal Shnaiderman Cc: dev@dpdk.org, pallavi.kadam@intel.com, dmitry.kozliuk@gmail.com, david.marchand@redhat.com, grive@u256.net, ranjit.menon@intel.com, navasile@linux.microsoft.com, harini.ramakrishnan@microsoft.com, ocardona@microsoft.com, anatoly.burakov@intel.com, fady@mellanox.com, bruce.richardson@intel.com Date: Thu, 25 Jun 2020 16:18:09 +0200 Message-ID: <2001711.5IMlzu0qbX@thomas> In-Reply-To: <20200624082847.21344-2-talshn@mellanox.com> References: <20200622075529.24180-2-talshn@mellanox.com> <20200624082847.21344-1-talshn@mellanox.com> <20200624082847.21344-2-talshn@mellanox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v9 01/10] eal: move OS common config objects 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" 24/06/2020 10:28, talshn@mellanox.com: > +void Could check the size in the function and return an error and log, instead of doing the check prior the call. > +eal_set_runtime_dir(char *run_dir, size_t size) > +{ > + strncpy(runtime_dir, run_dir, size); strlcpy would be better [...] > +void > +eal_config_remap(void *mem_cfg_addr) "remap" word may be confusing. What about "eal_config_switch"? > +{ > + memcpy(mem_cfg_addr, &early_mem_config, sizeof(early_mem_config)); After review with David, another proposal: Instead of creating this function, we could just reference early_mem_config thanks to the getter: rte_eal_get_configuration()->mem_config > + rte_config.mem_config = mem_cfg_addr; > + > + /* store address of the config in the config itself so that secondary > + * processes could later map the config into this exact location > + */ > + rte_config.mem_config->mem_cfg_addr = (uintptr_t) mem_cfg_addr; > + > + rte_config.mem_config->dma_maskbits = 0; > +} [...] > --- a/lib/librte_eal/common/eal_common_dynmem.c > +++ b/lib/librte_eal/common/eal_common_dynmem.c > + struct internal_config *internal_conf = > + eal_get_internal_configuration(); single or double indent for assignments? [...] > @@ -362,13 +368,16 @@ eal_dynmem_calc_num_pages_per_socket( > unsigned int requested, available; > int total_num_pages = 0; > uint64_t remaining_mem, cur_mem; > - uint64_t total_mem = internal_config.memory; > + const struct internal_config *internal_conf = > + eal_get_internal_configuration(); > + > + uint64_t total_mem = internal_conf->memory; Why this line is moved and separated with a blank line? [...] > @@ -491,7 +504,10 @@ int > rte_mem_alloc_validator_unregister(const char *name, int socket_id) > { > /* FreeBSD boots with legacy mem enabled by default */ This comment must be moved. > - if (internal_config.legacy_mem) { > + const struct internal_config *internal_conf = > + eal_get_internal_configuration(); > + > + if (internal_conf->legacy_mem) { > RTE_LOG(DEBUG, EAL, "Registering mem alloc validators not supported\n"); [...] > - internal_config.base_virtaddr = > + internal_conf->base_virtaddr = > RTE_PTR_ALIGN_CEIL((uintptr_t)addr, (size_t)RTE_PGSIZE_16M); This assignment had a single indent tab. Better to be consistent with the new assignments: [...] > + struct internal_config *internal_conf = > + eal_get_internal_configuration(); [...] > @@ -807,7 +815,10 @@ rte_mp_sendmsg(struct rte_mp_msg *msg) > if (check_input(msg) != 0) > return -1; > > - if (internal_config.no_shconf) { > + const struct internal_config *internal_conf = > + eal_get_internal_configuration(); Please declare variable at the top of the function. [...] > --- a/lib/librte_eal/freebsd/eal.c > +++ b/lib/librte_eal/freebsd/eal.c > @@ -287,13 +252,7 @@ rte_eal_config_create(void) > return -1; > } > > - memcpy(rte_mem_cfg_addr, &early_mem_config, sizeof(early_mem_config)); > - rte_config.mem_config = rte_mem_cfg_addr; > - > - /* store address of the config in the config itself so that secondary > - * processes could later map the config into this exact location > - */ > - rte_config.mem_config->mem_cfg_addr = (uintptr_t) rte_mem_cfg_addr; > + eal_config_remap(rte_mem_cfg_addr); As described above, we could keep this code in the OS implementation, and manage to get early_mem_config with the existing getter rte_eal_get_configuration()->mem_config. [...] > @@ -443,7 +447,10 @@ close_hugefile(int fd, char *path, int list_idx) > * primary process must unlink the file, but only when not in in-memory > * mode (as in that case there is no file to unlink). > */ > - if (!internal_config.in_memory && > + const struct internal_config *internal_conf = > + eal_get_internal_configuration(); Better to declare the variable at the top and keep the comment close to the next lines. > + > + if (!internal_conf->in_memory && > rte_eal_process_type() == RTE_PROC_PRIMARY && > unlink(path)) > RTE_LOG(ERR, EAL, "%s(): unlinking '%s' failed: %s\n", Thank you for the cleanup. Next step will be to review the namespace of the internal functions, the organization of the internal structures, and probably add more getter functions. There are a lot of cleanup to do in EAL. Any help is welcome!