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 B41F3A0588; Thu, 16 Apr 2020 00:17:40 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E9EBB1D659; Thu, 16 Apr 2020 00:17:39 +0200 (CEST) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by dpdk.org (Postfix) with ESMTP id B20C41D645 for ; Thu, 16 Apr 2020 00:17:37 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 0642B580331; Wed, 15 Apr 2020 18:17:37 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Wed, 15 Apr 2020 18:17:37 -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=mesmtp; bh=F+mizhUBgpYy+tJ7SeRU1tE2hpU5O3DEXKV5wnyzQa8=; b=g+PQO+GkayF4 hPtt8V1v5uf84hQr19OIeVJrqP9hVgy2aKVgnBMnb1Y//ghZnYHhZLY2+V/zgEGf JHTnOMo+SoSMqRnteXruc0Ln5XOh0c/EuIoSXRoXo9FBtPF+fCzJbhC84DBr9zE7 mq5x4UdY09opD1wHa+hFzi5SaWvoHsk= 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=fm2; bh=F+mizhUBgpYy+tJ7SeRU1tE2hpU5O3DEXKV5wnyzQ a8=; b=rrZaB75q4i2cdCwW2vD983H4GMUa0tAvPdXl5UY9cHoYXpNRI+ll3e5Em 6de7VBYcBGCWVBHgOBH+1Xspjlp19UHEz3/oltmvgXWp3YLsz23TTbk6oa/PS51t NPtQjxrDEsRhaX5Vl8PUYvvB0zWnnQoQ+WdGjwXf2dd0Q5jxF5psVKGljhn6LuOi PrW/D9gEWyT7FdrOyw2afsA3uEupqPnX7H/SBm27N2Qn8nzrRo4dT36UIKIQ+71i GztmVMB9j7XregCqlBXty8JD/MLscj0SZUEzVreWkBHTvZFQZejNvHqob2M7l8/i SJE/XRCyLKH8uMM7qnGD6vagZXRaQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrfeeggddtjecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth 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 00342328005D; Wed, 15 Apr 2020 18:17:34 -0400 (EDT) From: Thomas Monjalon To: Dmitry Kozlyuk 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 Date: Thu, 16 Apr 2020 00:17:33 +0200 Message-ID: <11413927.zapYfy813O@thomas> In-Reply-To: <20200414194426.1640704-7-dmitry.kozliuk@gmail.com> References: <20200410164342.1194634-1-dmitry.kozliuk@gmail.com> <20200414194426.1640704-1-dmitry.kozliuk@gmail.com> <20200414194426.1640704-7-dmitry.kozliuk@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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" 14/04/2020 21:44, Dmitry Kozlyuk: > System meory management is implemented differently for POSIX and meory -> memory > Windows. Introduce wrapper functions for operations used across DPDK: > > * rte_mem_map() > Create memory mapping for a regular file or a page file (swap). > This supports mapping to a reserved memory region even on Windows. > > * rte_mem_unmap() > Remove mapping created with rte_mem_map(). > > * rte_get_page_size() > Obtain default system page size. > > * rte_mem_lock() > Make arbitrary-sized memory region non-swappable. > > Wrappers follow POSIX semantics limited to DPDK tasks, but their > signatures deliberately differ from POSIX ones to be more safe and > expressive. > > Signed-off-by: Dmitry Kozlyuk [...] > +/** > + * 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. [...] > -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? [...] > + * 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? [...] > +/** > + * Memory protection flags. > + */ > +enum rte_mem_prot { > + RTE_PROT_READ = 1 << 0, /**< Read access. */ > + RTE_PROT_WRITE = 1 << 1, /**< Write access. */ > + RTE_PROT_EXECUTE = 1 << 2 /**< Code execution. */ > +}; Alignment of comments would look better :-) > + > +/** > + * 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? > +enum rte_map_flags { > + /** Changes of mapped memory are visible to other processes. */ > + RTE_MAP_SHARED = 1 << 0, > + /** Mapping is not backed by a regular file. */ > + RTE_MAP_ANONYMOUS = 1 << 1, > + /** Copy-on-write mapping, changes are invisible to other processes. */ > + RTE_MAP_PRIVATE = 1 << 2, > + /** Fail if requested address cannot be taken. */ > + RTE_MAP_FIXED = 1 << 3 > +}; > + > +/** > + * OS-independent implementation of POSIX mmap(3) > + * with MAP_ANONYMOUS Linux/FreeBSD extension. > + */ > +__rte_experimental > +void *rte_mem_map(void *requested_addr, size_t size, enum rte_mem_prot prot, > + enum rte_map_flags flags, int fd, size_t offset); > + > +/** > + * OS-independent implementation of POSIX munmap(3). > + */ > +__rte_experimental > +int rte_mem_unmap(void *virt, size_t size); > + > +/** > + * Get system page size. This function never failes. failes -> fails > + * > + * @return > + * Positive page size in bytes. > + */ > +__rte_experimental > +int rte_get_page_size(void); > + > +/** > + * Lock region in physical memory and prevent it from swapping. > + * > + * @param virt > + * The virtual address. > + * @param size > + * Size of the region. > + * @return > + * 0 on success, negative on error. > + * > + * @note Implementations may require @p virt and @p size to be multiples > + * of system page size. > + * @see rte_get_page_size() > + * @see rte_mem_lock_page() > + */ > +__rte_experimental > +int rte_mem_lock(const void *virt, size_t size); [...] > --- /dev/null > +++ b/lib/librte_eal/unix/eal_memory.c License and copyright missing. > @@ -0,0 +1,113 @@ > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include "eal_private.h" > + > +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; [...] > +void * > +eal_mem_reserve(void *requested_addr, size_t size, > + enum eal_mem_reserve_flags flags) > +{ > + int sys_flags = MAP_PRIVATE | MAP_ANONYMOUS; > + > +#ifdef MAP_HUGETLB > + if (flags & EAL_RESERVE_HUGEPAGES) > + sys_flags |= MAP_HUGETLB; > +#endif If MAP_HUGETLB is false, and flags contain EAL_RESERVE_HUGEPAGES, I think an error should be returned. > + if (flags & EAL_RESERVE_EXACT_ADDRESS) > + sys_flags |= MAP_FIXED; > + > + return mem_map(requested_addr, size, PROT_NONE, sys_flags, -1, 0); > +} [...] > +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?