From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergio Gonzalez Monroy Subject: Re: [PATCH v3] mem: command line option to delete hugepage backing files Date: Fri, 23 Oct 2015 10:57:45 +0100 Message-ID: <562A0499.6030302@intel.com> References: <1445419101-19690-1-git-send-email-shesha@cisco.com> <20151021163422.GA10344@bricha3-MOBL3> <5628A390.9080602@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "shesha Sreenivasamurthy (shesha)" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 5CD52C4D4 for ; Fri, 23 Oct 2015 11:58:09 +0200 (CEST) In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 22/10/2015 17:03, shesha Sreenivasamurthy (shesha) wrote: > Sergio, > Your comment regarding remap_all_functions is correct and can be fix= ed > by unlinking in remap_all_hugepages() too. However, regarding you comme= nt > that =C2=B3unmap_unneeded_hugepages=C2=B2 will fail =E2=80=B9 in the > unmap_unneeded_hugepages() we do not unlink if final_va is equal to NUL= L > guarded by RTE_EAL_SINGLE_FILE_SEGMENTS. My testing did not catch as > RTE_EAL_SINGLE_FILE_SEGMENTS was set. Is there any reason why we should > not skip unlinking if final_va is null always (removing ifdef > RTE_EAL_SINGLE_FILE_SEGMENTS) ? The issue with unmap_unneeded_hugepages happens regardless of=20 SINGLE_FILE_SEGMENT being set or not. The problem is that in that function, it assumes that no file has been=20 unlinked. In fact, with SINGLE_FILE_SEGMENT, it might re-open files again if it=20 needs to truncate it, so we need those files present in the file system. > However, if you think having a separate function is better, I am all fo= r > it. My initial thought was the same and to do the unlinking inside an=20 existing function, but as you may realized, the code is not the most straight forward, and=20 the resulting diff may be even bigger than with a single function. I think the single function in v3 works properly because we only unlink=20 the files left after we have done all this mapping-unmapping. Sergio > -- > - Thanks > char * (*shesha) (uint64_t cache, uint8_t F00D) > { return 0x0000C0DE; } > > > -----Original Message----- > From: Sergio Gonzalez Monroy > Date: Thursday, October 22, 2015 at 1:51 AM > To: Cisco Employee > Cc: "dev@dpdk.org" , Bruce Richardson > > Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete > hugepage backing files > > On 21/10/2015 17:34, Bruce Richardson wrote: >> On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy >> (shesha) wrote: >>> When an application using huge-pages crash or exists, the hugetlbfs >>> backing files are not cleaned up. This is a patch to clean those file= s. >>> There are multi-process DPDK applications that may be benefited by th= ose >>> backing files. Therefore, I have made that configurable so that the >>> application that does not need those backing files can remove them, t= hus >>> not changing the current default behavior. The application itself can >>> clean it up, however the rationale behind DPDK cleaning it up is, DPD= K >>> created it and therefore, it is better it unlinks it. >>> >>> Signed-off-by: Shesha Sreenivasamurthy >>> --- >>> lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++ >>> lib/librte_eal/common/eal_internal_cfg.h | 1 + >>> lib/librte_eal/common/eal_options.h | 2 ++ >>> lib/librte_eal/linuxapp/eal/eal_memory.c | 30 >>> ++++++++++++++++++++++++++++++ >>> 4 files changed, 45 insertions(+) >>> >> >>> +static int >>> +unlink_hugepage_files(struct hugepage_file *hugepg_tbl, >>> + unsigned num_hp_info) >>> +{ >>> + unsigned socket, size; >>> + int page, nrpages =3D 0; >>> + >>> + /* get total number of hugepages */ >>> + for (size =3D 0; size < num_hp_info; size++) >>> + for (socket =3D 0; socket < RTE_MAX_NUMA_NODES; socket++) >>> + nrpages +=3D internal_config.hugepage_info[size].num_pages[socket= ]; >>> + >>> + for (page =3D 0; page < nrpages; page++) { >>> + struct hugepage_file *hp =3D &hugepg_tbl[page]; >>> + if (hp->final_va !=3D NULL && unlink(hp->filepath)) { >>> + RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n", >>> + __func__, hp->filepath, strerror(errno)); >>> + } >>> + } >>> + return 0; >>> +} >>> + >>> /* >>> * unmaps hugepages that are not going to be used. since we origin= ally >>> allocate >>> * ALL hugepages (not just those we need), additional unmapping ne= eds >>> to >>> be done. >>> @@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void) >>> goto fail; >>> } >>> =20 >>> + /* free the hugepage backing files */ >>> + if (internal_config.hugepage_unlink && >>> + unlink_hugepage_files(tmp_hp, >>> + internal_config.num_hugepage_sizes) < 0) { >>> + RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n"); >>> + goto fail; >>> + } >>> + >> Sorry for the late comment, but... >> >> Rather than adding a whole new function to be called here, can the sam= e >> effect >> not be got by adding in 2/3 lines like: >> if (internal_config.hugepage_unlink) >> unlink(hugetlb[i].filepath) >> >> at line 409 of eal_memory.c where were have done our final mmap of the >> file. >> [You also need the same couple of lines for the 32-bit special case at >> line 351]. >> It would be a shorter diff. >> >> /Bruce > If you wanted to avoid the extra function call, I might be cleaner to > just unlink all files when > doing unmap_all_hugepages_orig. > My two cents: I think it would be easier to read/debug having a functio= n > that "unlinks files" instead > of unlinking files at different points in map_all_hugepages. > > Unfortunately the proposed approach does not work for all cases: > - If we have single file segment, map_all_hugepages does not get call a > second time, instead we call > remap_all_hugepages > - If we use options -m or --socket-mem, because unmap_unneeded_hugepage= s > does not expect files > already unlinked, it will fail when trying to unlink unneeded > hugepage files. > > The current patch would work as we only unlink after > unmap_unneeded_hugepages. > > Sergio > >