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 0AEE1A059A; Sat, 11 Apr 2020 03:16:54 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4FD921C1B5; Sat, 11 Apr 2020 03:16:54 +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 4CC6C1C196 for ; Sat, 11 Apr 2020 03:16:53 +0200 (CEST) Received: by mail-lf1-f66.google.com with SMTP id 131so2483448lfh.11 for ; Fri, 10 Apr 2020 18:16:53 -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=1Jj/6Ypu78zD4EAhUeVfw+11ikEouJf4KdyM73h+O8I=; b=WCtWtGPmLhKcfZ1Tqi3ygKWYOGjKJsyIulAYxqLkITa8QliBBb724/njocmUBCoMSu xtr/CNQyadRafOInu4/8uyD3QvAwdVKIJsFll3B5fQ551VFlcqUVBp0ziLEvcw6FWYCy RxM+DTP9nhrEd1U0bB10Hh8wbdv+QECxzTIgaZ4ZStXK+Be+M9mDm3eDY2mYo1zW39da LKkCmbUUaWBTs5ej9eXgfr392ytOuQQe1QDHMQg4HORXQxZvKj+Tv3QwdD3+CgM1DC47 VgPxMFciZCygPRVj21axFmjyjncpi6nglB5+B7WV/+u6zeKGZG5gESR9/TdFzwQEUQ7q 9X4A== 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=1Jj/6Ypu78zD4EAhUeVfw+11ikEouJf4KdyM73h+O8I=; b=Qy6TwT0l64K+XmhpyM0TjPXxNHegxnmIRugRkUgqJTRWIk9peFtx+UV90QIsGyEwkh /oq+i0TyiteAwrrlZw8lp/UA089LaH4Uvaa53eQkoiRjTko3PZ19HmdfJlRflPfudv1u ee0afie120D2iUF8HgKmnvvM5j2frVyZNr6AKrhwV0T+guArT7gkGFcS4FWyH3wMEMFz O49OWZY2mmq2qHAxeJXNKBCgCZwDmC1ZyhUxzGRkHbFU4b+1UfQuD6tRvTLjvlcjeCrh rfkyDXg/lEkqd1TBTTm3RrucGWfkhDf5G7MBUnih+2fepPU54NCLD7s+BNF8XsYrwXBo pk/w== X-Gm-Message-State: AGi0PuarDL8I/2jz6hiC/9nLtN9j30wPuawC+qo0hcbu3sU6WQccm7+q SqWnh6TfyNC8pVnM12zTpLM= X-Google-Smtp-Source: APiQypJXXtV/ecG/wEADj8MzLrm4kI1QwJniaiBF1Q+bHeOZ22Im0qkx5AEXoDK4+LDymjflrxXuDA== X-Received: by 2002:a05:6512:695:: with SMTP id t21mr4049032lfe.158.1586567812650; Fri, 10 Apr 2020 18:16:52 -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 j13sm2356063lfb.19.2020.04.10.18.16.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Apr 2020 18:16:51 -0700 (PDT) Date: Sat, 11 Apr 2020 04:16:50 +0300 From: Dmitry Kozlyuk To: Narcisa Ana Maria Vasile Cc: dev@dpdk.org, "Dmitry Malloy (MESHCHANINOV)" , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , Thomas Monjalon , Harini Ramakrishnan , Omar Cardona , Pallavi Kadam , Ranjit Menon , John McNamara , Marko Kovacevic , Anatoly Burakov , Bruce Richardson Message-ID: <20200411041650.4cb7f3aa@Sovereign> In-Reply-To: <20200410220410.GA8780@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20200330041026.784624-1-dmitry.kozliuk@gmail.com> <20200410164342.1194634-1-dmitry.kozliuk@gmail.com> <20200410164342.1194634-11-dmitry.kozliuk@gmail.com> <20200410220410.GA8780@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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 v2 10/10] eal/windows: implement basic memory management 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" [snip] > > +static int > > +alloc_seg(struct rte_memseg *ms, void *requested_addr, int socket_id, > > + struct hugepage_info *hi) > > +{ > > + HANDLE current_process; > > + unsigned int numa_node; > > + size_t alloc_sz; > > + void *addr; > > + rte_iova_t iova = RTE_BAD_IOVA; > > + PSAPI_WORKING_SET_EX_INFORMATION info; > > + PSAPI_WORKING_SET_EX_BLOCK *page; > > + > > + if (ms->len > 0) { > > + /* If a segment is already allocated as needed, return it. */ > > + if ((ms->addr == requested_addr) && > > + (ms->socket_id == socket_id) && > > + (ms->hugepage_sz == hi->hugepage_sz)) { > > + return 0; > > + } > > + > > + /* Bugcheck, should not happen. */ > > + RTE_LOG(DEBUG, EAL, "Attempted to reallocate segment %p " > > + "(size %zu) on socket %d", ms->addr, > > + ms->len, ms->socket_id); > > + return -1; > > + } > > + > > + current_process = GetCurrentProcess(); > > + numa_node = eal_socket_numa_node(socket_id); > > + alloc_sz = hi->hugepage_sz; > > + > > + if (requested_addr == NULL) { > > + /* Request a new chunk of memory and enforce address hint. */ > > Does requested_addr being NULL means that no hint was provided? It also looks like eal_mem_alloc_socket > ignores the address hint anyway and just calls VirtualAllocExNuma with NULL for lpAddress. Maybe remove > the second part of the comment. Correct, also see below. > > > + addr = eal_mem_alloc_socket(alloc_sz, socket_id); > > + if (addr == NULL) { > > + RTE_LOG(DEBUG, EAL, "Cannot allocate %zu bytes " > > + "on socket %d\n", alloc_sz, socket_id); > > + return -1; > > + } > > + > > + if (addr != requested_addr) { > > requested_addr is NULL on this branch and we confirmed with the previous 'if' that addr is not NULL. > Should this branch be removed, since requested_addr is NULL, so there is no hint provided? Good catch, this check doesn't belong here. In fact, apart from an allocation failure, address hint might not be respected iff the hint is not aligned to hugepage boundary, which would be an allocator bug (use of an incorrect MSL). > > + RTE_LOG(DEBUG, EAL, "Address hint %p not respected, " > > + "got %p\n", requested_addr, addr); > > + goto error; > > + } > > + } else { > > + /* Requested address is already reserved, commit memory. */ > > + addr = eal_mem_commit(requested_addr, alloc_sz, socket_id); > > + if (addr == NULL) { > > + RTE_LOG(DEBUG, EAL, "Cannot commit reserved memory %p " > > + "(size %zu)\n", requested_addr, alloc_sz); > > + goto error; > > Execution jumps to 'error' with an invalid addr, so it will try to call eal_mem_decommit with NULL as parameter. > Instead of 'goto error', maybe we should return here. Will fix in v3. I'd like to also clarify the following. On Windows, eal_mem_commit() is not atomic: it may first split a reserved region, then commit it. If splitting succeeds and commit fails, the reserved region remains split into parts. eal_mem_decommit() does not coalesce these parts intentionally: they're hugepage-sized, so the next eal_mem_commit() on the same region just doesn't have to split it. > > > + } > > + } > > + > > + /* Force OS to allocate a physical page and select a NUMA node. > > + * Hugepages are not pageable in Windows, so there's no race > > + * for physical address. > > + */ > > + *(volatile int *)addr = *(volatile int *)addr; > > + > > + /* Only try to obtain IOVA if it's available, so that applications > > + * that do not need IOVA can use this allocator. > > + */ > > + if (rte_eal_using_phys_addrs()) { > > + iova = rte_mem_virt2iova(addr); > > + if (iova == RTE_BAD_IOVA) { > > + RTE_LOG(DEBUG, EAL, > > + "Cannot get IOVA of allocated segment\n"); > > + goto error; > > + } > > + } > > + > > + /* Only "Ex" function can handle hugepages. */ > > + info.VirtualAddress = addr; > > + if (!QueryWorkingSetEx(current_process, &info, sizeof(info))) { > > + RTE_LOG_WIN32_ERR("QueryWorkingSetEx()"); > > + goto error; > > + } > > + > > + page = &info.VirtualAttributes; > > + if (!page->Valid || !page->LargePage) { > > + RTE_LOG(DEBUG, EAL, "Got regular page instead of hugepage\n"); > > + goto error; > > + } > > + if (page->Node != numa_node) { > > + RTE_LOG(DEBUG, EAL, > > + "NUMA node hint %u (socket %d) not respected, got %u\n", > > + numa_node, socket_id, page->Node); > > + goto error; > > + } > > + > > + ms->addr = addr; > > + ms->hugepage_sz = hi->hugepage_sz; > > + ms->len = alloc_sz; > > + ms->nchannel = rte_memory_get_nchannel(); > > + ms->nrank = rte_memory_get_nrank(); > > + ms->iova = iova; > > + ms->socket_id = socket_id; > > + > > + return 0; > > + > > +error: > > + /* Only jump here when `addr` and `alloc_sz` are valid. */ > > + eal_mem_decommit(addr, alloc_sz); > > + return -1; > > +} [snip] -- Dmitry Kozlyuk