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 CEA0CA0588; Thu, 16 Apr 2020 01:32:55 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 630F21D587; Thu, 16 Apr 2020 01:32:55 +0200 (CEST) Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) by dpdk.org (Postfix) with ESMTP id E18721D585 for ; Thu, 16 Apr 2020 01:32:54 +0200 (CEST) Received: by mail-lf1-f66.google.com with SMTP id t11so4144194lfe.4 for ; Wed, 15 Apr 2020 16:32:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=NhN2kmINJ5i9TebGFr+F9IiwL/8dDiB5fk1RCt0i1iE=; b=kO4fQ6ac3akBfmJhgsVncXisTG/eQ7mkiurQLy7AyZv2wukcg7D+FAPotG9byLeNlq NFL+F+nc7B3aP/tRpo70bEcr4Wg16vSR6rmPwiyH5HjkSUCvRKZvRKlWhdd3ITZ6riek zMSapyc2n70YUflc6KrxfsmD/+fy2RkxSz4f4Rw9w1EhM/NJSUe5Da253v4JIaVWJsBL Rflz1gSSYReUjsG3We+vwbzc2PwA6/GBYIbH2UGtt1et8TmiUHW+nsFskYCHfsCmCZG6 Id6ticut2iIn7Q2daX3aC4v65YlIqYOlFTUtePDQS3Y9mmJaz72EUMHXSpLsmCHXc60+ 9vXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=NhN2kmINJ5i9TebGFr+F9IiwL/8dDiB5fk1RCt0i1iE=; b=aNQ3jko+YWJK2aOqVjzpDdebgEjFlYl4Amdypx8fLyEr3ICq3FaX3Rb18aDFsOnOpX x1xiLR0xpejefpt8pEmqjnkOZobabWqFT688N5BMeCHSQpaL8AWwdJl05FodHJV2VRh2 tHhQa3YmSxeZ5i2YtzTeDhduFT1CF5uXiZLRaIHhidPIS0ASTT6Tqp32xJxzmKF0nDdz uq0nyz+2pYcHmkluKM3TkxFj6WG4F9Cm0+dRLxauWoDFc7a/nCpxEl6mopBcY9XNU3Yd 5J1Yq+VO+Jz5DxoCJFDneln9s6dB/ezLH8n/Oohd1sV7CDnbzbfcKWEDj59NG1hk3cXt aPig== X-Gm-Message-State: AGi0PuZnp18lYfPwjFwZOozUp2k4CxLo0UOEgSnWjirDar5PZ3tWk2Jl z+lVwfP60JNK017HcGdknH4= X-Google-Smtp-Source: APiQypKtJgh/IRmmjE+loIOkEOn3Mw0XgV1MSXcn1vF386coss98GvGYfGxqjqlwGh52k/8xwYzNiw== X-Received: by 2002:ac2:4257:: with SMTP id m23mr4370978lfl.141.1586993574305; Wed, 15 Apr 2020 16:32:54 -0700 (PDT) Received: from Sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id c2sm1063358ljk.97.2020.04.15.16.32.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Apr 2020 16:32:52 -0700 (PDT) Date: Thu, 16 Apr 2020 02:32:51 +0300 From: Dmitry Kozlyuk To: Thomas Monjalon Cc: dev@dpdk.org, "Dmitry Malloy (MESHCHANINOV)" , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , Anatoly Burakov , Harini Ramakrishnan , Omar Cardona , Pallavi Kadam , Ranjit Menon , bruce.richardson@intel.com, david.marchand@redhat.com Message-ID: <20200416023251.595d0c41@Sovereign> In-Reply-To: <11413927.zapYfy813O@thomas> References: <20200410164342.1194634-1-dmitry.kozliuk@gmail.com> <20200414194426.1640704-1-dmitry.kozliuk@gmail.com> <20200414194426.1640704-7-dmitry.kozliuk@gmail.com> <11413927.zapYfy813O@thomas> X-Mailer: Claws Mail 3.17.5 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 06/10] eal: introduce memory management wrappers 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" Answering the questions. All snipped comments will be fixed in v4. > [...] > > +/** > > + * Memory reservation flags. > > + */ > > +enum eal_mem_reserve_flags { > > + /**< Reserve hugepages (support may be limited or missing). */ > > + EAL_RESERVE_HUGEPAGES = 1 << 0, > > + /**< Fail if requested address is not available. */ > > + EAL_RESERVE_EXACT_ADDRESS = 1 << 1 > > +}; > > Maybe more context is needed to understand the meaning of these flags. Will extend the comment in v4. It's basically MAP_HUGE and MAP_FIXED. > [...] > > -eal_get_virtual_area(void *requested_addr, size_t *size, > > - size_t page_sz, int flags, int mmap_flags); > > +eal_get_virtual_area(void *requested_addr, size_t *size, size_t page_sz, > > + int flags, int mmap_flags); > > Is there any change here? No, will fix this artifact. > [...] > > + * If @code virt @endcode and @code size @endcode describe a part of the > > I am not sure about using @code. > It makes reading from source harder. > Is there a real benefit? It should be either @p or no markup (as in the rest of the comments), @code is indeed inappropriate. > > + > > +/** > > + * Memory mapping additional flags. > > + * > > + * In Linux and FreeBSD, each flag is semantically equivalent > > + * to OS-specific mmap(3) flag with the same or similar name. > > + * In Windows, POSIX and MAP_ANONYMOUS semantics are followed. > > + */ > > I don't understand this comment. > The flags and meanings are the same no matter the OS, right? Correct. MAP_ANONYMOUS is not POSIX so I mentioned it explicitly. I'll try to come up with better wording. > > +static void * > > +mem_map(void *requested_addr, size_t size, int prot, int flags, > > + int fd, size_t offset) > > +{ > > + void *virt = mmap(requested_addr, size, prot, flags, fd, offset); > > + if (virt == MAP_FAILED) { > > + RTE_LOG(ERR, EAL, > > Not sure it should be a log level so high. > We could imagine checking a memory map. > What about INFO level? > The real error log will be made by the caller. > > > + "Cannot mmap(%p, 0x%zx, 0x%x, 0x%x, %d, 0x%zx): %s\n", > > + requested_addr, size, prot, flags, fd, offset, > > + strerror(errno)); > > + rte_errno = errno; > > + return NULL; The same level is used now in places from which this code is extracted: lib/librte_eal/common/{eal_common_fbarray.c:97,eal_common_memory:131}, see also lib/librte_pci/rte_pci.c:144. To my understanding, DEBUG is used to log implementation-specific details like these OS API calls, so I'll change level to that. > [...] > > +int > > +rte_get_page_size(void) > > +{ > > + return getpagesize(); > > +} > > + > > +int > > +rte_mem_lock(const void *virt, size_t size) > > +{ > > + return mlock(virt, size); > > +} > > Why don't you replace existing code with these new functions? In this patchset I tried to touch existing code as little as possible, at least I'd like to limit the scope to EAL. Libraries and drivers using Unix functions directly will fail to compile when enabled on Windows, but patches will be trivial. I propose replacing calls in EAL in v4. -- Dmitry Kozlyuk