From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 384C8C43334 for ; Fri, 10 Jun 2022 14:52:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F7A38D00BF; Fri, 10 Jun 2022 10:52:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A7978D00B3; Fri, 10 Jun 2022 10:52:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5FA898D00BF; Fri, 10 Jun 2022 10:52:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 46BB18D00B3 for ; Fri, 10 Jun 2022 10:52:06 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 0D81281501 for ; Fri, 10 Jun 2022 14:52:06 +0000 (UTC) X-FDA: 79562616252.02.6DD5756 Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf04.hostedemail.com (Postfix) with ESMTP id A214C4002D for ; Fri, 10 Jun 2022 14:52:05 +0000 (UTC) Received: by mail-lf1-f52.google.com with SMTP id a15so43219914lfb.9 for ; Fri, 10 Jun 2022 07:52:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5f+Waz9DKOzVJw8cnavLk1yh8n2DotWHZ3M6vDecDqY=; b=gCBiahf5R+LsGkEpKBxrIDJ3pbwHYt3Rw1VeT/5IP4yckKm8WaulZ/Na7zD2bXFhJd nsS/fySRFy6Uk9shzJQAUhyWcwMx4sUOLXsWhIQn1uZbMkuaU6L/jxAELsSXXWHXlKp4 Yd7UQgNb//+4Uaf88TC4Pm8T5pnkTqJeuzIulkjUb9hRxKO2hXEK0ZsBpeF87vXfEeW1 SIVKcD3mXlNdXmm/JLJ7VguD6oK9GHvVw5E2WvBtFnvodDkAaeOLeVkoUpcWbeMNBFKE /jI+1baINTVvGZfi+lwceQhzDNHKG8Ypjet6Frr8vbhtu8w/QJljgJqOk1t1ccYoeDkX w04A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5f+Waz9DKOzVJw8cnavLk1yh8n2DotWHZ3M6vDecDqY=; b=YdrYlOHs8sV22490upDrkVM3cCHG/We+RKN3WuAcYuNyDSxt2mGS3SESGFbb0EOCeo yjt6X6U2nLwwGOrGt0MMWCBBIC9Uta8tpZllC4TRgbwuvQj3/xfVSsK901eeWv5Ahs7Z ju0UFE9ZqZv7EI3pGL288MpRNMlQu1h1BdC029eXwiZ8qnQy+QiAC11/P2GZjK9usYRY nXtwu4YPHoQaQsg8peEicgV4i41mGxMRYjPmHQi1tWXlIYbGB3Lh0LXdYFr7PPOSPLqo Ob23DONBTx29eo69SQMyNy/G1Nu+vcUgiiwuXveWgenF90A1NXfrXU5umf52A5u2vEtx ZQvA== X-Gm-Message-State: AOAM533G7ENqy2z5gAM46V7qhy5a/upUrAVV7MMs/V71EZFLy11cU8lT 60EFomCpsb1uLFP/eF0li5xFG0RQxPLIk4ord9IcWQ== X-Google-Smtp-Source: ABdhPJxh4DnvFPwg06xMZU+T6VMVatC2K65rJMnX8Gz4zvsUI0xJtg1KdOFtgObvPfnZYADTUu3DJ5lukk+1owtjOTE= X-Received: by 2002:a05:6512:249:b0:479:a3c:de with SMTP id b9-20020a056512024900b004790a3c00demr28430968lfo.128.1654872723482; Fri, 10 Jun 2022 07:52:03 -0700 (PDT) MIME-Version: 1.0 References: <20220604004004.954674-1-zokeefe@google.com> <20220604004004.954674-10-zokeefe@google.com> In-Reply-To: From: "Zach O'Keefe" Date: Fri, 10 Jun 2022 07:51:20 -0700 Message-ID: Subject: Re: [PATCH v6 09/15] mm/madvise: introduce MADV_COLLAPSE sync hugepage collapse To: Yang Shi Cc: Alex Shi , David Hildenbrand , David Rientjes , Matthew Wilcox , Michal Hocko , Pasha Tatashin , Peter Xu , Rongwei Wang , SeongJae Park , Song Liu , Vlastimil Babka , Zi Yan , Linux MM , Andrea Arcangeli , Andrew Morton , Arnd Bergmann , Axel Rasmussen , Chris Kennelly , Chris Zankel , Helge Deller , Hugh Dickins , Ivan Kokshaysky , "James E.J. Bottomley" , Jens Axboe , "Kirill A. Shutemov" , Matt Turner , Max Filippov , Miaohe Lin , Minchan Kim , Patrick Xia , Pavel Begunkov , Thomas Bogendoerfer Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1654872725; a=rsa-sha256; cv=none; b=atAMx4aLI/RhsXG73Nzy8tRaWg8H4HAPZNISLzlyDXwnD4iRIPp/DoEqY6Oz8wWc5sDCVL zYOCgMyKmMDPQzX38cqze0/im7M24YcpT1Uex5UCKv6CWmPxwwyeVQNdZ1r2mFiVlcP2FO S//rakkW8so/P+cON2PKwK6xnAgLQDg= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=gCBiahf5; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of zokeefe@google.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1654872725; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5f+Waz9DKOzVJw8cnavLk1yh8n2DotWHZ3M6vDecDqY=; b=sFtmB7kIs2hnEv1JTGZpkk/rJKSKm17bUlzkz07u7TQJLEmE6tEnplnP6IQIM4Cmn/T6gm Z+IshSpmsuTBdyyEzF91f4/UwVvRgp5H/UGwzRHRNhzB27HR3Bq4sVKR0RQipMw1XzAdOT qV3Cb9ryJdJE/y0zGHOjXmLGfFEX7Q0= X-Stat-Signature: scwwcbanepsyeqzua834gm63hogcrtya X-Rspamd-Queue-Id: A214C4002D X-Rspam-User: X-Rspamd-Server: rspam10 Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=gCBiahf5; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of zokeefe@google.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=zokeefe@google.com X-HE-Tag: 1654872725-873207 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jun 9, 2022 at 11:52 AM Yang Shi wrote: > > On Thu, Jun 9, 2022 at 10:35 AM Zach O'Keefe wrote: > > > > On Tue, Jun 7, 2022 at 5:39 PM Yang Shi wrote: > > > > > > On Tue, Jun 7, 2022 at 3:48 PM Zach O'Keefe wrote: > > > > > > > > On Mon, Jun 6, 2022 at 4:53 PM Yang Shi wrote: > > > > > > > > > > On Fri, Jun 3, 2022 at 5:40 PM Zach O'Keefe wrote: > > > > > > > > > > > > This idea was introduced by David Rientjes[1]. > > > > > > > > > > > > Introduce a new madvise mode, MADV_COLLAPSE, that allows users to request a > > > > > > synchronous collapse of memory at their own expense. > > > > > > > > > > > > The benefits of this approach are: > > > > > > > > > > > > * CPU is charged to the process that wants to spend the cycles for the > > > > > > THP > > > > > > * Avoid unpredictable timing of khugepaged collapse > > > > > > > > > > > > An immediate user of this new functionality are malloc() implementations > > > > > > that manage memory in hugepage-sized chunks, but sometimes subrelease > > > > > > memory back to the system in native-sized chunks via MADV_DONTNEED; > > > > > > zapping the pmd. Later, when the memory is hot, the implementation > > > > > > could madvise(MADV_COLLAPSE) to re-back the memory by THPs to regain > > > > > > hugepage coverage and dTLB performance. TCMalloc is such an > > > > > > implementation that could benefit from this[2]. > > > > > > > > > > > > Only privately-mapped anon memory is supported for now, but it is > > > > > > expected that file and shmem support will be added later to support the > > > > > > use-case of backing executable text by THPs. Current support provided > > > > > > by CONFIG_READ_ONLY_THP_FOR_FS may take a long time on a large system > > > > > > which might impair services from serving at their full rated load after > > > > > > (re)starting. Tricks like mremap(2)'ing text onto anonymous memory to > > > > > > immediately realize iTLB performance prevents page sharing and demand > > > > > > paging, both of which increase steady state memory footprint. With > > > > > > MADV_COLLAPSE, we get the best of both worlds: Peak upfront performance > > > > > > and lower RAM footprints. > > > > > > > > > > > > This call is independent of the system-wide THP sysfs settings, but will > > > > > > fail for memory marked VM_NOHUGEPAGE. > > > > > > > > > > > > THP allocation may enter direct reclaim and/or compaction. > > > > > > > > > > > > [1] https://lore.kernel.org/linux-mm/d098c392-273a-36a4-1a29-59731cdf5d3d@google.com/ > > > > > > [2] https://github.com/google/tcmalloc/tree/master/tcmalloc > > > > > > > > > > > > Suggested-by: David Rientjes > > > > > > Signed-off-by: Zach O'Keefe > > > > > > --- > > > > > > arch/alpha/include/uapi/asm/mman.h | 2 + > > > > > > arch/mips/include/uapi/asm/mman.h | 2 + > > > > > > arch/parisc/include/uapi/asm/mman.h | 2 + > > > > > > arch/xtensa/include/uapi/asm/mman.h | 2 + > > > > > > include/linux/huge_mm.h | 12 +++ > > > > > > include/uapi/asm-generic/mman-common.h | 2 + > > > > > > mm/khugepaged.c | 124 +++++++++++++++++++++++++ > > > > > > mm/madvise.c | 5 + > > > > > > 8 files changed, 151 insertions(+) > > > > > > > > > > > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h > > > > > > index 4aa996423b0d..763929e814e9 100644 > > > > > > --- a/arch/alpha/include/uapi/asm/mman.h > > > > > > +++ b/arch/alpha/include/uapi/asm/mman.h > > > > > > @@ -76,6 +76,8 @@ > > > > > > > > > > > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */ > > > > > > > > > > > > +#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */ > > > > > > + > > > > > > /* compatibility flags */ > > > > > > #define MAP_FILE 0 > > > > > > > > > > > > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h > > > > > > index 1be428663c10..c6e1fc77c996 100644 > > > > > > --- a/arch/mips/include/uapi/asm/mman.h > > > > > > +++ b/arch/mips/include/uapi/asm/mman.h > > > > > > @@ -103,6 +103,8 @@ > > > > > > > > > > > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */ > > > > > > > > > > > > +#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */ > > > > > > + > > > > > > /* compatibility flags */ > > > > > > #define MAP_FILE 0 > > > > > > > > > > > > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h > > > > > > index a7ea3204a5fa..22133a6a506e 100644 > > > > > > --- a/arch/parisc/include/uapi/asm/mman.h > > > > > > +++ b/arch/parisc/include/uapi/asm/mman.h > > > > > > @@ -70,6 +70,8 @@ > > > > > > #define MADV_WIPEONFORK 71 /* Zero memory on fork, child only */ > > > > > > #define MADV_KEEPONFORK 72 /* Undo MADV_WIPEONFORK */ > > > > > > > > > > > > +#define MADV_COLLAPSE 73 /* Synchronous hugepage collapse */ > > > > > > + > > > > > > #define MADV_HWPOISON 100 /* poison a page for testing */ > > > > > > #define MADV_SOFT_OFFLINE 101 /* soft offline page for testing */ > > > > > > > > > > > > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h > > > > > > index 7966a58af472..1ff0c858544f 100644 > > > > > > --- a/arch/xtensa/include/uapi/asm/mman.h > > > > > > +++ b/arch/xtensa/include/uapi/asm/mman.h > > > > > > @@ -111,6 +111,8 @@ > > > > > > > > > > > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */ > > > > > > > > > > > > +#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */ > > > > > > + > > > > > > /* compatibility flags */ > > > > > > #define MAP_FILE 0 > > > > > > > > > > > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > > > > > > index 648cb3ce7099..2ca2f3b41fc8 100644 > > > > > > --- a/include/linux/huge_mm.h > > > > > > +++ b/include/linux/huge_mm.h > > > > > > @@ -240,6 +240,9 @@ void __split_huge_pud(struct vm_area_struct *vma, pud_t *pud, > > > > > > > > > > > > int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags, > > > > > > int advice); > > > > > > +int madvise_collapse(struct vm_area_struct *vma, > > > > > > + struct vm_area_struct **prev, > > > > > > + unsigned long start, unsigned long end); > > > > > > void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start, > > > > > > unsigned long end, long adjust_next); > > > > > > spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma); > > > > > > @@ -395,6 +398,15 @@ static inline int hugepage_madvise(struct vm_area_struct *vma, > > > > > > BUG(); > > > > > > return 0; > > > > > > } > > > > > > + > > > > > > +static inline int madvise_collapse(struct vm_area_struct *vma, > > > > > > + struct vm_area_struct **prev, > > > > > > + unsigned long start, unsigned long end) > > > > > > +{ > > > > > > + BUG(); > > > > > > + return 0; > > > > > > > > > > I wish -ENOSYS could have been returned, but it seems madvise() > > > > > doesn't support this return value. > > > > > > > > This is somewhat tangential, but I agree that ENOSYS (or some other > > > > errno, but ENOSYS makes most sense to me, after EINVAL, (ENOTSUP?)) > > > > should be anointed the dedicated return value for "madvise mode not > > > > supported". Ran into this recently when wanting some form of feature > > > > detection for MADV_COLLAPSE where EINVAL is overloaded (including > > > > madvise mode not supported). Happy to move this forward if others > > > > agree. > > > > > > I did a quick test by calling MADV_HUGEPAGE on !THP kernel, madvise() > > > actually returns -EINVAL by madvise_behavior_valid(). So > > > madvise_collapse() won't be called at all. So madvise_collapse() is > > > basically used to make !THP compile happy. > > > > Ya, exactly. I was thinking -ENOTSUP could be used in > > place of -ENVAL in the madvise_behavior_valid() path to tell callers > > of madvise(2) if a given madvise mode was supported or not. At the > > moment -EINVAL return could mean a number of different things. Anyways > > - that's a side conversation. > > > > > I think we could just return -EINVAL. > > > > That sounds fine - as you mention it's code that shouldn't be called > > anyways and is just there to satisfy !THP. Was just basing off > > hugepage_madvise(). > > You could modify huepage_madvise() to simply return -EINVAL in your > patch too. It is not worth for a separate patch IMHO. Sure, if you think it's a worthwhile cleanup to remove a BUG(), then I don't see the harm. > > > > > > > > > > > > +} > > > > > > + > > > > > > static inline void vma_adjust_trans_huge(struct vm_area_struct *vma, > > > > > > unsigned long start, > > > > > > unsigned long end, > > > > > > diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h > > > > > > index 6c1aa92a92e4..6ce1f1ceb432 100644 > > > > > > --- a/include/uapi/asm-generic/mman-common.h > > > > > > +++ b/include/uapi/asm-generic/mman-common.h > > > > > > @@ -77,6 +77,8 @@ > > > > > > > > > > > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */ > > > > > > > > > > > > +#define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */ > > > > > > + > > > > > > /* compatibility flags */ > > > > > > #define MAP_FILE 0 > > > > > > > > > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > > > > > index 4ad04f552347..073d6bb03b37 100644 > > > > > > --- a/mm/khugepaged.c > > > > > > +++ b/mm/khugepaged.c > > > > > > @@ -2404,3 +2404,127 @@ void khugepaged_min_free_kbytes_update(void) > > > > > > set_recommended_min_free_kbytes(); > > > > > > mutex_unlock(&khugepaged_mutex); > > > > > > } > > > > > > + > > > > > > +static int madvise_collapse_errno(enum scan_result r) > > > > > > +{ > > > > > > + switch (r) { > > > > > > + case SCAN_PMD_NULL: > > > > > > + case SCAN_ADDRESS_RANGE: > > > > > > + case SCAN_VMA_NULL: > > > > > > + case SCAN_PTE_NON_PRESENT: > > > > > > + case SCAN_PAGE_NULL: > > > > > > + /* > > > > > > + * Addresses in the specified range are not currently mapped, > > > > > > + * or are outside the AS of the process. > > > > > > + */ > > > > > > + return -ENOMEM; > > > > > > + case SCAN_ALLOC_HUGE_PAGE_FAIL: > > > > > > + case SCAN_CGROUP_CHARGE_FAIL: > > > > > > + /* A kernel resource was temporarily unavailable. */ > > > > > > + return -EAGAIN; > > > > > > > > > > I thought this should return -ENOMEM too. > > > > > > > > Do you mean specifically SCAN_CGROUP_CHARGE_FAIL? > > > > > > No, I mean both. > > > > > > > > > > > At least going by the comment above do_madvise(), and in the man > > > > pages, for ENOMEM: "Addresses in the specified range are not currently > > > > mapped, or are outside the address space of the process." doesn't > > > > really apply here (though I don't know if "A kernel resource was > > > > temporarily unavailable" applies any better). > > > > > > Yes, the man page does say so. But IIRC some MADV_ operations do > > > return -ENOMEM for memory allocation failure, for example, > > > MADV_POPULATE_READ/WRITE. Typically the man pages don't cover all > > > cases. > > > > Good point, I missed MADV_POPULATE_READ/WRITE didn't go through the > > -ENOMEM -> -EAGAIN remapping at the bottom of madvise_vma_behavior(). > > > > > > > > > > That said, should we differentiate between allocation and charging > > > > failure? At least in the case of a userspace agent using > > > > process_madvise(2) to collapse memory on behalf of others, knowing > > > > "this memcg is at its limit" vs "no THPs available" would be valuable. > > > > Maybe the former should be EBUSY? > > > > > > IMHO we don't have to differentiate allocation and charging. > > > > After some consideration (thanks for starting this discussion and > > prompting me to do so), I do think it's very valuable for callers to > > know when THP allocation fails, and that an errno should be reserved > > for that. The caller needs to know when a generic error, specific to > > the memory being collapsed, occurs vs THP allocation failure to help > > guide next actions: fallback to other strategy, sleep, MADV_DONTNEED / > > free memory elsewhere, etc. > > > > As a concrete example, process init code that tries to back certain > > segments of text by hugepages. Some existing strategies folks use for > > this are CONFIG_READ_ONLY_THP_FOR_FS + khugepaged, and anon mremap(2) > > tricks. CONFIG_READ_ONLY_THP_FOR_FS + MADV_COLLPASE might add a third, > > and if it fails, it'd be nice to know which other option to fall back > > to, depending on how badly the user wants THP backing. If THP > > allocation fails, then likely the anon mremap(2) trick will fail too > > (unless some reclaim/compaction is done). > > > > Less immediately concrete, but a userspace agent seeking to optimize > > system-wide THP utilization surely wants to know when it's exhausted > > its precious THP supply. > > > > So I'd like to see -EAGAIN reserved for THP allocation failure > > (-ENOMEM is taken by AS errors, and it'd be nice to be consistent with > > other modes here). I think -EBUSY for memcg charging makes sense, and > > tells the caller something actionable and useful, so I'd like to see > > it differentiated from -ENOMEM. > > OK, it makes some sense to differentiate from -ENOMEM. But I still > don't see too much value to differentiate allocation failure vs > charging failure. When charging is failed other tricks are unlikely to > succeed either IMHO unless more aggressive reclaim is done. > > But GFP_TRANSHUGE is used by MADV_COLLAPSE, it means direct reclaim > has been tried before returning failure for both allocation and > charging. For fallback measures / actions regarding what to do for a single process / memcg, I agree. However, at least in the system-wide case where we might be responsible for process_madvise(MADV_COLLAPSE)'ing memory from memcg A and B, a memcg charge failure for A shouldn't impact a decision to collapse memory for B - whereas a THP allocation failure encountered when attempting collapse of A likely means the same would happen if we tried to collapse memory for B. > > > > Would appreciate feedback from folks here before setting these in > > stone and preventing the errno from ever being useful to callers. > > > > > > > > > > > > + default: > > > > > > + return -EINVAL; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev, > > > > > > + unsigned long start, unsigned long end) > > > > > > +{ > > > > > > + struct collapse_control cc = { > > > > > > + .enforce_page_heuristics = false, > > > > > > + .enforce_thp_enabled = false, > > > > > > + .last_target_node = NUMA_NO_NODE, > > > > > > + .gfp = GFP_TRANSHUGE | __GFP_THISNODE, > > > > > > + }; > > > > > > + struct mm_struct *mm = vma->vm_mm; > > > > > > + unsigned long hstart, hend, addr; > > > > > > + int thps = 0, last_fail = SCAN_FAIL; > > > > > > + bool mmap_locked = true; > > > > > > + > > > > > > + BUG_ON(vma->vm_start > start); > > > > > > + BUG_ON(vma->vm_end < end); > > > > > > + > > > > > > + *prev = vma; > > > > > > + > > > > > > + /* TODO: Support file/shmem */ > > > > > > + if (!vma->anon_vma || !vma_is_anonymous(vma)) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + hstart = (start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK; > > > > > > + hend = end & HPAGE_PMD_MASK; > > > > > > + > > > > > > + /* > > > > > > + * Set VM_HUGEPAGE so that hugepage_vma_check() can pass even if > > > > > > + * TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG is set (i.e. "madvise" mode). > > > > > > + * Note that hugepage_vma_check() doesn't enforce that > > > > > > + * TRANSPARENT_HUGEPAGE_FLAG or TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG > > > > > > + * must be set (i.e. "never" mode) > > > > > > + */ > > > > > > + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) > > > > > > > > > > hugepage_vma_check() doesn't check vma size, so MADV_COLLAPSE may be > > > > > running for a unsuitable vma, hugepage_vma_revalidate() called by > > > > > khugepaged_scan_pmd() may find it out finally, but it is a huge waste > > > > > of effort. So, it is better to check vma size upfront. > > > > > > > > This actually does check the vma size, but it's subtle. hstart and > > > > hend are clamped to the first/last > > > > hugepaged-aligned address covered by [start,end], which are themselves > > > > contained in vma->vm_start/vma->vm_end, respectively. We then check > > > > that addr = hstart < hend ; so if the main loop passes the first > > > > check, we know that vma->vm_start <= addr and addr + HPAGE_PMD_SIZE <= > > > > > > Aha, yes, I overlooked that. > > > > > > > vma->vma_end. Agreed that we might be needlessly doing mmgrab() and > > > > lru_add_drain() needlessly though. > > > > > > Yeah > > > > > > > > > > > > BTW, my series moved the vma size check in hugepage_vma_check(), so if > > > > > your series could be based on top of that, you get that for free. > > > > > > > > I'll try rebasing on top of your series, thank you! > > > > > > You don't have to do it right now. I don't know what series will be > > > merged to mm tree first. Just a heads up. > > > > Thanks! Seems beneficial here though, so I'll do that and add a note > > in the cover letter. > > Thank you so much. That would make my life easier :-) Happy to see the cleanup! Thanks again for that. > > > > > > > > > > > > + return -EINVAL; > > > > > > + > > > > > > + mmgrab(mm); > > > > > > + lru_add_drain(); > > > > > > + > > > > > > + for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) { > > > > > > + int result = SCAN_FAIL; > > > > > > + bool retry = true; /* Allow one retry per hugepage */ > > > > > > +retry: > > > > > > + if (!mmap_locked) { > > > > > > + cond_resched(); > > > > > > + mmap_read_lock(mm); > > > > "> > + mmap_locked = true; > > > > > > + result = hugepage_vma_revalidate(mm, addr, &vma, &cc); > > > > > > > > > > How's about making hugepage_vma_revalidate() return SCAN_SUCCEED too? > > > > > It seems more consistent. > > > > > > > > Ya, I didn't like this either. I'll add this to "mm/khugepaged: pipe > > > > enum scan_result codes back to callers" > > > > > > > > > > + if (result) { > > > > > > + last_fail = result; > > > > > > + goto out_nolock; > > > > > > + } > > > > > > + } > > > > > > + mmap_assert_locked(mm); > > > > > > + memset(cc.node_load, 0, sizeof(cc.node_load)); > > > > > > + result = khugepaged_scan_pmd(mm, vma, addr, &mmap_locked, &cc); > > > > > > + if (!mmap_locked) > > > > > > + *prev = NULL; /* Tell caller we dropped mmap_lock */ > > > > > > + > > > > > > + switch (result) { > > > > > > + case SCAN_SUCCEED: > > > > > > + case SCAN_PMD_MAPPED: > > > > > > + ++thps; > > > > > > + break; > > > > > > + /* Whitelisted set of results where continuing OK */ > > > > > > + case SCAN_PMD_NULL: > > > > > > + case SCAN_PTE_NON_PRESENT: > > > > > > + case SCAN_PTE_UFFD_WP: > > > > > > + case SCAN_PAGE_RO: > > > > > > + case SCAN_LACK_REFERENCED_PAGE: > > > > > > + case SCAN_PAGE_NULL: > > > > > > + case SCAN_PAGE_COUNT: > > > > > > + case SCAN_PAGE_LOCK: > > > > > > + case SCAN_PAGE_COMPOUND: > > > > > > + last_fail = result; > > > > > > + break; > > > > > > + case SCAN_PAGE_LRU: > > > > > > + if (retry) { > > > > > > + lru_add_drain_all(); > > > > > > + retry = false; > > > > > > + goto retry; > > > > > > > > > > I'm not sure whether the retry logic is necessary or not, do you have > > > > > any data about how retry improves the success rate? You could just > > > > > replace lru_add_drain() to lru_add_drain_all() and remove the retry > > > > > logic IMHO. I'd prefer to keep it simple at the moment personally. > > > > > > > > Transparently, I've only had success hitting this logic on small vms > > > > under selftests. That said, it does happen, and I can't imagine this > > > > hurting, especially on larger systems + tasks using lots of mem. > > > > Originally, I didn't plan to do this, but as things shook out and we > > > > had SCAN_PAGE_LRU so readily available, it seemed like we got this for > > > > free. > > > > > > "small vms" mean small virtual machines? > > > > > > When the logic is hit, does lru_add_drain_all() help to improve the > > > success rate? > > > > Ya, I've been doing most dev/testing on small, 2 cpu virtual machines. > > I've been mmap()ing a multi-hugepage sized region, then faulting it in > > - presumably being preempted and rescheduled on another cpu while > > iterating over the region and faulting. I had ran into the > > lru_add_drain_all() vs lru_add_drain() during testing/dev since > > *occasionally* (admittedly, not very often the IIRC the former helped > > tests pass. > > > > That said, I set out to try and repro this semi-reliably, with little > > success - I'm almost always finding the pages on the LRU. Still > > playing around with this.. > > > > > I don't mean this hurts anything. I'm just thinking about whether the > > > extra complexity is worth it or not. And calling lru_add_drain_all() > > > with holding mmap_lock might have some scalability issues since > > > draining lru for all is not cheap. > > > > Good point. At the very least, it seems like we should unlock > > mmap_lock before retry. > > You could, but it still sounds overkilling to me. All the extra > complexity is just used to optimize for small sized machines which > unlikely run with THP in real life TBH. AFAIK, this *tries* to optimize for larger machines where lru_add_drain_all() is more costly. Likewise, tries to optimize for larger processes that might be scheduled on multiple cpus. This isn't likely to be in a particularly hot path - so I'm fine reducing complexity. If / when I can gather more data at scale, we can see if lru_add_drain_all() is too costly. Also - again - thanks for taking time to review and help out here :) > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > > + fallthrough; > > > > > > + default: > > > > > > + last_fail = result; > > > > > > + /* Other error, exit */ > > > > > > + goto out_maybelock; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > +out_maybelock: > > > > > > + /* Caller expects us to hold mmap_lock on return */ > > > > > > + if (!mmap_locked) > > > > > > + mmap_read_lock(mm); > > > > > > +out_nolock: > > > > > > + mmap_assert_locked(mm); > > > > > > + mmdrop(mm); > > > > > > + > > > > > > + return thps == ((hend - hstart) >> HPAGE_PMD_SHIFT) ? 0 > > > > > > + : madvise_collapse_errno(last_fail); > > > > > > +} > > > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > > > > > index 46feb62ce163..eccac2620226 100644 > > > > > > --- a/mm/madvise.c > > > > > > +++ b/mm/madvise.c > > > > > > @@ -59,6 +59,7 @@ static int madvise_need_mmap_write(int behavior) > > > > > > case MADV_FREE: > > > > > > case MADV_POPULATE_READ: > > > > > > case MADV_POPULATE_WRITE: > > > > > > + case MADV_COLLAPSE: > > > > > > return 0; > > > > > > default: > > > > > > /* be safe, default to 1. list exceptions explicitly */ > > > > > > @@ -1057,6 +1058,8 @@ static int madvise_vma_behavior(struct vm_area_struct *vma, > > > > > > if (error) > > > > > > goto out; > > > > > > break; > > > > > > + case MADV_COLLAPSE: > > > > > > + return madvise_collapse(vma, prev, start, end); > > > > > > } > > > > > > > > > > > > anon_name = anon_vma_name(vma); > > > > > > @@ -1150,6 +1153,7 @@ madvise_behavior_valid(int behavior) > > > > > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > > > > case MADV_HUGEPAGE: > > > > > > case MADV_NOHUGEPAGE: > > > > > > + case MADV_COLLAPSE: > > > > > > #endif > > > > > > case MADV_DONTDUMP: > > > > > > case MADV_DODUMP: > > > > > > @@ -1339,6 +1343,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start, > > > > > > * MADV_NOHUGEPAGE - mark the given range as not worth being backed by > > > > > > * transparent huge pages so the existing pages will not be > > > > > > * coalesced into THP and new pages will not be allocated as THP. > > > > > > + * MADV_COLLAPSE - synchronously coalesce pages into new THP. > > > > > > * MADV_DONTDUMP - the application wants to prevent pages in the given range > > > > > > * from being included in its core dump. > > > > > > * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump. > > > > > > -- > > > > > > 2.36.1.255.ge46751e96f-goog > > > > > >