* Re: [PATCH v7 20/42] KVM: SEV: Make 'uaddr' parameter optional for KVM_SEV_SNP_LAUNCH_UPDATE
From: Suzuki K Poulose @ 2026-06-04 15:29 UTC (permalink / raw)
To: ackerleytng, aik, andrew.jones, binbin.wu, brauner, chao.p.peng,
david, ira.weiny, jmattson, jthoughton, michael.roth, oupton,
pankaj.gupta, qperret, rick.p.edgecombe, rientjes, shivankg,
steven.price, tabba, willy, wyihan, yan.y.zhao, forkloop,
pratyush, aneesh.kumar, liam, Paolo Bonzini, Sean Christopherson,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Jonathan Corbet, Shuah Khan, Shuah Khan,
Vishal Annapurve, Andrew Morton, Chris Li, Kairui Song,
Kemeng Shi, Nhat Pham, Baoquan He, Barry Song, Axel Rasmussen,
Yuanchu Xie, Wei Xu, Youngjun Park, Qi Zheng, Shakeel Butt,
Kiryl Shutsemau, Jason Gunthorpe, Vlastimil Babka
Cc: kvm, linux-kernel, linux-trace-kernel, linux-doc, linux-kselftest,
linux-mm, linux-coco
In-Reply-To: <20260522-gmem-inplace-conversion-v7-20-2f0fae496530@google.com>
On 23/05/2026 01:18, Ackerley Tng via B4 Relay wrote:
> From: Michael Roth <michael.roth@amd.com>
>
> For vm_memory_attributes=1, in-place conversion/population is not
> supported, so the initial contents necessarily must need to come
> from a separate src address, which is enforced by the current
> implementation. However, for vm_memory_attributes=0, it is possible for
> guest memory to be initialized directly from userspace by mmap()'ing the
> guest_memfd and writing to it while the corresponding GPA ranges are in
> a 'shared' state before converting them to the 'private' state expected
> by KVM_SEV_SNP_LAUNCH_UPDATE.
>
> Update the handling/documentation for KVM_SEV_SNP_LAUNCH_UPDATE to allow
> for 'uaddr' to be set to NULL when vm_memory_attributes=0, which
> SNP_LAUNCH_UPDATE will then use to determine when it should/shouldn't
> copy in data from a separate memory location. Continue to enforce
> non-NULL for the original vm_memory_attributes=1 case.
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> [Added src_page check in error handling path when the firmware command fails]
> [Dropped ifdef CONFIG_KVM_VM_MEMORY_ATTRIBUTES]
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
> Documentation/virt/kvm/x86/amd-memory-encryption.rst | 15 +++++++++++----
> arch/x86/kvm/svm/sev.c | 18 +++++++++++++-----
> virt/kvm/kvm_main.c | 1 +
> 3 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> index b2395dd4769de..43085f65b2d85 100644
> --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
> @@ -503,7 +503,8 @@ secrets.
>
> It is required that the GPA ranges initialized by this command have had the
> KVM_MEMORY_ATTRIBUTE_PRIVATE attribute set in advance. See the documentation
> -for KVM_SET_MEMORY_ATTRIBUTES for more details on this aspect.
> +for KVM_SET_MEMORY_ATTRIBUTES/KVM_SET_MEMORY_ATTRIBUTES2 for more details on
> +this aspect.
>
> Upon success, this command is not guaranteed to have processed the entire
> range requested. Instead, the ``gfn_start``, ``uaddr``, and ``len`` fields of
> @@ -511,9 +512,15 @@ range requested. Instead, the ``gfn_start``, ``uaddr``, and ``len`` fields of
> remaining range that has yet to be processed. The caller should continue
> calling this command until those fields indicate the entire range has been
> processed, e.g. ``len`` is 0, ``gfn_start`` is equal to the last GFN in the
> -range plus 1, and ``uaddr`` is the last byte of the userspace-provided source
> -buffer address plus 1. In the case where ``type`` is KVM_SEV_SNP_PAGE_TYPE_ZERO,
> -``uaddr`` will be ignored completely.
> +range plus 1, and ``uaddr`` (if specified) is the last byte of the
> +userspace-provided source buffer address plus 1.
> +
> +In the case where ``type`` is KVM_SEV_SNP_PAGE_TYPE_ZERO, ``uaddr`` will be
> +ignored completely. Otherwise, ``uaddr`` is required if
> +kvm.vm_memory_attributes=1 and optional if kvm.vm_memory_attributes=0, since
> +in the latter case guest memory can be initialized directly from userspace
> +prior to converting it to private and passing the GPA range on to this
> +interface.
Just to confirm, so the sev_gmem_prepare doesn't destroy the contents in
the process of making it "private" ? i.e., the contents of a SNP shared
page are preserved while transitioning to "SNP Private" (via RMP
update).
Suzuki
>
> Parameters (in): struct kvm_sev_snp_launch_update
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 1a361f08c7a3d..e1dbc827c2807 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2343,7 +2343,15 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> int level;
> int ret;
>
> - if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page))
> + /*
> + * For vm_memory_attributes=1, in-place conversion/population is not
> + * supported, so the initial contents necessarily need to come from a
> + * separate src address. For vm_memory_attributes=0, this isn't
> + * necessarily the case, since the pages may have been populated
> + * directly from userspace before calling KVM_SEV_SNP_LAUNCH_UPDATE.
> + */
> + if (vm_memory_attributes &&
> + sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page)
> return -EINVAL;
>
> ret = snp_lookup_rmpentry((u64)pfn, &assigned, &level);
> @@ -2390,7 +2398,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> */
> if (ret && !snp_page_reclaim(kvm, pfn) &&
> sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
> - sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
> + sev_populate_args->fw_error == SEV_RET_INVALID_PARAM && src_page) {
> void *src_vaddr = kmap_local_page(src_page);
> void *dst_vaddr = kmap_local_pfn(pfn);
>
> @@ -2423,8 +2431,8 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
> if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
> return -EFAULT;
>
> - pr_debug("%s: GFN start 0x%llx length 0x%llx type %d flags %d\n", __func__,
> - params.gfn_start, params.len, params.type, params.flags);
> + pr_debug("%s: GFN start 0x%llx length 0x%llx type %d flags %d src %llx\n", __func__,
> + params.gfn_start, params.len, params.type, params.flags, params.uaddr);
>
> if (!params.len || !PAGE_ALIGNED(params.len) || params.flags ||
> (params.type != KVM_SEV_SNP_PAGE_TYPE_NORMAL &&
> @@ -2481,7 +2489,7 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
>
> params.gfn_start += count;
> params.len -= count * PAGE_SIZE;
> - if (params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
> + if (src && params.type != KVM_SEV_SNP_PAGE_TYPE_ZERO)
> params.uaddr += count * PAGE_SIZE;
>
> if (copy_to_user(u64_to_user_ptr(argp->data), ¶ms, sizeof(params)))
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ba195bb239aaa..3bf212fd99193 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -105,6 +105,7 @@ module_param(allow_unsafe_mappings, bool, 0444);
> #ifdef CONFIG_KVM_VM_MEMORY_ATTRIBUTES
> bool vm_memory_attributes = true;
> module_param(vm_memory_attributes, bool, 0444);
> +EXPORT_SYMBOL_FOR_KVM_INTERNAL(vm_memory_attributes);
> #endif
> DEFINE_STATIC_CALL_RET0(__kvm_get_memory_attributes, kvm_get_memory_attributes_t);
> EXPORT_SYMBOL_FOR_KVM_INTERNAL(STATIC_CALL_KEY(__kvm_get_memory_attributes));
>
^ permalink raw reply
* Re: [PATCH v2 08/10] riscv: cpufeature: Introduce ISA bases bitmap and rva23u64 detection
From: Andrew Jones @ 2026-06-04 15:29 UTC (permalink / raw)
To: Guodong Xu
Cc: Jonathan Corbet, Paul Walmsley, Palmer Dabbelt, Conor Dooley,
Albert Ou, Alexandre Ghiti, Shuah Khan, Anup Patel, Atish Patra,
Shuah Khan, Deepak Gupta, Zong Li, Christian Brauner,
Charlie Jenkins, Samuel Holland, linux-doc, linux-riscv,
linux-kernel, linux-kselftest, kvm, kvm-riscv, Guodong Xu
In-Reply-To: <CAH1PCMa-5W9PsX8cDLUk6-MkcM53HOz2QtaxCHd+XOr7DgH5+w@mail.gmail.com>
On Sat, May 30, 2026 at 08:42:29AM +0800, Guodong Xu wrote:
> On Thu, May 28, 2026 at 12:35 AM Andrew Jones
> > Sashiko points out a few things about this patch which I think I
> > agree with
> >
> > https://sashiko.dev/#/patchset/20260511-rva23u64-hwprobe-v2-v2-0-21c5a544f1dc%40riscstar.com?part=8
>
> Quote the following from Sashiko.dev:
> > Should this mask specify the individual subset extensions required by the
> > profile instead of the superset extensions like RISCV_ISA_EXT_B,
> > RISCV_ISA_EXT_C, and RISCV_ISA_EXT_V?
>
> My preference is to leave the mask on B/C/V (and A) as-is. I'd prefer to
> keep matching on the single-letter, rather than expanding them. Here is why:
>
> - The RVA23 profile lists A, B, C and V as single-letter mandatory
> extensions; it doesn't enumerate Zaamo/Zalrsc, Zba/Zbb/Zbs, Zc* or the
> Zve*/Zvl* subsets in the mandatory set.
>
> - In current merged code, hwprobe_isa_ext0() is already using
> riscv_isa_extension_available() signle letter checking for C and V.
>
> PS:
> B maybe a special one, just in case anybody raise it. As the community
> discussed when I adding it into the bindings, because B comes later than
> its sub-components zba/zbb/zbs, so, when I added B, I cleaned up all
> in-tree dts files which declared zba/zbb/zbs but not B and made them declare
> both.
>
> Link: https://lore.kernel.org/linux-riscv/20260115-adding-b-dtsi-v2-0-254dd61cf947@riscstar.com/
> [1]
>
> Also, in the bindings: extensions.yaml, a schema rule is added which requires
> a node listing zba, zbb and zbs to also list b (and the reverse). Moving on,
> new dtsi/dts fils, a node with only the subsets fails dtbs_check.
>
> One may argue that the schema check doesn't cover ACPI path. But again,
> shouldn't the vendor who publishs RVA23 hardware be conformant to the
> extensions wording in RVA23 v1.0 spec?
>
> What do you think?
>
I certainly see a case for the kernel staying out of the extension
dependency validation game. I think it makes sense for an ISA string
validation tool to exist for vendors to do sanity checks on their
ISA strings, but that's not the kernel's role. OTOH, whether or not
the kernel wants to try and detect inconsistencies with the ISA
string in order to build confidence in using what it sees there
and publishing what it sees there to usermode, through hwprobe, might
still be worth debating.
Without CPUID / ID_* registers for Linux to be able to check an
authoritative source of truth about what is and isn't supported by
the CPU, riscv Linux has to decide to either blindly trust the
hardware description or do sanity checks / probes in order to
confirm what it sees there. Maybe we can assume that any extension
used by the kernel will trip over itself quickly, alerting vendors
to fix their ISA strings, but I'm not sure we can make that assumption
for usermode extensions that Linux doesn't use, but does expose
through hwprobe. What apps need to run in testing to exercise them?
How long will those apps need to run before they trip over something?
The more I think I about it, the more I think the lack of CPUID / ID_*
registers puts Linux in a tight spot. If Linux trustingly publishes
what firmware tells it to to userspace and userspace blows up, Linux
will have to share some of the blame for having misled it. So, should
Linux validate everything it publishes somehow? Or, should it at least
do relatively cheap sanity checks on everything it publishes in order
to build some confidence?
Thanks,
drew
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 14:45 UTC (permalink / raw)
To: Nico Pache
Cc: linux-doc, linux-kernel, linux-mm, linux-trace-kernel, aarcange,
akpm, anshuman.khandual, apopple, baohua, baolin.wang, byungchul,
catalin.marinas, cl, corbet, dave.hansen, david, dev.jain, gourry,
hannes, hughd, jack, jackmanb, jannh, jglisse, joshua.hahnjy, kas,
lance.yang, liam, mathieu.desnoyers, matthew.brost, mhiramat,
mhocko, peterx, pfalcato, rakie.kim, raquini, rdunlap,
richard.weiyang, rientjes, rostedt, rppt, ryan.roberts, shivankg,
sunnanyong, surenb, thomas.hellstrom, tiwai, usamaarif642, vbabka,
vishal.moola, wangkefeng.wang, will, willy, yang, ying.huang, ziy,
zokeefe
In-Reply-To: <20260522150009.121603-12-npache@redhat.com>
On Fri, May 22, 2026 at 09:00:06AM -0600, Nico Pache wrote:
> Enable khugepaged to collapse to mTHP orders. This patch implements the
> main scanning logic using a bitmap to track occupied pages and a stack
> structure that allows us to find optimal collapse sizes.
>
> Previous to this patch, PMD collapse had 3 main phases, a light weight
> scanning phase (mmap_read_lock) that determines a potential PMD
> collapse, an alloc phase (mmap unlocked), then finally heavier collapse
> phase (mmap_write_lock).
>
> To enabled mTHP collapse we make the following changes:
>
> During PMD scan phase, track occupied pages in a bitmap. When mTHP
> orders are enabled, we remove the restriction of max_ptes_none during the
> scan phase to avoid missing potential mTHP collapse candidates. Once we
> have scanned the full PMD range and updated the bitmap to track occupied
> pages, we use the bitmap to find the optimal mTHP size.
>
> Implement collapse_scan_bitmap() to perform binary recursion on the bitmap
> and determine the best eligible order for the collapse. A stack structure
> is used instead of traditional recursion to manage the search. This also
> prevents a traditional recursive approach when the kernel stack struct is
> limited. The algorithm recursively splits the bitmap into smaller chunks to
> find the highest order mTHPs that satisfy the collapse criteria. We start
> by attempting the PMD order, then moved on the consecutively lower orders
> (mTHP collapse). The stack maintains a pair of variables (offset, order),
> indicating the number of PTEs from the start of the PMD, and the order of
> the potential collapse candidate.
>
> The algorithm for consuming the bitmap works as such:
> 1) push (0, HPAGE_PMD_ORDER) onto the stack
> 2) pop the stack
> 3) check if the number of set bits in that (offset,order) pair
> statisfy the max_ptes_none threshold for that order
> 4) if yes, attempt collapse
> 5) if no (or collapse fails), push two new stack items representing
> the left and right halves of the current bitmap range, at the
> next lower order
> 6) repeat at step (2) until stack is empty.
>
> Below is a diagram representing the algorithm and stack items:
>
> offset mid_offset
> | |
> | |
> v v
> ____________________________________
> | PTE Page Table |
> --------------------------------------
> <-------><------->
> order-1 order-1
>
> mTHP collapses reject regions containing swapped out or shared pages.
> This is because adding new entries can lead to new none pages, and these
> may lead to constant promotion into a higher order mTHP. A similar
> issue can occur with "max_ptes_none > HPAGE_PMD_NR/2" due to a collapse
> introducing at least 2x the number of pages, and on a future scan will
> satisfy the promotion condition once again. This issue is prevented via
> the collapse_max_ptes_none() function which imposes the max_ptes_none
> restrictions above.
>
> We currently only support mTHP collapse for max_ptes_none values of 0
> and HPAGE_PMD_NR - 1. resulting in the following behavior:
>
> - max_ptes_none=0: Never introduce new empty pages during collapse
> - max_ptes_none=HPAGE_PMD_NR-1: Always try collapse to the highest
> available mTHP order
>
> Any other max_ptes_none value will emit a warning and default mTHP
> collapse to max_ptes_none=0. There should be no behavior change for PMD
> collapse.
>
> Once we determine what mTHP sizes fits best in that PMD range a collapse
> is attempted. A minimum collapse order of 2 is used as this is the lowest
> order supported by anon memory as defined by THP_ORDERS_ALL_ANON.
>
> Currently madv_collapse is not supported and will only attempt PMD
> collapse.
>
> We can also remove the check for is_khugepaged inside the PMD scan as
> the collapse_max_ptes_none() function handles this logic now.
>
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
> mm/khugepaged.c | 181 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 172 insertions(+), 9 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 64ceebc9d8a7..d3d7db8be26c 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -99,6 +99,30 @@ static DEFINE_READ_MOSTLY_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS);
>
> static struct kmem_cache *mm_slot_cache __ro_after_init;
>
> +#define KHUGEPAGED_MIN_MTHP_ORDER 2
> +/*
> + * mthp_collapse() does an iterative DFS over a binary tree, from
> + * HPAGE_PMD_ORDER down to KHUGEPAGED_MIN_MTHP_ORDER. The max stack
> + * size needed for a DFS on a binary tree is height + 1, where
> + * height = HPAGE_PMD_ORDER - KHUGEPAGED_MIN_MTHP_ORDER.
> + *
> + * ilog2 is used in place of HPAGE_PMD_ORDER because some architectures
> + * (e.g. ppc64le) do not define HPAGE_PMD_ORDER until after build time.
> + */
> +#define MTHP_STACK_SIZE (ilog2(MAX_PTRS_PER_PTE) - KHUGEPAGED_MIN_MTHP_ORDER + 1)
> +
> +/*
> + * Defines a range of PTE entries in a PTE page table which are being
> + * considered for mTHP collapse.
> + *
> + * @offset: the offset of the first PTE entry in a PMD range.
> + * @order: the order of the PTE entries being considered for collapse.
> + */
> +struct mthp_range {
> + u16 offset;
> + u8 order;
> +};
> +
> struct collapse_control {
> bool is_khugepaged;
>
> @@ -110,6 +134,12 @@ struct collapse_control {
>
> /* nodemask for allocation fallback */
> nodemask_t alloc_nmask;
> +
> + /* Each bit represents a single occupied (!none/zero) page. */
> + DECLARE_BITMAP(mthp_bitmap, MAX_PTRS_PER_PTE);
> + /* A mask of the current range being considered for mTHP collapse. */
> + DECLARE_BITMAP(mthp_bitmap_mask, MAX_PTRS_PER_PTE);
> + struct mthp_range mthp_bitmap_stack[MTHP_STACK_SIZE];
> };
>
> /**
> @@ -1411,20 +1441,137 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s
> return result;
> }
>
> +static void collapse_mthp_stack_push(struct collapse_control *cc, int *stack_size,
> + u16 offset, u8 order)
> +{
> + const int size = *stack_size;
> + struct mthp_range *stack = &cc->mthp_bitmap_stack[size];
> +
> + VM_WARN_ON_ONCE(size >= MTHP_STACK_SIZE);
> + stack->order = order;
> + stack->offset = offset;
> + (*stack_size)++;
> +}
> +
> +static struct mthp_range collapse_mthp_stack_pop(struct collapse_control *cc,
> + int *stack_size)
> +{
> + const int size = *stack_size;
> +
> + VM_WARN_ON_ONCE(size <= 0);
> + (*stack_size)--;
> + return cc->mthp_bitmap_stack[size - 1];
> +}
> +
> +static unsigned int collapse_mthp_count_present(struct collapse_control *cc,
> + u16 offset, unsigned int nr_ptes)
> +{
> + bitmap_zero(cc->mthp_bitmap_mask, MAX_PTRS_PER_PTE);
> + bitmap_set(cc->mthp_bitmap_mask, offset, nr_ptes);
> + return bitmap_weight_and(cc->mthp_bitmap, cc->mthp_bitmap_mask, MAX_PTRS_PER_PTE);
> +}
> +
> +/*
> + * mthp_collapse() consumes the bitmap that is generated during
> + * collapse_scan_pmd() to determine what regions and mTHP orders fit best.
> + *
> + * Each bit in cc->mthp_bitmap represents a single occupied (!none/zero) page.
> + * A stack structure cc->mthp_bitmap_stack is used to check different regions
> + * of the bitmap for collapse eligibility. The stack maintains a pair of
> + * variables (offset, order), indicating the number of PTEs from the start of
> + * the PMD, and the order of the potential collapse candidate respectively. We
> + * start at the PMD order and check if it is eligible for collapse; if not, we
> + * add two entries to the stack at a lower order to represent the left and right
> + * halves of the PTE page table we are examining.
> + *
> + * offset mid_offset
> + * | |
> + * | |
> + * v v
> + * --------------------------------------
> + * | cc->mthp_bitmap |
> + * --------------------------------------
> + * <-------><------->
> + * order-1 order-1
> + *
> + * For each of these, we determine how many PTE entries are occupied in the
> + * range of PTE entries we propose to collapse, then we compare this to a
> + * threshold number of PTE entries which would need to be occupied for a
> + * collapse to be permitted at that order (accounting for max_ptes_none).
> + *
> + * If a collapse is permitted, we attempt to collapse the PTE range into a
> + * mTHP.
> + */
> +static int mthp_collapse(struct mm_struct *mm, struct vm_area_struct *vma,
> + unsigned long address, int referenced, int unmapped,
> + struct collapse_control *cc, unsigned long enabled_orders)
> +{
> + unsigned int nr_occupied_ptes, nr_ptes, max_ptes_none;
> + int collapsed = 0, stack_size = 0;
> + unsigned long collapse_address;
> + struct mthp_range range;
> + u16 offset;
> + u8 order;
> +
> + collapse_mthp_stack_push(cc, &stack_size, 0, HPAGE_PMD_ORDER);
> +
> + while (stack_size) {
> + range = collapse_mthp_stack_pop(cc, &stack_size);
> + order = range.order;
> + offset = range.offset;
> + nr_ptes = 1UL << order;
> +
> + if (!test_bit(order, &enabled_orders))
> + goto next_order;
> +
> + max_ptes_none = collapse_max_ptes_none(cc, vma, order);
> +
> + nr_occupied_ptes = collapse_mthp_count_present(cc, offset,
> + nr_ptes);
> +
> + if (nr_occupied_ptes >= nr_ptes - max_ptes_none) {
> + int ret;
> +
> + collapse_address = address + offset * PAGE_SIZE;
> + ret = collapse_huge_page(mm, collapse_address, referenced,
> + unmapped, cc, order);
> + if (ret == SCAN_SUCCEED) {
> + collapsed += nr_ptes;
> + continue;
> + }
> + }
> +
> +next_order:
> + if ((BIT(order) - 1) & enabled_orders) {
> + const u8 next_order = order - 1;
> + const u16 mid_offset = offset + (nr_ptes / 2);
> +
> + collapse_mthp_stack_push(cc, &stack_size, mid_offset,
> + next_order);
> + collapse_mthp_stack_push(cc, &stack_size, offset,
> + next_order);
> + }
> + }
> + return collapsed;
> +}
> +
> static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> struct vm_area_struct *vma, unsigned long start_addr,
> bool *lock_dropped, struct collapse_control *cc)
> {
> - const unsigned int max_ptes_none = collapse_max_ptes_none(cc, vma, HPAGE_PMD_ORDER);
> const unsigned int max_ptes_shared = collapse_max_ptes_shared(cc, HPAGE_PMD_ORDER);
> const unsigned int max_ptes_swap = collapse_max_ptes_swap(cc, HPAGE_PMD_ORDER);
> + unsigned int max_ptes_none = collapse_max_ptes_none(cc, vma, HPAGE_PMD_ORDER);
> + enum tva_type tva_flags = cc->is_khugepaged ? TVA_KHUGEPAGED : TVA_FORCED_COLLAPSE;
> pmd_t *pmd;
> - pte_t *pte, *_pte;
> - int none_or_zero = 0, shared = 0, referenced = 0;
> + pte_t *pte, *_pte, pteval;
> + int i;
> + int none_or_zero = 0, shared = 0, nr_collapsed = 0, referenced = 0;
> enum scan_result result = SCAN_FAIL;
> struct page *page = NULL;
> struct folio *folio = NULL;
> unsigned long addr;
> + unsigned long enabled_orders;
> spinlock_t *ptl;
> int node = NUMA_NO_NODE, unmapped = 0;
>
> @@ -1436,8 +1583,19 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> goto out;
> }
>
> + bitmap_zero(cc->mthp_bitmap, MAX_PTRS_PER_PTE);
> memset(cc->node_load, 0, sizeof(cc->node_load));
> nodes_clear(cc->alloc_nmask);
> +
> + enabled_orders = collapse_allowable_orders(vma, vma->vm_flags, tva_flags);
> +
> + /*
> + * If PMD is the only enabled order, enforce max_ptes_none, otherwise
> + * scan all pages to populate the bitmap for mTHP collapse.
> + */
> + if (enabled_orders != BIT(HPAGE_PMD_ORDER))
> + max_ptes_none = KHUGEPAGED_MAX_PTES_LIMIT;
Hmm, this is a bit odd, what if the user set max_ptes_none = 0?
I assume we handle the 0/511 thing elsewhere?
> +
> pte = pte_offset_map_lock(mm, pmd, start_addr, &ptl);
> if (!pte) {
> cc->progress++;
> @@ -1445,11 +1603,13 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> goto out;
> }
>
> - for (addr = start_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
> - _pte++, addr += PAGE_SIZE) {
> + for (i = 0; i < HPAGE_PMD_NR; i++) {
> + _pte = pte + i;
> + addr = start_addr + i * PAGE_SIZE;
> + pteval = ptep_get(_pte);
> +
> cc->progress++;
>
> - pte_t pteval = ptep_get(_pte);
> if (pte_none_or_zero(pteval)) {
> if (++none_or_zero > max_ptes_none) {
> result = SCAN_EXCEED_NONE_PTE;
> @@ -1529,6 +1689,8 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> }
> }
>
> + /* Set bit for occupied pages */
> + __set_bit(i, cc->mthp_bitmap);
> /*
> * Record which node the original page is from and save this
> * information to cc->node_load[].
> @@ -1587,10 +1749,11 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> if (result == SCAN_SUCCEED) {
> /* collapse_huge_page expects the lock to be dropped before calling */
> mmap_read_unlock(mm);
> - result = collapse_huge_page(mm, start_addr, referenced,
> - unmapped, cc, HPAGE_PMD_ORDER);
> - /* collapse_huge_page will return with the mmap_lock released */
> + nr_collapsed = mthp_collapse(mm, vma, start_addr, referenced,
> + unmapped, cc, enabled_orders);
I guess mthp_collapse() also does PMD collapse if only PMD is enabled?
It feels like this name is a bit confusing then :)
But I guess we can do a follow up to think of a better name possibly.
> + /* mmap_lock was released above, set lock_dropped */
> *lock_dropped = true;
> + result = nr_collapsed ? SCAN_SUCCEED : SCAN_FAIL;
> }
> out:
> trace_mm_khugepaged_scan_pmd(mm, folio, referenced,
> --
> 2.54.0
>
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 14:40 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Lance Yang, Nico Pache, linux-doc, linux-kernel, linux-mm,
linux-trace-kernel, aarcange, akpm, anshuman.khandual, apopple,
baohua, baolin.wang, byungchul, catalin.marinas, cl, corbet,
dave.hansen, dev.jain, gourry, hannes, hughd, jack, jackmanb,
jannh, jglisse, joshua.hahnjy, kas, liam, mathieu.desnoyers,
matthew.brost, mhiramat, mhocko, peterx, pfalcato, rakie.kim,
raquini, rdunlap, richard.weiyang, rientjes, rostedt, rppt,
ryan.roberts, shivankg, sunnanyong, surenb, thomas.hellstrom,
tiwai, usamaarif642, vbabka, vishal.moola, wangkefeng.wang, will,
willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <46bb9d9e-03f0-4e26-9ac9-1cdc5ba9bf4d@kernel.org>
On Wed, Jun 03, 2026 at 10:05:08AM +0200, David Hildenbrand (Arm) wrote:
> On 6/2/26 17:44, Lance Yang wrote:
> >
> >
> > On 2026/6/2 18:58, Nico Pache wrote:
> >> On Sun, May 31, 2026 at 1:19 AM Lance Yang <lance.yang@linux.dev> wrote:
> >>>
> >>>
> >>> [...]
> >>>
> >>> Hmm ... don't we lose the allocation-failure result here?
> >>>
> >>> Previously collapse_scan_pmd() propagated SCAN_ALLOC_HUGE_PAGE_FAIL from
> >>> collapse_huge_page(), so khugepaged would call khugepaged_alloc_sleep()
> >>> in khugepaged_do_scan().
> >>>
> >>> Now if allocation fails and nr_collapsed stays 0, we just return
> >>> SCAN_FAIL. So we won't back off via khugepaged_alloc_sleep() anymore?
> >>
> >> Ok I did the error propagation! I think I handled both of these cases
> >> you brought up pretty easily.
> >
> > Thanks.
> >
> >> However I don't know what to do in the following case: We successfully
> >> collapsed some portion of the PMD, but during that process, we also
> >> hit an allocation failure. Is it best to back off entirely? or can we
> >> treat some forward progress as a sign we can continue trying collapses
> >> without sleeping.
> >>
> >> Basically, do we prioritize SCAN_ALLOC_HUGE_PAGE_FAIL or the
> >> successful collapses as the returned value?
> >
> > Thinking out loud, forward progress should win here, the allocation
> > failure only matter if we made no progress at all?
>
> Agreed, in the first approach, forward progress makes sense.
Sounds sensible to me.
>
> --
> Cheers,
>
> David
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 14:19 UTC (permalink / raw)
To: Nico Pache
Cc: David Hildenbrand (Arm), linux-doc, linux-kernel, linux-mm,
linux-trace-kernel, aarcange, akpm, anshuman.khandual, apopple,
baohua, baolin.wang, byungchul, catalin.marinas, cl, corbet,
dave.hansen, dev.jain, gourry, hannes, hughd, jack, jackmanb,
jannh, jglisse, joshua.hahnjy, kas, lance.yang, liam,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, vbabka, vishal.moola, wangkefeng.wang,
will, willy, yang, ying.huang, ziy, zokeefe, Usama Arif,
usamaarif642
In-Reply-To: <aiGFIrg8_vZZxnPg@lucifer>
On Thu, Jun 04, 2026 at 03:14:59PM +0100, Lorenzo Stoakes wrote:
> On Tue, Jun 02, 2026 at 11:23:35AM -0600, Nico Pache wrote:
> >
> >
> > On 6/1/26 7:15 AM, David Hildenbrand (Arm) wrote:
> > >>>
> > >>> Reading this, it is unclear why exactly do we need the stack.
> > >>
> > >> So I looked into your items below. It seems logical, and I think it
> > >> works the same way; however, your method seems slightly harder to
> > >> understand due to all the edge cases and more error-prone to future
> > >> changes (the stack holds implicit knowledge of the offset/order that
> > >> must now be tracked in the edge cases).
> > >>
> > >> Given the stack is 24 bytes, I'm not sure if the extra complexity is
> > >> worth saving that small amount of memory. Although we would also be
> > >> getting rid of (3?) functions, so both approaches have pros and cons.
> > >
> > > I consider a simple forward loop over the offset ... less complexity compared to
> > > a stack structure :)
> > >
> > >>
> > >> I will implement a patch comparing your solution against mine and send
> > >> it here, then we can decide which approach is better.
> > >
> > > Right, throw it over the fence and I'll see how to improve it further.
> >
> > Ok heres what the diff looks like on top of my V19.
> >
> > you can access the tree here https://gitlab.com/npache/linux/-/commits/mthp-v19?ref_type=heads for easier review.
> >
> > So far I have no problem with this approach it appeared cleaner than i thought. Did some light testing. Gonna throw it more through the ringer tomorrow.
> >
> >
> > From 9496c5d17eba7f6d04820d78c7c6f1592a58888a Mon Sep 17 00:00:00 2001
> > From: Nico Pache <npache@redhat.com>
> > Date: Tue, 2 Jun 2026 10:26:18 -0600
> > Subject: [PATCH] convert from stack to forward loop
> >
> > Signed-off-by: Nico Pache <npache@redhat.com>
> > ---
> > mm/khugepaged.c | 96 ++++++++-----------------------------------------
> > 1 file changed, 15 insertions(+), 81 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 498eba009751..6de935e76ceb 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -100,28 +100,6 @@ static DEFINE_READ_MOSTLY_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS);
> > static struct kmem_cache *mm_slot_cache __ro_after_init;
> >
> > #define KHUGEPAGED_MIN_MTHP_ORDER 2
> > -/*
> > - * mthp_collapse() does an iterative DFS over a binary tree, from
> > - * HPAGE_PMD_ORDER down to KHUGEPAGED_MIN_MTHP_ORDER. The max stack
> > - * size needed for a DFS on a binary tree is height + 1, where
> > - * height = HPAGE_PMD_ORDER - KHUGEPAGED_MIN_MTHP_ORDER.
> > - *
> > - * ilog2 is used in place of HPAGE_PMD_ORDER because some architectures
> > - * (e.g. ppc64le) do not define HPAGE_PMD_ORDER until after build time.
> > - */
> > -#define MTHP_STACK_SIZE (ilog2(MAX_PTRS_PER_PTE) - KHUGEPAGED_MIN_MTHP_ORDER + 1)
> > -
> > -/*
> > - * Defines a range of PTE entries in a PTE page table which are being
> > - * considered for mTHP collapse.
> > - *
> > - * @offset: the offset of the first PTE entry in a PMD range.
> > - * @order: the order of the PTE entries being considered for collapse.
> > - */
> > -struct mthp_range {
> > - u16 offset;
> > - u8 order;
> > -};
> >
> > struct collapse_control {
> > bool is_khugepaged;
> > @@ -137,7 +115,6 @@ struct collapse_control {
> >
> > /* Each bit represents a single occupied (!none/zero) page. */
> > DECLARE_BITMAP(mthp_present_ptes, MAX_PTRS_PER_PTE);
> > - struct mthp_range mthp_bitmap_stack[MTHP_STACK_SIZE];
> > };
> >
> > /**
> > @@ -1458,50 +1435,14 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s
> > return result;
> > }
> >
> > -static void collapse_mthp_stack_push(struct collapse_control *cc, int *stack_size,
> > - u16 offset, u8 order)
> > -{
> > - const int size = *stack_size;
> > - struct mthp_range *stack = &cc->mthp_bitmap_stack[size];
> > -
> > - VM_WARN_ON_ONCE(size >= MTHP_STACK_SIZE);
> > - stack->order = order;
> > - stack->offset = offset;
> > - (*stack_size)++;
> > -}
> > -
> > -static struct mthp_range collapse_mthp_stack_pop(struct collapse_control *cc,
> > - int *stack_size)
> > -{
> > - const int size = *stack_size;
> > -
> > - VM_WARN_ON_ONCE(size <= 0);
> > - (*stack_size)--;
> > - return cc->mthp_bitmap_stack[size - 1];
> > -}
> > -
> > /*
> > * mthp_collapse() consumes the bitmap that is generated during
> > * collapse_scan_pmd() to determine what regions and mTHP orders fit best.
> > *
> > * Each bit in cc->mthp_present_ptes represents a single occupied (!none/zero)
> > - * page. A stack structure cc->mthp_bitmap_stack is used to check different
> > - * regions of the bitmap for collapse eligibility. The stack maintains a pair
> > - * of variables (offset, order), indicating the number of PTEs from the start
> > - * of the PMD, and the order of the potential collapse candidate respectively.
> > - * We start at the PMD order and check if it is eligible for collapse; if not,
> > - * we add two entries to the stack at a lower order to represent the left and
> > - * right halves of the PTE page table we are examining.
> > - *
> > - * offset mid_offset
> > - * | |
> > - * | |
> > - * v v
> > - * --------------------------------------
> > - * | cc->mthp_present_ptes |
> > - * --------------------------------------
> > - * <-------><------->
> > - * order-1 order-1
> > + * page. We start at the PMD order and check if it is eligible for collapse;
> > + * if not, we check the left and right halves of the PTE page table we are
> > + * examining at a lower order.
>
> Yeah this is not good enough sorry, before there was some kind of explanation of
> the algortihm, just because you can explain the _code_ more simply, that's not
> very useful.
>
> I had to sit down and spend quite a bit of time to figure out how the actual
> output looks so I think that should be explained.
>
> > *
> > * For each of these, we determine how many PTE entries are occupied in the
> > * range of PTE entries we propose to collapse, then we compare this to a
> > @@ -1517,26 +1458,20 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> > {
> > unsigned int nr_occupied_ptes, nr_ptes, max_ptes_none;
> > enum scan_result last_result = SCAN_FAIL;
> > - int collapsed = 0, stack_size = 0;
> > + int collapsed = 0;
> > bool alloc_failed = false;
> > unsigned long collapse_address;
> > - struct mthp_range range;
> > - u16 offset;
> > - u8 order;
> > + unsigned int offset = 0;
> > + unsigned int order = HPAGE_PMD_ORDER;
> >
> > - collapse_mthp_stack_push(cc, &stack_size, 0, HPAGE_PMD_ORDER);
> >
> > - while (stack_size) {
> > - range = collapse_mthp_stack_pop(cc, &stack_size);
> > - order = range.order;
> > - offset = range.offset;
> > + while (offset < HPAGE_PMD_NR) {
> > nr_ptes = 1UL << order;
> >
> > if (!test_bit(order, &enabled_orders))
> > goto next_order;
> >
> > max_ptes_none = collapse_max_ptes_none(cc, NULL, order);
> > -
> > nr_occupied_ptes = bitmap_weight_from(cc->mthp_present_ptes, offset,
> > offset + nr_ptes);
> >
> > @@ -1553,7 +1488,7 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> > collapsed += nr_ptes;
> > fallthrough;
> > case SCAN_PTE_MAPPED_HUGEPAGE:
> > - continue;
> > + goto next_offset;
> > /* Cases where lower orders might still succeed */
> > case SCAN_ALLOC_HUGE_PAGE_FAIL:
> > alloc_failed = true;
> > @@ -1581,15 +1516,14 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> > }
> >
>
> This obviously needs some comments describing what you're doing here. I think
> David said so too.
>
> > next_order:
> > - if ((BIT(order) - 1) & enabled_orders) {
> > - const u8 next_order = order - 1;
> > - const u16 mid_offset = offset + (nr_ptes / 2);
> > -
> > - collapse_mthp_stack_push(cc, &stack_size, mid_offset,
> > - next_order);
> > - collapse_mthp_stack_push(cc, &stack_size, offset,
> > - next_order);
> > + if (order > KHUGEPAGED_MIN_MTHP_ORDER &&
> > + (BIT(order) - 1) & enabled_orders) {
>
> Wait, if I disable an order this changes the way we get mTHP doesn't it?
>
> Let's say I disable order-4 but retain order-3 and order-2 for offset 0 we get:
>
> 9->8->7->6->5->5->6->5->5->7
>
> And we simply can't get order-3 no?
>
> This seems broken doesn't it? Maybe I'm missing something?
OK it's the way this is written, very confusing. I do not know why you are
writing this code in this 'compressed' way.
(1 << order) - 1) & enabled_orders is to see if there's _any others_ to check.
>
>
> > + order = order - 1;
>
> order--?
>
> > + continue;
> > }
> > +next_offset:
> > + offset += nr_ptes;
> > + order = min_t(int, __ffs(offset), HPAGE_PMD_ORDER);
>
> Also wouldn't, in the case where an enabled order check above skips an order--,
> we could have offset=0 here and end up just looping around checking from (0,
> HPAGE_PMD_ORDER) again? That also seems broken?
Yeah sorry the offset += nr_ptes fixes that anyway.
And the fact it's a mask check above makes this OK.
So the logic seems probably fine but it needs to be clearer.
>
> Also, what's __ffs(0)? Isn't it undefined? We shouldn't be relying on undefined
> behaviour no?
>
> https://elixir.bootlin.com/linux/v7.0.10/source/include/asm-generic/bitops/builtin-__ffs.h#L5
> Says as much?
>
> I guess we're assuming we're not going to get to 0 here, but that could do with
> a comment or a VM_WARN_ON_ONCE() at least.
>
> Also why aren't we making this a function?
>
> static inline unsigned int max_order_from_offset(unsigned int offset)
> {
> if (!offset)
> return HPAGE_PMD_ORDER;
>
> return __ffs(offset);
> }
>
> Though __ffs() works on unsigned long... probably... OK?
>
> > }
> > done:
> > if (collapsed)
> > --
> > 2.54.0
> >
> >
> >
> > >
> > > [...]
> > >
> > >>>> + bitmap_zero(cc->mthp_bitmap, MAX_PTRS_PER_PTE);
> > >>>> memset(cc->node_load, 0, sizeof(cc->node_load));
> > >>>> nodes_clear(cc->alloc_nmask);
> > >>>> +
> > >>>> + enabled_orders = collapse_allowable_orders(vma, vma->vm_flags, tva_flags);
> > >>>> +
> > >>>> + /*
> > >>>> + * If PMD is the only enabled order, enforce max_ptes_none, otherwise
> > >>>> + * scan all pages to populate the bitmap for mTHP collapse.
> > >>>> + */
> > >>>
> > >>> You should note here, that we re-verify in mthp_collapse().
> > >>>
> > >>> But the question is, whether we should relocate the check completely into
> > >>> mthp_collapse(), instead of conditionally duplicating it.
> > >>>
> > >>> What speaks against always populating the bitmap and making the decision in
> > >>> mthp_collapse()?
> > >>>
> > >>> Sure, we might scan a page table a bit longer, but the code gets clearer ... and
> > >>> I am not sure if scanning some more page table entries is really that critical here.
> > >>
> > >> Someone asked me to preserve the legacy behavior (PMD only). Although
> > >> rather trivial, if you set max_ptes_none=0 for example, we'd still
> > >> have to do 511 iterations for no reason if PMD collapse is the only
> > >> enabled order rather than bailing immediately.
> > >>
> > >> I'm ok with dropping it, but I think its the correct approach (despite
> > >> the extra complexity). @Usama Arif brought up this point here
> > >> https://lore.kernel.org/all/f8f7bb71-ca31-46ee-a62d-7ddfd83e0ead@gmail.com/
> > >
> > > We talk about regressions, but I am not sure if we care about scanning speed
> > > within a page table that much?
> > >
> > > After all, we locked it and already read some entries.
> > >
> > > Having the same check at two places to optimize for PMD order might right now
> > > feel like a good optimization, but likely an irrelevant one in a near future?
> > >
> > > Anyhow, won't push back, as long as we document why we are special casing things
> > > here.
> > >
> >
>
> Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 14:14 UTC (permalink / raw)
To: Nico Pache
Cc: David Hildenbrand (Arm), linux-doc, linux-kernel, linux-mm,
linux-trace-kernel, aarcange, akpm, anshuman.khandual, apopple,
baohua, baolin.wang, byungchul, catalin.marinas, cl, corbet,
dave.hansen, dev.jain, gourry, hannes, hughd, jack, jackmanb,
jannh, jglisse, joshua.hahnjy, kas, lance.yang, liam,
mathieu.desnoyers, matthew.brost, mhiramat, mhocko, peterx,
pfalcato, rakie.kim, raquini, rdunlap, richard.weiyang, rientjes,
rostedt, rppt, ryan.roberts, shivankg, sunnanyong, surenb,
thomas.hellstrom, tiwai, vbabka, vishal.moola, wangkefeng.wang,
will, willy, yang, ying.huang, ziy, zokeefe, Usama Arif,
usamaarif642
In-Reply-To: <19639b08-5bf1-4974-9635-c458d512fa38@redhat.com>
On Tue, Jun 02, 2026 at 11:23:35AM -0600, Nico Pache wrote:
>
>
> On 6/1/26 7:15 AM, David Hildenbrand (Arm) wrote:
> >>>
> >>> Reading this, it is unclear why exactly do we need the stack.
> >>
> >> So I looked into your items below. It seems logical, and I think it
> >> works the same way; however, your method seems slightly harder to
> >> understand due to all the edge cases and more error-prone to future
> >> changes (the stack holds implicit knowledge of the offset/order that
> >> must now be tracked in the edge cases).
> >>
> >> Given the stack is 24 bytes, I'm not sure if the extra complexity is
> >> worth saving that small amount of memory. Although we would also be
> >> getting rid of (3?) functions, so both approaches have pros and cons.
> >
> > I consider a simple forward loop over the offset ... less complexity compared to
> > a stack structure :)
> >
> >>
> >> I will implement a patch comparing your solution against mine and send
> >> it here, then we can decide which approach is better.
> >
> > Right, throw it over the fence and I'll see how to improve it further.
>
> Ok heres what the diff looks like on top of my V19.
>
> you can access the tree here https://gitlab.com/npache/linux/-/commits/mthp-v19?ref_type=heads for easier review.
>
> So far I have no problem with this approach it appeared cleaner than i thought. Did some light testing. Gonna throw it more through the ringer tomorrow.
>
>
> From 9496c5d17eba7f6d04820d78c7c6f1592a58888a Mon Sep 17 00:00:00 2001
> From: Nico Pache <npache@redhat.com>
> Date: Tue, 2 Jun 2026 10:26:18 -0600
> Subject: [PATCH] convert from stack to forward loop
>
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
> mm/khugepaged.c | 96 ++++++++-----------------------------------------
> 1 file changed, 15 insertions(+), 81 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 498eba009751..6de935e76ceb 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -100,28 +100,6 @@ static DEFINE_READ_MOSTLY_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS);
> static struct kmem_cache *mm_slot_cache __ro_after_init;
>
> #define KHUGEPAGED_MIN_MTHP_ORDER 2
> -/*
> - * mthp_collapse() does an iterative DFS over a binary tree, from
> - * HPAGE_PMD_ORDER down to KHUGEPAGED_MIN_MTHP_ORDER. The max stack
> - * size needed for a DFS on a binary tree is height + 1, where
> - * height = HPAGE_PMD_ORDER - KHUGEPAGED_MIN_MTHP_ORDER.
> - *
> - * ilog2 is used in place of HPAGE_PMD_ORDER because some architectures
> - * (e.g. ppc64le) do not define HPAGE_PMD_ORDER until after build time.
> - */
> -#define MTHP_STACK_SIZE (ilog2(MAX_PTRS_PER_PTE) - KHUGEPAGED_MIN_MTHP_ORDER + 1)
> -
> -/*
> - * Defines a range of PTE entries in a PTE page table which are being
> - * considered for mTHP collapse.
> - *
> - * @offset: the offset of the first PTE entry in a PMD range.
> - * @order: the order of the PTE entries being considered for collapse.
> - */
> -struct mthp_range {
> - u16 offset;
> - u8 order;
> -};
>
> struct collapse_control {
> bool is_khugepaged;
> @@ -137,7 +115,6 @@ struct collapse_control {
>
> /* Each bit represents a single occupied (!none/zero) page. */
> DECLARE_BITMAP(mthp_present_ptes, MAX_PTRS_PER_PTE);
> - struct mthp_range mthp_bitmap_stack[MTHP_STACK_SIZE];
> };
>
> /**
> @@ -1458,50 +1435,14 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s
> return result;
> }
>
> -static void collapse_mthp_stack_push(struct collapse_control *cc, int *stack_size,
> - u16 offset, u8 order)
> -{
> - const int size = *stack_size;
> - struct mthp_range *stack = &cc->mthp_bitmap_stack[size];
> -
> - VM_WARN_ON_ONCE(size >= MTHP_STACK_SIZE);
> - stack->order = order;
> - stack->offset = offset;
> - (*stack_size)++;
> -}
> -
> -static struct mthp_range collapse_mthp_stack_pop(struct collapse_control *cc,
> - int *stack_size)
> -{
> - const int size = *stack_size;
> -
> - VM_WARN_ON_ONCE(size <= 0);
> - (*stack_size)--;
> - return cc->mthp_bitmap_stack[size - 1];
> -}
> -
> /*
> * mthp_collapse() consumes the bitmap that is generated during
> * collapse_scan_pmd() to determine what regions and mTHP orders fit best.
> *
> * Each bit in cc->mthp_present_ptes represents a single occupied (!none/zero)
> - * page. A stack structure cc->mthp_bitmap_stack is used to check different
> - * regions of the bitmap for collapse eligibility. The stack maintains a pair
> - * of variables (offset, order), indicating the number of PTEs from the start
> - * of the PMD, and the order of the potential collapse candidate respectively.
> - * We start at the PMD order and check if it is eligible for collapse; if not,
> - * we add two entries to the stack at a lower order to represent the left and
> - * right halves of the PTE page table we are examining.
> - *
> - * offset mid_offset
> - * | |
> - * | |
> - * v v
> - * --------------------------------------
> - * | cc->mthp_present_ptes |
> - * --------------------------------------
> - * <-------><------->
> - * order-1 order-1
> + * page. We start at the PMD order and check if it is eligible for collapse;
> + * if not, we check the left and right halves of the PTE page table we are
> + * examining at a lower order.
Yeah this is not good enough sorry, before there was some kind of explanation of
the algortihm, just because you can explain the _code_ more simply, that's not
very useful.
I had to sit down and spend quite a bit of time to figure out how the actual
output looks so I think that should be explained.
> *
> * For each of these, we determine how many PTE entries are occupied in the
> * range of PTE entries we propose to collapse, then we compare this to a
> @@ -1517,26 +1458,20 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> {
> unsigned int nr_occupied_ptes, nr_ptes, max_ptes_none;
> enum scan_result last_result = SCAN_FAIL;
> - int collapsed = 0, stack_size = 0;
> + int collapsed = 0;
> bool alloc_failed = false;
> unsigned long collapse_address;
> - struct mthp_range range;
> - u16 offset;
> - u8 order;
> + unsigned int offset = 0;
> + unsigned int order = HPAGE_PMD_ORDER;
>
> - collapse_mthp_stack_push(cc, &stack_size, 0, HPAGE_PMD_ORDER);
>
> - while (stack_size) {
> - range = collapse_mthp_stack_pop(cc, &stack_size);
> - order = range.order;
> - offset = range.offset;
> + while (offset < HPAGE_PMD_NR) {
> nr_ptes = 1UL << order;
>
> if (!test_bit(order, &enabled_orders))
> goto next_order;
>
> max_ptes_none = collapse_max_ptes_none(cc, NULL, order);
> -
> nr_occupied_ptes = bitmap_weight_from(cc->mthp_present_ptes, offset,
> offset + nr_ptes);
>
> @@ -1553,7 +1488,7 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> collapsed += nr_ptes;
> fallthrough;
> case SCAN_PTE_MAPPED_HUGEPAGE:
> - continue;
> + goto next_offset;
> /* Cases where lower orders might still succeed */
> case SCAN_ALLOC_HUGE_PAGE_FAIL:
> alloc_failed = true;
> @@ -1581,15 +1516,14 @@ static enum scan_result mthp_collapse(struct mm_struct *mm,
> }
>
This obviously needs some comments describing what you're doing here. I think
David said so too.
> next_order:
> - if ((BIT(order) - 1) & enabled_orders) {
> - const u8 next_order = order - 1;
> - const u16 mid_offset = offset + (nr_ptes / 2);
> -
> - collapse_mthp_stack_push(cc, &stack_size, mid_offset,
> - next_order);
> - collapse_mthp_stack_push(cc, &stack_size, offset,
> - next_order);
> + if (order > KHUGEPAGED_MIN_MTHP_ORDER &&
> + (BIT(order) - 1) & enabled_orders) {
Wait, if I disable an order this changes the way we get mTHP doesn't it?
Let's say I disable order-4 but retain order-3 and order-2 for offset 0 we get:
9->8->7->6->5->5->6->5->5->7
And we simply can't get order-3 no?
This seems broken doesn't it? Maybe I'm missing something?
> + order = order - 1;
order--?
> + continue;
> }
> +next_offset:
> + offset += nr_ptes;
> + order = min_t(int, __ffs(offset), HPAGE_PMD_ORDER);
Also wouldn't, in the case where an enabled order check above skips an order--,
we could have offset=0 here and end up just looping around checking from (0,
HPAGE_PMD_ORDER) again? That also seems broken?
Also, what's __ffs(0)? Isn't it undefined? We shouldn't be relying on undefined
behaviour no?
https://elixir.bootlin.com/linux/v7.0.10/source/include/asm-generic/bitops/builtin-__ffs.h#L5
Says as much?
I guess we're assuming we're not going to get to 0 here, but that could do with
a comment or a VM_WARN_ON_ONCE() at least.
Also why aren't we making this a function?
static inline unsigned int max_order_from_offset(unsigned int offset)
{
if (!offset)
return HPAGE_PMD_ORDER;
return __ffs(offset);
}
Though __ffs() works on unsigned long... probably... OK?
> }
> done:
> if (collapsed)
> --
> 2.54.0
>
>
>
> >
> > [...]
> >
> >>>> + bitmap_zero(cc->mthp_bitmap, MAX_PTRS_PER_PTE);
> >>>> memset(cc->node_load, 0, sizeof(cc->node_load));
> >>>> nodes_clear(cc->alloc_nmask);
> >>>> +
> >>>> + enabled_orders = collapse_allowable_orders(vma, vma->vm_flags, tva_flags);
> >>>> +
> >>>> + /*
> >>>> + * If PMD is the only enabled order, enforce max_ptes_none, otherwise
> >>>> + * scan all pages to populate the bitmap for mTHP collapse.
> >>>> + */
> >>>
> >>> You should note here, that we re-verify in mthp_collapse().
> >>>
> >>> But the question is, whether we should relocate the check completely into
> >>> mthp_collapse(), instead of conditionally duplicating it.
> >>>
> >>> What speaks against always populating the bitmap and making the decision in
> >>> mthp_collapse()?
> >>>
> >>> Sure, we might scan a page table a bit longer, but the code gets clearer ... and
> >>> I am not sure if scanning some more page table entries is really that critical here.
> >>
> >> Someone asked me to preserve the legacy behavior (PMD only). Although
> >> rather trivial, if you set max_ptes_none=0 for example, we'd still
> >> have to do 511 iterations for no reason if PMD collapse is the only
> >> enabled order rather than bailing immediately.
> >>
> >> I'm ok with dropping it, but I think its the correct approach (despite
> >> the extra complexity). @Usama Arif brought up this point here
> >> https://lore.kernel.org/all/f8f7bb71-ca31-46ee-a62d-7ddfd83e0ead@gmail.com/
> >
> > We talk about regressions, but I am not sure if we care about scanning speed
> > within a page table that much?
> >
> > After all, we locked it and already read some entries.
> >
> > Having the same check at two places to optimize for PMD order might right now
> > feel like a good optimization, but likely an irrelevant one in a near future?
> >
> > Anyhow, won't push back, as long as we document why we are special casing things
> > here.
> >
>
Thanks, Lorenzo
^ permalink raw reply
* [PATCH] docs: arm64: Document that text_offset is always 0
From: Rasmus Villemoes @ 2026-06-04 14:08 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Ard Biesheuvel, Will Deacon, Jonathan Corbet, linux-doc,
linux-kernel, Rasmus Villemoes
When trying to figure out where to place and call an arm64 Image in
memory, reading booting.rst should provide the answer. However, it
requires quite some digging to figure out that text_offset is set via
".quad 0" in head.S and is thus actually always 0 since v5.10.
Update the documentation and make that explicit. Reword the 2MB
requirement accordingly, and remove the paragraphs that only apply to
the ancient versions where text_offset could be non-zero, as they only
confuse a current reader.
Fixes: 120dc60d0bdb ("arm64: get rid of TEXT_OFFSET")
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
I've included a Fixes tag since I spent way too much time tracking
down where that text_offset might be defined. The mentioned commit did
get rid of all references to TEXT_OFFSET-the-macro, but not
text_offset-the-concept.
Documentation/arch/arm64/booting.rst | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
index 13ef311dace8..f4cc25b1fd56 100644
--- a/Documentation/arch/arm64/booting.rst
+++ b/Documentation/arch/arm64/booting.rst
@@ -55,9 +55,6 @@ not exceed 2 megabytes in size. Since the dtb will be mapped cacheable
using blocks of up to 2 megabytes in size, it must not be placed within
any 2M region which must be mapped with any specific attributes.
-NOTE: versions prior to v4.2 also require that the DTB be placed within
-the 512 MB region starting at text_offset bytes below the kernel Image.
-
3. Decompress the kernel image
------------------------------
@@ -93,6 +90,8 @@ Header notes:
- As of v3.17, all fields are little endian unless stated otherwise.
+- As of v5.10, text_offset is always 0.
+
- code0/code1 are responsible for branching to stext.
- when booting through EFI, code0/code1 are initially skipped.
@@ -100,12 +99,6 @@ Header notes:
entry point (efi_stub_entry). When the stub has done its work, it
jumps to code0 to resume the normal boot process.
-- Prior to v3.17, the endianness of text_offset was not specified. In
- these cases image_size is zero and text_offset is 0x80000 in the
- endianness of the kernel. Where image_size is non-zero image_size is
- little-endian and must be respected. Where image_size is zero,
- text_offset can be assumed to be 0x80000.
-
- The flags field (introduced in v3.17) is a little-endian 64-bit field
composed as follows:
@@ -135,12 +128,9 @@ Header notes:
end of the kernel image. The amount of space required will vary
depending on selected features, and is effectively unbound.
-The Image must be placed text_offset bytes from a 2MB aligned base
-address anywhere in usable system RAM and called there. The region
-between the 2 MB aligned base address and the start of the image has no
-special significance to the kernel, and may be used for other purposes.
-At least image_size bytes from the start of the image must be free for
-use by the kernel.
+The Image must be placed at a 2MB aligned base address anywhere in
+usable system RAM and called there. At least image_size bytes from
+the start of the image must be free for use by the kernel.
NOTE: versions prior to v4.6 cannot make use of memory below the
physical offset of the Image so it is recommended that the Image be
placed as close as possible to the start of system RAM.
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v3 2/4] mm/zswap: Implement proactive writeback
From: Shakeel Butt @ 2026-06-04 14:01 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Hao Jia, Johannes Weiner, mhocko, tj, mkoutny, roman.gushchin,
Nhat Pham, akpm, chengming.zhou, muchun.song, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <CAO9r8zM4SDdTgz9L2s1VfXL8K2VBjMD9ej2BTDxaGge1t2+quA@mail.gmail.com>
On Wed, Jun 03, 2026 at 10:36:07PM -0700, Yosry Ahmed wrote:
> > >> But doesn't it make more sense to specify the compressed size, which is
> > >> ultimately the amount of memory you actually want to reclaim.
> > >>
> > >
> > > I personally prefer compressed size to pre-compressed size. That's
> > > kinda what user cares about, no?
> > >
> > > One thing we can do is let users prescribe a compressed size, but
> > > internally, we can multiply that by the average compression ratio.
> > > That gives us a guesstimate of how many pages we need to reclaim, and
> > > you can follow the rest of your implementation as is (perhaps with
> > > short-circuit when we reach the goal with fewer pages reclaimed).
> >
> > Got it. I will change it to use the compressed size in the next version.
> >
> > Yosry, Nhat, should we continue using the zswap_writeback_only key to
> > trigger proactive writeback?
>
> I *really* want the memcg maintainers to chime in here, it's
> ultimately their call.
>
> Michal? Johannes? Shakeel? Roman? Anyone? :D
Sorry for the delay, I will take a look in a day or two.
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 13:59 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel,
aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, liam, mathieu.desnoyers,
matthew.brost, mhiramat, mhocko, peterx, pfalcato, rakie.kim,
raquini, rdunlap, richard.weiyang, rientjes, rostedt, rppt,
ryan.roberts, shivankg, sunnanyong, surenb, thomas.hellstrom,
tiwai, usamaarif642, vbabka, vishal.moola, wangkefeng.wang, will,
willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <aiF25dvH4qd_C4aj@lucifer>
On Thu, Jun 04, 2026 at 02:53:39PM +0100, Lorenzo Stoakes wrote:
> (Checking the algorithm here)
>
> On Mon, Jun 01, 2026 at 10:11:24AM +0200, David Hildenbrand (Arm) wrote:
> > On 5/22/26 17:00, Nico Pache wrote:
> >
> > Finally time for the core piece :)
> >
> > > Enable khugepaged to collapse to mTHP orders. This patch implements the
> > > main scanning logic using a bitmap to track occupied pages and a stack
> > > structure that allows us to find optimal collapse sizes.
> > >
> > > Previous to this patch, PMD collapse had 3 main phases, a light weight
> > > scanning phase (mmap_read_lock) that determines a potential PMD
> > > collapse, an alloc phase (mmap unlocked), then finally heavier collapse
> > > phase (mmap_write_lock).
> > >
> > > To enabled mTHP collapse we make the following changes:
> > >
> > > During PMD scan phase, track occupied pages in a bitmap. When mTHP
> > > orders are enabled, we remove the restriction of max_ptes_none during the
> > > scan phase to avoid missing potential mTHP collapse candidates. Once we
> > > have scanned the full PMD range and updated the bitmap to track occupied
> > > pages, we use the bitmap to find the optimal mTHP size.
> > >
> > > Implement collapse_scan_bitmap() to perform binary recursion on the bitmap
> > > and determine the best eligible order for the collapse. A stack structure
> > > is used instead of traditional recursion to manage the search. This also
> > > prevents a traditional recursive approach when the kernel stack struct is
> > > limited. The algorithm recursively splits the bitmap into smaller chunks to
> > > find the highest order mTHPs that satisfy the collapse criteria. We start
> > > by attempting the PMD order, then moved on the consecutively lower orders
> > > (mTHP collapse). The stack maintains a pair of variables (offset, order),
>
> This is inaccurate, it's only consecutively smaller until you hit smallest then
> it starts bumping around 2 -> 3 -> 2 -> 3 -> 2 -> .. -> 4 -> 3 -> 2 -> 3 -> 2 -> 4 -> etc.
>
> More like consecutively smaller, then always trying for the smallest possible
> fit?
>
> Would be good to describe why we do this, presumably to get a best _fit_?
>
> > > indicating the number of PTEs from the start of the PMD, and the order of
> > > the potential collapse candidate.
> > >
> > > The algorithm for consuming the bitmap works as such:
> > > 1) push (0, HPAGE_PMD_ORDER) onto the stack
> > > 2) pop the stack
> > > 3) check if the number of set bits in that (offset,order) pair
> > > statisfy the max_ptes_none threshold for that order
> > > 4) if yes, attempt collapse
> > > 5) if no (or collapse fails), push two new stack items representing
> > > the left and right halves of the current bitmap range, at the
> > > next lower order
>
> I notice the ordering is wrong here, you actualy push the mid_offset first then
> the offset (e.g. 'right', then 'left'):
>
> collapse_mthp_stack_push(cc, &stack_size, mid_offset,
> next_order);
> collapse_mthp_stack_push(cc, &stack_size, offset,
> next_order);
>
> So that way you are popping the 'left' first then the 'right'.
>
> So seems you'll get:
>
> stack={0, 9}
>
> Pop (0, order=9):
>
> |----------------------------------------|
> |########################################|
> |----------------------------------------|
>
> stack={256, 8}, {0, 8}
>
> Pop (0, order=8):
>
> |--------------------|-------------------|
> |####################| |
> |--------------------|-------------------|
>
>
> stack={256, 8}, {128, 7}, {0, 7}
>
> Pop (0, order=7):
>
> |----------|-----------------------------|
> |##########| |
> |----------|-----------------------------|
>
> stack={256, 8}, {128, 7}, {64, 6}, {0, 6}
>
> Pop (0, order=6):
>
> |----|-----------------------------------|
> |####| |
> |----|-----------------------------------|
>
> ...
>
> stack={256, 8}, ..., { 8, 3 }, {0, 2}
>
> Pop (0, order=2):
>
> |-|--------------------------------------|
> |#| |
> |-|--------------------------------------|
>
> Then finally :) we get the offsets :)
>
> stack={256, 8}, ..., {8, 3}, {4, 2}
>
> Pop (4, order=2):
>
> |-|-|------------------------------------|
> | |#| |
> |-|-|------------------------------------|
>
> stack={256, 8}, ..., { 12, 2 }, {8, 3}
(Shouldn't be {12, 2} there :)
> Pop (8, order=3):
>
> |---|--|---------------------------------|
> | |##| |
> |---|--|---------------------------------|
>
> stack={256, 8}, ..., { 12, 2 }, {12, 2}, {8, 2}
(Shouldn't duplicate {12, 2} there :)
>
> Pop (8, order=2):
>
> |---|-|----------------------------------|
> | |#| |
> |---|-|----------------------------------|
>
> etc.
>
>
> It seems to me that you're going to keep iterating down until you match an mTHP
> when a larger mTHP could have been had?
>
> So we're going:
>
> order 9 -> 8 -> 7 -> 6 -> ... -> 2 -> 3 -> 2 -> 4 -> 3 -> 2
>
> I guess the point is to avoid only getting the largest possible
>
>
>
>
> I guess if we did try to get the largest then we'd only get 2 of the largest
> possible then exhaust the whole PMD, should a PMD-sized entry not be possble.
>
> > > 6) repeat at step (2) until stack is empty.
> > >
> > > Below is a diagram representing the algorithm and stack items:
> > >
> > > offset mid_offset
> > > | |
> > > | |
> > > v v
> > > ____________________________________
> > > | PTE Page Table |
> > > --------------------------------------
> > > <-------><------->
> > > order-1 order-1
> >
> >
> > Reading this, it is unclear why exactly do we need the stack.
> >
> > Why can't you work with offset + cur_order?
> >
> > Initially,
> >
> > offset = 0;
> > cur_order = HPAGE_PMD_ORDER;
> >
> > If collapse succeeded, advance to next range.
> > If collapse failed, try next smaller order, keeping offset unchanged.
> >
> > if (failed && cur_order > KHUGEPAGED_MIN_MTHP_ORDER) {
> > /* Try next smaller order. */
> > cur_order = cur_order - 1;
>
> OK this matches the stack for the 0 offset entries...
>
> > } else {
> > /* Skip to next chunk. */
> > offset += 1 << cur_order;
> > cur_order = max_order_from_offset(offset);
>
> Then 1 << 2 -> 4 so go to offset=4.
>
> max_order_from_offset(4) = 2. so (4, offset=2) same as above.
>
> Then we'd loop back here and go to offset = 8, and max_order_from_offset(8) = 3
>
> And, yeah this seems equivalent.
>
> > }
>
> >
> > Of course, handling disabled orders. max_order_from_offset() is rather trivial
> > (natural buddy order, capped at HPAGE_PMD_ORDER).
>
> Something like?
>
> static unsigned long max_order_from_offset(unsigned long offset)
> {
> if (!offset)
> return HPAGE_PMD_ORDER;
>
> return ilog2(offset);
Oops, we need the LSB so ffs.
> }
>
> >
> > What's the benefit of the stack?
>
> Yeah it seems equivalent. Good idea!
>
> Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support
From: Lorenzo Stoakes @ 2026-06-04 13:53 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Nico Pache, linux-doc, linux-kernel, linux-mm, linux-trace-kernel,
aarcange, akpm, anshuman.khandual, apopple, baohua, baolin.wang,
byungchul, catalin.marinas, cl, corbet, dave.hansen, dev.jain,
gourry, hannes, hughd, jack, jackmanb, jannh, jglisse,
joshua.hahnjy, kas, lance.yang, liam, mathieu.desnoyers,
matthew.brost, mhiramat, mhocko, peterx, pfalcato, rakie.kim,
raquini, rdunlap, richard.weiyang, rientjes, rostedt, rppt,
ryan.roberts, shivankg, sunnanyong, surenb, thomas.hellstrom,
tiwai, usamaarif642, vbabka, vishal.moola, wangkefeng.wang, will,
willy, yang, ying.huang, ziy, zokeefe
In-Reply-To: <b8380eb3-096a-49f1-9ace-99c1e75888b4@kernel.org>
(Checking the algorithm here)
On Mon, Jun 01, 2026 at 10:11:24AM +0200, David Hildenbrand (Arm) wrote:
> On 5/22/26 17:00, Nico Pache wrote:
>
> Finally time for the core piece :)
>
> > Enable khugepaged to collapse to mTHP orders. This patch implements the
> > main scanning logic using a bitmap to track occupied pages and a stack
> > structure that allows us to find optimal collapse sizes.
> >
> > Previous to this patch, PMD collapse had 3 main phases, a light weight
> > scanning phase (mmap_read_lock) that determines a potential PMD
> > collapse, an alloc phase (mmap unlocked), then finally heavier collapse
> > phase (mmap_write_lock).
> >
> > To enabled mTHP collapse we make the following changes:
> >
> > During PMD scan phase, track occupied pages in a bitmap. When mTHP
> > orders are enabled, we remove the restriction of max_ptes_none during the
> > scan phase to avoid missing potential mTHP collapse candidates. Once we
> > have scanned the full PMD range and updated the bitmap to track occupied
> > pages, we use the bitmap to find the optimal mTHP size.
> >
> > Implement collapse_scan_bitmap() to perform binary recursion on the bitmap
> > and determine the best eligible order for the collapse. A stack structure
> > is used instead of traditional recursion to manage the search. This also
> > prevents a traditional recursive approach when the kernel stack struct is
> > limited. The algorithm recursively splits the bitmap into smaller chunks to
> > find the highest order mTHPs that satisfy the collapse criteria. We start
> > by attempting the PMD order, then moved on the consecutively lower orders
> > (mTHP collapse). The stack maintains a pair of variables (offset, order),
This is inaccurate, it's only consecutively smaller until you hit smallest then
it starts bumping around 2 -> 3 -> 2 -> 3 -> 2 -> .. -> 4 -> 3 -> 2 -> 3 -> 2 -> 4 -> etc.
More like consecutively smaller, then always trying for the smallest possible
fit?
Would be good to describe why we do this, presumably to get a best _fit_?
> > indicating the number of PTEs from the start of the PMD, and the order of
> > the potential collapse candidate.
> >
> > The algorithm for consuming the bitmap works as such:
> > 1) push (0, HPAGE_PMD_ORDER) onto the stack
> > 2) pop the stack
> > 3) check if the number of set bits in that (offset,order) pair
> > statisfy the max_ptes_none threshold for that order
> > 4) if yes, attempt collapse
> > 5) if no (or collapse fails), push two new stack items representing
> > the left and right halves of the current bitmap range, at the
> > next lower order
I notice the ordering is wrong here, you actualy push the mid_offset first then
the offset (e.g. 'right', then 'left'):
collapse_mthp_stack_push(cc, &stack_size, mid_offset,
next_order);
collapse_mthp_stack_push(cc, &stack_size, offset,
next_order);
So that way you are popping the 'left' first then the 'right'.
So seems you'll get:
stack={0, 9}
Pop (0, order=9):
|----------------------------------------|
|########################################|
|----------------------------------------|
stack={256, 8}, {0, 8}
Pop (0, order=8):
|--------------------|-------------------|
|####################| |
|--------------------|-------------------|
stack={256, 8}, {128, 7}, {0, 7}
Pop (0, order=7):
|----------|-----------------------------|
|##########| |
|----------|-----------------------------|
stack={256, 8}, {128, 7}, {64, 6}, {0, 6}
Pop (0, order=6):
|----|-----------------------------------|
|####| |
|----|-----------------------------------|
...
stack={256, 8}, ..., { 8, 3 }, {0, 2}
Pop (0, order=2):
|-|--------------------------------------|
|#| |
|-|--------------------------------------|
Then finally :) we get the offsets :)
stack={256, 8}, ..., {8, 3}, {4, 2}
Pop (4, order=2):
|-|-|------------------------------------|
| |#| |
|-|-|------------------------------------|
stack={256, 8}, ..., { 12, 2 }, {8, 3}
Pop (8, order=3):
|---|--|---------------------------------|
| |##| |
|---|--|---------------------------------|
stack={256, 8}, ..., { 12, 2 }, {12, 2}, {8, 2}
Pop (8, order=2):
|---|-|----------------------------------|
| |#| |
|---|-|----------------------------------|
etc.
It seems to me that you're going to keep iterating down until you match an mTHP
when a larger mTHP could have been had?
So we're going:
order 9 -> 8 -> 7 -> 6 -> ... -> 2 -> 3 -> 2 -> 4 -> 3 -> 2
I guess the point is to avoid only getting the largest possible
I guess if we did try to get the largest then we'd only get 2 of the largest
possible then exhaust the whole PMD, should a PMD-sized entry not be possble.
> > 6) repeat at step (2) until stack is empty.
> >
> > Below is a diagram representing the algorithm and stack items:
> >
> > offset mid_offset
> > | |
> > | |
> > v v
> > ____________________________________
> > | PTE Page Table |
> > --------------------------------------
> > <-------><------->
> > order-1 order-1
>
>
> Reading this, it is unclear why exactly do we need the stack.
>
> Why can't you work with offset + cur_order?
>
> Initially,
>
> offset = 0;
> cur_order = HPAGE_PMD_ORDER;
>
> If collapse succeeded, advance to next range.
> If collapse failed, try next smaller order, keeping offset unchanged.
>
> if (failed && cur_order > KHUGEPAGED_MIN_MTHP_ORDER) {
> /* Try next smaller order. */
> cur_order = cur_order - 1;
OK this matches the stack for the 0 offset entries...
> } else {
> /* Skip to next chunk. */
> offset += 1 << cur_order;
> cur_order = max_order_from_offset(offset);
Then 1 << 2 -> 4 so go to offset=4.
max_order_from_offset(4) = 2. so (4, offset=2) same as above.
Then we'd loop back here and go to offset = 8, and max_order_from_offset(8) = 3
And, yeah this seems equivalent.
> }
>
> Of course, handling disabled orders. max_order_from_offset() is rather trivial
> (natural buddy order, capped at HPAGE_PMD_ORDER).
Something like?
static unsigned long max_order_from_offset(unsigned long offset)
{
if (!offset)
return HPAGE_PMD_ORDER;
return ilog2(offset);
}
>
> What's the benefit of the stack?
Yeah it seems equivalent. Good idea!
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH v2 14/18] perf test cs-etm: Remove unused Coresight workloads
From: James Clark @ 2026-06-04 13:34 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Mike Leach, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Amir Ayupov, Jonathan Corbet,
Shuah Khan, Paschalis Mpeis, coresight, linux-perf-users,
linux-kernel, Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <8721d1b4-ce27-4ab0-9a24-138c31440e94@linaro.org>
On 04/06/2026 2:31 pm, James Clark wrote:
>
>
> On 03/06/2026 6:25 pm, Leo Yan wrote:
>> On Tue, Jun 02, 2026 at 03:26:56PM +0100, James Clark wrote:
>>
>> [...]
>>
>>> -When you run perf test, it will do a lot of self tests. Some of those
>>> -tests will cover CoreSight (only if enabled and on ARM64). You
>>> -generally would run perf test from the tools/perf directory in the
>>> -kernel tree. Some tests will check some internal perf support like:
>>> -
>>> - Check Arm CoreSight trace data recording and synthesized samples
>>> - Check Arm SPE trace data recording and synthesized samples
>>
>> Should we keep the chunk for but suggest a perf command as
>>
>> perf test "CoreSight" ?
>>
>
> I put this at the end, not sure if you saw it:
>
> +There are a set of Perf tests for CoreSight which can be run with::
> +
> + sudo perf test coresight
>
>
> "When you run perf test, it will do a lot of self tests." didn't seem
> useful. And the other part about the directory was general advice which
> I'm not even sure is accurate. The tests are supposed to install and you
> can run them from anywhere.
>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index b539be153f6a..7efb893edcbb 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2751,7 +2751,6 @@ F: tools/perf/arch/arm/util/cs-etm.h
>>> F: tools/perf/arch/arm/util/pmu.c
>>> F: tools/perf/tests/shell/*coresight*
>>> F: tools/perf/tests/shell/coresight/*
>>> -F: tools/perf/tests/shell/lib/*coresight*
>>
>> Need to delete tools/perf/tests/shell/coresight/* ?
I delete /*coresight* in the later commit when moving everything into
the coresight/ folder, so only coresight/* remains. I think that's right?
>>
>> Thanks,
>> Leo
>
^ permalink raw reply
* Re: [PATCH v2 14/18] perf test cs-etm: Remove unused Coresight workloads
From: James Clark @ 2026-06-04 13:31 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Mike Leach, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Amir Ayupov, Jonathan Corbet,
Shuah Khan, Paschalis Mpeis, coresight, linux-perf-users,
linux-kernel, Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <20260603172546.GE101133@e132581.arm.com>
On 03/06/2026 6:25 pm, Leo Yan wrote:
> On Tue, Jun 02, 2026 at 03:26:56PM +0100, James Clark wrote:
>
> [...]
>
>> -When you run perf test, it will do a lot of self tests. Some of those
>> -tests will cover CoreSight (only if enabled and on ARM64). You
>> -generally would run perf test from the tools/perf directory in the
>> -kernel tree. Some tests will check some internal perf support like:
>> -
>> - Check Arm CoreSight trace data recording and synthesized samples
>> - Check Arm SPE trace data recording and synthesized samples
>
> Should we keep the chunk for but suggest a perf command as
>
> perf test "CoreSight" ?
>
I put this at the end, not sure if you saw it:
+There are a set of Perf tests for CoreSight which can be run with::
+
+ sudo perf test coresight
"When you run perf test, it will do a lot of self tests." didn't seem
useful. And the other part about the directory was general advice which
I'm not even sure is accurate. The tests are supposed to install and you
can run them from anywhere.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b539be153f6a..7efb893edcbb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2751,7 +2751,6 @@ F: tools/perf/arch/arm/util/cs-etm.h
>> F: tools/perf/arch/arm/util/pmu.c
>> F: tools/perf/tests/shell/*coresight*
>> F: tools/perf/tests/shell/coresight/*
>> -F: tools/perf/tests/shell/lib/*coresight*
>
> Need to delete tools/perf/tests/shell/coresight/* ?
>
> Thanks,
> Leo
^ permalink raw reply
* Re: [PATCH v2 06/18] perf test cs-etm: Replace unroll loop thread with deterministic decode test
From: James Clark @ 2026-06-04 13:21 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Mike Leach, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Amir Ayupov, Jonathan Corbet,
Shuah Khan, Paschalis Mpeis, coresight, linux-perf-users,
linux-kernel, Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <20260603170823.GA101133@e132581.arm.com>
On 03/06/2026 6:08 pm, Leo Yan wrote:
> On Wed, Jun 03, 2026 at 05:01:02PM +0100, James Clark wrote:
>
> [...]
>
>>>> +# Remove open brace lines as they may not be hit depending on the compiler
>>>> +sed -i \
>>>> + -e '/deterministic.c:8$/d' \
>>>> + -e '/deterministic.c:15$/d' \
>>>> + -e '/deterministic.c:23$/d' \
>>>> + "$tmpdir/script"
>>>
>>> Is this related to the function definition?
>>>
>>> I can see the brace lines with change below. It might be more reliable
>>> if adding unused function argument, which can give chance for hit
>>> function entry.
>>>
>>> static int function1(void)
>>> {
>>> ...
>>>
>>> return 0;
>>> }
>>
>> Originally I included the brace lines in the test and it was working even
>> without function arguments, but Sashiko mentioned that they may not always
>> be hit.
>
> I tried Clang to build the program and can see the brace lines are
> missed for function1() / function2().
>
> Does Sashiko mention any reasons causing the issue?
>
>> I think its point was that there is no hard rule about debug symbols for
>> open braces and the behavior might change from one version of the compiler
>> to the next, or whether there is a function prologue or inlining or not etc.
>>
>> I don't think it's important to the test at all though? So to err on the
>> side of caution it makes sense to not test for them. Unless there's a reason
>> you think testing for open braces is important? Surely just testing for
>> actual lines of code appearing in a certain order is enough.
>
> As the test program is named as "deterministic", wouldn't we expect the
> test to hit every code line run in the program?
>
> It is fine for me to skip some checks _if_ we know the reason. I dumped
> the disassembly, it shows function entry is a distinguished position
> from the first calculation sentence (same for both GCC and Clang). And
> there have no difference for a function entry after I tweaked the
> function return type from "void" to "int". I still have no clue why
> brace lines are misses.
>
> Thanks,
> Leo
Seems like the compiler is free to emit a prologue or not for a static
function. Adding an argument might force it to for one version, but I
don't see that being reliable for every future version. And I don't
think we gain anything by testing the braces.
Testing clang did reveal that it returns back to the call site where GCC
didn't though. So I'll also filter out the call site and only test the
"dt_work += 7;" lines. That's enough to check that the decode has done
everything in the right order and shouldn't be fragile to anything that
isn't a real bug.
^ permalink raw reply
* Re: [PATCH v3 1/4] mm/zswap: Make shrink_worker writeback cursor per-memcg
From: Hao Jia @ 2026-06-04 13:06 UTC (permalink / raw)
To: Yosry Ahmed
Cc: akpm, tj, hannes, shakeel.butt, mhocko, mkoutny, nphamcs,
chengming.zhou, muchun.song, roman.gushchin, cgroups, linux-mm,
linux-kernel, linux-doc, Hao Jia
In-Reply-To: <CAO9r8zPBH6-0SQ6-_ZOhTQeyu=rz4F=ugikCrU-JR_skm6fEWA@mail.gmail.com>
On 2026/6/4 13:34, Yosry Ahmed wrote:
>>>> For instance, suppose a parent memcg has two children, memcg1 and memcg2,
>>>> each with 200MB of zswap (100MB inactive). Triggering proactive writeback on
>>>> the parent memcg will exhaust memcg1's inactive zswap pages. After that,
>>>> even though memcg2 still has plenty of inactive zswap pages, it will
>>>> continue to write back memcg1's active zswap pages. Writing back active
>>>> zswap pages causes the user-space agent to prematurely abort the writeback
>>>> because it detects that certain memcg metrics have exceeded predefined
>>>> thresholds.
>>>
>>> This will only happen if the reclaim size is smaller than the batch
>>> size, right? Otherwise the kernel should reclaim more or less equally
>>> from both memcgs?
>>>
>>
>> I gave it some thought. Not using a cursor could lead to unfairness
>> issues with certain writeback sizes:
>>
>> - If the writeback size is an odd multiple of WB_BATCH (e.g.,
>> triggering a writeback of 3 * WB_BATCH), with 2 child cgroups, the
>> writeback ratio might end up being 2:1.
>> - If a memcg has 5 child cgroups and a writeback of 2 * WB_BATCH is
>> triggered, it might repeatedly write back from only the first 2 child
>> cgroups.
>>
>> Although setting a smaller WB_BATCH might mitigate this unfairness, it
>> could hurt writeback efficiency. Let's just use per-memcg cursors to
>> completely fix these corner cases.
>
> Exactly, the batch size should be small enough that any unfairness is
> not a problem. I would honestly just do batching without a per-memcg
> cursor, unless we have numbers to prove that the efficiency is
> affected when we use a small batch size. Let's only introduce
> complexity when needed please.
If you prefer not to use per-cgroup cursors, do we still need to keep
the global cursor (i.e., the root cgroup's cursor) zswap_next_shrink?
I found this part to be quite tricky when trying to reuse the main logic
of shrink_worker() in zswap_proactive_writeback().
Of course, I think we could also keep zswap_next_shrink and write a
small helper to check if it's the root cgroup, allowing us to use
different memcg iteration methods.
Thanks，
Hao
^ permalink raw reply
* Re: [PATCH mm-unstable v18 06/14] mm/khugepaged: generalize collapse_huge_page for mTHP collapse
From: Lorenzo Stoakes @ 2026-06-04 12:55 UTC (permalink / raw)
To: Nico Pache
Cc: linux-doc, linux-kernel, linux-mm, linux-trace-kernel, aarcange,
akpm, anshuman.khandual, apopple, baohua, baolin.wang, byungchul,
catalin.marinas, cl, corbet, dave.hansen, david, dev.jain, gourry,
hannes, hughd, jack, jackmanb, jannh, jglisse, joshua.hahnjy, kas,
lance.yang, liam, mathieu.desnoyers, matthew.brost, mhiramat,
mhocko, peterx, pfalcato, rakie.kim, raquini, rdunlap,
richard.weiyang, rientjes, rostedt, rppt, ryan.roberts, shivankg,
sunnanyong, surenb, thomas.hellstrom, tiwai, usamaarif642, vbabka,
vishal.moola, wangkefeng.wang, will, willy, yang, ying.huang, ziy,
zokeefe, Usama Arif
In-Reply-To: <CAA1CXcDxZEmWtmGFiKDKSPSae8pN0at4vYV24FOs+t_GTGkZ6g@mail.gmail.com>
On Thu, Jun 04, 2026 at 06:45:58AM -0600, Nico Pache wrote:
> On Thu, Jun 4, 2026 at 6:40 AM Lorenzo Stoakes <ljs@kernel.org> wrote:
> >
> > On Thu, Jun 04, 2026 at 12:38:30PM +0100, Lorenzo Stoakes wrote:
> > > I will go review the thread about the cache maintenance separately and
> > > respond about that.
> > >
> > > On Fri, May 22, 2026 at 09:00:01AM -0600, Nico Pache wrote:
> > > > Pass an order and offset to collapse_huge_page to support collapsing anon
> > > > memory to arbitrary orders within a PMD. order indicates what mTHP size we
> > > > are attempting to collapse to, and offset indicates were in the PMD to
> > > > start the collapse attempt.
> > > >
> > > > For non-PMD collapse we must leave the anon VMA write locked until after
> > > > we collapse the mTHP-- in the PMD case all the pages are isolated, but in
> > > > the mTHP case this is not true, and we must keep the lock to prevent
> > > > access/changes to the page tables. This can happen if the rmap walkers hit
> > > > a pmd_none while the PMD entry is currently unavailable due to being
> > > > temporarily removed during the collapse phase.
> > > >
> > > > Acked-by: Usama Arif <usama.arif@linux.dev>
> > > > Signed-off-by: Nico Pache <npache@redhat.com>
> > >
> > > The logic LGTM generally, some questions for understanding below, and of
> > > course as per above I want to review the Lance/David subthread.
> > >
> > > Thanks!
> > >
> > > > ---
> > > > mm/khugepaged.c | 93 +++++++++++++++++++++++++++++--------------------
> > > > 1 file changed, 55 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > > index fab35d318641..d64f42f66236 100644
> > > > --- a/mm/khugepaged.c
> > > > +++ b/mm/khugepaged.c
> > > > @@ -1214,34 +1214,36 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru
> > > > * while allocating a THP, as that could trigger direct reclaim/compaction.
> > > > * Note that the VMA must be rechecked after grabbing the mmap_lock again.
> > > > */
> > > > -static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long address,
> > > > - int referenced, int unmapped, struct collapse_control *cc)
> > > > +static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long start_addr,
> > > > + int referenced, int unmapped, struct collapse_control *cc,
> > > > + unsigned int order)
> > > > {
> > > > + const unsigned long pmd_addr = start_addr & HPAGE_PMD_MASK;
> > > > + const unsigned long end_addr = start_addr + (PAGE_SIZE << order);
> > > > LIST_HEAD(compound_pagelist);
> > > > pmd_t *pmd, _pmd;
> > > > - pte_t *pte;
> > > > + pte_t *pte = NULL;
> > >
> > > As mentioned elsewhere for some reason this was dropped in
> > > mm-unstable. Maybe a bad conflict resolution?
> > >
> > > > pgtable_t pgtable;
> > > > struct folio *folio;
> > > > spinlock_t *pmd_ptl, *pte_ptl;
> > > > enum scan_result result = SCAN_FAIL;
> > > > struct vm_area_struct *vma;
> > > > struct mmu_notifier_range range;
> > > > + bool anon_vma_locked = false;
> > > >
> > > > - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> > > > -
> > > > - result = alloc_charge_folio(&folio, mm, cc, HPAGE_PMD_ORDER);
> > > > + result = alloc_charge_folio(&folio, mm, cc, order);
> > > > if (result != SCAN_SUCCEED)
> > > > goto out_nolock;
> > > >
> > > > mmap_read_lock(mm);
> > > > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > > > - HPAGE_PMD_ORDER);
> > > > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > > > + &vma, cc, order);
> > > > if (result != SCAN_SUCCEED) {
> > > > mmap_read_unlock(mm);
> > > > goto out_nolock;
> > > > }
> > > >
> > > > - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> > > > + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> > > > if (result != SCAN_SUCCEED) {
> > > > mmap_read_unlock(mm);
> > > > goto out_nolock;
> > > > @@ -1253,8 +1255,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > > * released when it fails. So we jump out_nolock directly in
> > > > * that case. Continuing to collapse causes inconsistency.
> > > > */
> > > > - result = __collapse_huge_page_swapin(mm, vma, address, pmd,
> > > > - referenced, HPAGE_PMD_ORDER);
> > > > + result = __collapse_huge_page_swapin(mm, vma, start_addr, pmd,
> > > > + referenced, order);
> > > > if (result != SCAN_SUCCEED)
> > > > goto out_nolock;
> > > > }
> > > > @@ -1269,20 +1271,21 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > > * mmap_lock.
> > > > */
> > > > mmap_write_lock(mm);
> > > > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > > > - HPAGE_PMD_ORDER);
> > > > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > > > + &vma, cc, order);
> > > > if (result != SCAN_SUCCEED)
> > > > goto out_up_write;
> > > > /* check if the pmd is still valid */
> > > > vma_start_write(vma);
> >
> > Hmm actually I think we have another problem here.
> >
> > For PMD THP this is fine. Only a single VMA can span the range we need, and it
> > will span the entire PMD.
> >
> > But for mTHP we have an issue...
> >
> > See below...
> >
> > > > - result = check_pmd_still_valid(mm, address, pmd);
> > > > + result = check_pmd_still_valid(mm, pmd_addr, pmd);
> > > > if (result != SCAN_SUCCEED)
> > > > goto out_up_write;
> > > >
> > > > anon_vma_lock_write(vma->anon_vma);
> > > > + anon_vma_locked = true;
> > >
> > > I worry that we hold this lock a lot longer now? Maybe the algorithmic
> > > change alters that, but Claude did suggest on the s390 bug that longer lock
> > > hold might be an issue.
> > >
> > > I wonder if we'll observe lock contention as a result?
> > >
> > > Correct me if I'm wrong and we're not holding longer than previously,
> > > however. Just appears that we do.
> > >
> > > >
> > > > - mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, address,
> > > > - address + HPAGE_PMD_SIZE);
> > > > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, start_addr,
> > > > + end_addr);
> > > > mmu_notifier_invalidate_range_start(&range);
> > > >
> > > > pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */
> > > > @@ -1294,26 +1297,23 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > > * Parallel GUP-fast is fine since GUP-fast will back off when
> > > > * it detects PMD is changed.
> > > > */
> > > > - _pmd = pmdp_collapse_flush(vma, address, pmd);
> > > > + _pmd = pmdp_collapse_flush(vma, pmd_addr, pmd);
> >
> > ...So we exclude VMA locked faults faulting in a new PMD entry for PMD-sized THP
> > but for mTHP we might have _another_ VMA that spans another part of the range
> > mapped by the same PMD entry.
> >
> > So we clear this, but we do not have a write lock on any other VMA, and so
> > racing VMA read locks can install a new PMD entry.
> >
> > > > spin_unlock(pmd_ptl);
> >
> > Especially since you unlock this :)
> >
> > And...
> >
> > > > mmu_notifier_invalidate_range_end(&range);
> > > > tlb_remove_table_sync_one();
> > > >
> > > > - pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl);
> > > > + pte = pte_offset_map_lock(mm, &_pmd, start_addr, &pte_ptl);
> > > > if (pte) {
> > > > - result = __collapse_huge_page_isolate(vma, address, pte, cc,
> > > > - HPAGE_PMD_ORDER,
> > > > - &compound_pagelist);
> > > > + result = __collapse_huge_page_isolate(vma, start_addr, pte, cc,
> > > > + order, &compound_pagelist);
> > > > spin_unlock(pte_ptl);
> > > > } else {
> > > > result = SCAN_NO_PTE_TABLE;
> > > > }
> > > >
> > > > if (unlikely(result != SCAN_SUCCEED)) {
> > > > - if (pte)
> > > > - pte_unmap(pte);
> > >
> > > OK I seem to remember this is because we're holding the anon_vma lock
> > > longer. That does imply that on e.g. x86-64 the RCU lock is being held a
> > > bit longer also as well as the anon_vma loc.
> > >
> > > I guess it's also because we need to hold anon_vma and pte lock because
> > > we're fiddling around at PTE level for mTHP not just PMD level as 'classic'
> > > THP did.
> > >
> > > (Rememberings going on here :)
> > >
> > > > spin_lock(pmd_ptl);
> > > > - BUG_ON(!pmd_none(*pmd));
> > > > + WARN_ON_ONCE(!pmd_none(*pmd));
> >
> > ...this will get triggered.
> >
> > I don't know whether we can safely hold the PMD lock across everything here for
> > mTHP?
> >
> > Maybe the solution would have to be to scan through VMAs in the range of the PMD
> > and VMA write lock each of them?
>
> I believe we've spoken about this before, but because we always make
Maybe worth a comment then...? Ah how rewarding review is :)
This is something that somebody else might very well wonder about and
forget that it happens to be covered there.
Also:
/* Always check the PMD order to ensure its not shared by another VMA */
Is pretty lightweight there. Something about avoiding racing page faults
would be helpful.
> sure the VMA spans the full PMD we won't ever hit this issue. If we
> wanted to support mTHP collapse on regions smaller than a PMD, the
> locking gets tricky (hence the design choice to not do that for now).
>
> This is handled by the HPAGE_ORDER in hugepage_vma_revalidate().
The existing code is atrocious, and sticking this on top has added to the
pile of assumptions and conventions and having to go check a bunch of
functions to 'just know' you're safe for X, Y, Z.
We really need to see some cleanup series coming after this and I'm going
to get pretty grumpy(ier) if we don't.
>
> /* Always check the PMD order to ensure its not shared by another VMA */
> if (!thp_vma_suitable_order(vma, address, PMD_ORDER))
>
> -- Nico
>
> >
> > That could cause some 'interesting' lock contention issues though? Then again,
> > we will be releasing the mmap write lock soon enough which will drop the VMA
> > write locks.
> >
> > > > /*
> > > > * We can only use set_pmd_at when establishing
> > > > * hugepmds and never for establishing regular pmds that
> > > > @@ -1321,21 +1321,24 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > > */
> > > > pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> > > > spin_unlock(pmd_ptl);
> > > > - anon_vma_unlock_write(vma->anon_vma);
> > > > goto out_up_write;
> > > > }
> > > >
> > > > /*
> > > > - * All pages are isolated and locked so anon_vma rmap
> > > > - * can't run anymore.
> > > > + * For PMD collapse all pages are isolated and locked so anon_vma
> > > > + * rmap can't run anymore. For mTHP collapse the PMD entry has been
> > > > + * removed and not all pages are isolated and locked, so we must hold
> > >
> > > Right because some PTE entries be unaffected by the change.
> > >
> > > > + * the lock to prevent neighboring folios from attempting to access
> > > > + * this PMD until its reinstalled.
> > >
> > > OK. This is slightly annoying for my CoW context work as it means there's
> > > another case where we need to explicitly hold an anon_vma lock for
> > > correctness :)
> > >
> > > Anyway I will think about that separately, is what it is. And in fact
> > > motivates to want this merged earlier so I can work against it :)
> > >
> > >
> > > > */
> > > > - anon_vma_unlock_write(vma->anon_vma);
> > > > + if (is_pmd_order(order)) {
> > > > + anon_vma_unlock_write(vma->anon_vma);
> > > > + anon_vma_locked = false;
> > > > + }
> > > >
> > > > result = __collapse_huge_page_copy(pte, folio, pmd, _pmd,
> > > > - vma, address, pte_ptl,
> > > > - HPAGE_PMD_ORDER,
> > > > - &compound_pagelist);
> > > > - pte_unmap(pte);
> > > > + vma, start_addr, pte_ptl,
> > > > + order, &compound_pagelist);
> > > > if (unlikely(result != SCAN_SUCCEED))
> > > > goto out_up_write;
> > > >
> > > > @@ -1345,18 +1348,32 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > > * write.
> > > > */
> > > > __folio_mark_uptodate(folio);
> > > > - pgtable = pmd_pgtable(_pmd);
> > > > -
> > > > spin_lock(pmd_ptl);
> > > > - BUG_ON(!pmd_none(*pmd));
> > > > - pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > > > - map_anon_folio_pmd_nopf(folio, pmd, vma, address);
> > > > + WARN_ON_ONCE(!pmd_none(*pmd));
> > > > + if (is_pmd_order(order)) {
> > > > + pgtable = pmd_pgtable(_pmd);
> > > > + pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > > > + map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
> > > > + } else {
> > > > + /*
> > > > + * set_ptes is called in map_anon_folio_pte_nopf with the
> > > > + * pmd_ptl lock still held; this is safe as the PMD is expected
> > >
> > > PMD entry you mean?
> > >
> > > > + * to be none. The pmd entry is then repopulated below.
> > > > + */
> > > > + map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
> > >
> > > So here we populate entries in the existing PTE _table_ to point at the new
> > > order>0 folio? With arm64 of course doing transparent contpte stuff?
> > >
> > > > + smp_wmb(); /* make PTEs visible before PMD. See pmd_install() */
> > > > + pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> > >
> > > And then we reinstall the pre-existing PMD _entry_ from none -> what it was
> > > before?
> > >
> > > > + }
> > > > spin_unlock(pmd_ptl);
> > > >
> > > > folio = NULL;
> > > >
> > > > result = SCAN_SUCCEED;
> > > > out_up_write:
> > > > + if (anon_vma_locked)
> > > > + anon_vma_unlock_write(vma->anon_vma);
> > > > + if (pte)
> > > > + pte_unmap(pte);
> > > > mmap_write_unlock(mm);
> > > > out_nolock:
> > > > if (folio)
> > > > @@ -1536,7 +1553,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> > > > /* collapse_huge_page expects the lock to be dropped before calling */
> > > > mmap_read_unlock(mm);
> > > > result = collapse_huge_page(mm, start_addr, referenced,
> > > > - unmapped, cc);
> > > > + unmapped, cc, HPAGE_PMD_ORDER);
> > > > /* collapse_huge_page will return with the mmap_lock released */
> > > > *lock_dropped = true;
> > > > }
> > > > --
> > > > 2.54.0
> > > >
> >
> > Thanks, Lorenzo
> >
>
^ permalink raw reply
* Re: [PATCH mm-unstable v18 06/14] mm/khugepaged: generalize collapse_huge_page for mTHP collapse
From: Nico Pache @ 2026-06-04 12:45 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: linux-doc, linux-kernel, linux-mm, linux-trace-kernel, aarcange,
akpm, anshuman.khandual, apopple, baohua, baolin.wang, byungchul,
catalin.marinas, cl, corbet, dave.hansen, david, dev.jain, gourry,
hannes, hughd, jack, jackmanb, jannh, jglisse, joshua.hahnjy, kas,
lance.yang, liam, mathieu.desnoyers, matthew.brost, mhiramat,
mhocko, peterx, pfalcato, rakie.kim, raquini, rdunlap,
richard.weiyang, rientjes, rostedt, rppt, ryan.roberts, shivankg,
sunnanyong, surenb, thomas.hellstrom, tiwai, usamaarif642, vbabka,
vishal.moola, wangkefeng.wang, will, willy, yang, ying.huang, ziy,
zokeefe, Usama Arif
In-Reply-To: <aiFw80oLty6F_-8m@lucifer>
On Thu, Jun 4, 2026 at 6:40 AM Lorenzo Stoakes <ljs@kernel.org> wrote:
>
> On Thu, Jun 04, 2026 at 12:38:30PM +0100, Lorenzo Stoakes wrote:
> > I will go review the thread about the cache maintenance separately and
> > respond about that.
> >
> > On Fri, May 22, 2026 at 09:00:01AM -0600, Nico Pache wrote:
> > > Pass an order and offset to collapse_huge_page to support collapsing anon
> > > memory to arbitrary orders within a PMD. order indicates what mTHP size we
> > > are attempting to collapse to, and offset indicates were in the PMD to
> > > start the collapse attempt.
> > >
> > > For non-PMD collapse we must leave the anon VMA write locked until after
> > > we collapse the mTHP-- in the PMD case all the pages are isolated, but in
> > > the mTHP case this is not true, and we must keep the lock to prevent
> > > access/changes to the page tables. This can happen if the rmap walkers hit
> > > a pmd_none while the PMD entry is currently unavailable due to being
> > > temporarily removed during the collapse phase.
> > >
> > > Acked-by: Usama Arif <usama.arif@linux.dev>
> > > Signed-off-by: Nico Pache <npache@redhat.com>
> >
> > The logic LGTM generally, some questions for understanding below, and of
> > course as per above I want to review the Lance/David subthread.
> >
> > Thanks!
> >
> > > ---
> > > mm/khugepaged.c | 93 +++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 55 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index fab35d318641..d64f42f66236 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -1214,34 +1214,36 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru
> > > * while allocating a THP, as that could trigger direct reclaim/compaction.
> > > * Note that the VMA must be rechecked after grabbing the mmap_lock again.
> > > */
> > > -static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long address,
> > > - int referenced, int unmapped, struct collapse_control *cc)
> > > +static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long start_addr,
> > > + int referenced, int unmapped, struct collapse_control *cc,
> > > + unsigned int order)
> > > {
> > > + const unsigned long pmd_addr = start_addr & HPAGE_PMD_MASK;
> > > + const unsigned long end_addr = start_addr + (PAGE_SIZE << order);
> > > LIST_HEAD(compound_pagelist);
> > > pmd_t *pmd, _pmd;
> > > - pte_t *pte;
> > > + pte_t *pte = NULL;
> >
> > As mentioned elsewhere for some reason this was dropped in
> > mm-unstable. Maybe a bad conflict resolution?
> >
> > > pgtable_t pgtable;
> > > struct folio *folio;
> > > spinlock_t *pmd_ptl, *pte_ptl;
> > > enum scan_result result = SCAN_FAIL;
> > > struct vm_area_struct *vma;
> > > struct mmu_notifier_range range;
> > > + bool anon_vma_locked = false;
> > >
> > > - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> > > -
> > > - result = alloc_charge_folio(&folio, mm, cc, HPAGE_PMD_ORDER);
> > > + result = alloc_charge_folio(&folio, mm, cc, order);
> > > if (result != SCAN_SUCCEED)
> > > goto out_nolock;
> > >
> > > mmap_read_lock(mm);
> > > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > > - HPAGE_PMD_ORDER);
> > > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > > + &vma, cc, order);
> > > if (result != SCAN_SUCCEED) {
> > > mmap_read_unlock(mm);
> > > goto out_nolock;
> > > }
> > >
> > > - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> > > + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> > > if (result != SCAN_SUCCEED) {
> > > mmap_read_unlock(mm);
> > > goto out_nolock;
> > > @@ -1253,8 +1255,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > * released when it fails. So we jump out_nolock directly in
> > > * that case. Continuing to collapse causes inconsistency.
> > > */
> > > - result = __collapse_huge_page_swapin(mm, vma, address, pmd,
> > > - referenced, HPAGE_PMD_ORDER);
> > > + result = __collapse_huge_page_swapin(mm, vma, start_addr, pmd,
> > > + referenced, order);
> > > if (result != SCAN_SUCCEED)
> > > goto out_nolock;
> > > }
> > > @@ -1269,20 +1271,21 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > * mmap_lock.
> > > */
> > > mmap_write_lock(mm);
> > > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > > - HPAGE_PMD_ORDER);
> > > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > > + &vma, cc, order);
> > > if (result != SCAN_SUCCEED)
> > > goto out_up_write;
> > > /* check if the pmd is still valid */
> > > vma_start_write(vma);
>
> Hmm actually I think we have another problem here.
>
> For PMD THP this is fine. Only a single VMA can span the range we need, and it
> will span the entire PMD.
>
> But for mTHP we have an issue...
>
> See below...
>
> > > - result = check_pmd_still_valid(mm, address, pmd);
> > > + result = check_pmd_still_valid(mm, pmd_addr, pmd);
> > > if (result != SCAN_SUCCEED)
> > > goto out_up_write;
> > >
> > > anon_vma_lock_write(vma->anon_vma);
> > > + anon_vma_locked = true;
> >
> > I worry that we hold this lock a lot longer now? Maybe the algorithmic
> > change alters that, but Claude did suggest on the s390 bug that longer lock
> > hold might be an issue.
> >
> > I wonder if we'll observe lock contention as a result?
> >
> > Correct me if I'm wrong and we're not holding longer than previously,
> > however. Just appears that we do.
> >
> > >
> > > - mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, address,
> > > - address + HPAGE_PMD_SIZE);
> > > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, start_addr,
> > > + end_addr);
> > > mmu_notifier_invalidate_range_start(&range);
> > >
> > > pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */
> > > @@ -1294,26 +1297,23 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > * Parallel GUP-fast is fine since GUP-fast will back off when
> > > * it detects PMD is changed.
> > > */
> > > - _pmd = pmdp_collapse_flush(vma, address, pmd);
> > > + _pmd = pmdp_collapse_flush(vma, pmd_addr, pmd);
>
> ...So we exclude VMA locked faults faulting in a new PMD entry for PMD-sized THP
> but for mTHP we might have _another_ VMA that spans another part of the range
> mapped by the same PMD entry.
>
> So we clear this, but we do not have a write lock on any other VMA, and so
> racing VMA read locks can install a new PMD entry.
>
> > > spin_unlock(pmd_ptl);
>
> Especially since you unlock this :)
>
> And...
>
> > > mmu_notifier_invalidate_range_end(&range);
> > > tlb_remove_table_sync_one();
> > >
> > > - pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl);
> > > + pte = pte_offset_map_lock(mm, &_pmd, start_addr, &pte_ptl);
> > > if (pte) {
> > > - result = __collapse_huge_page_isolate(vma, address, pte, cc,
> > > - HPAGE_PMD_ORDER,
> > > - &compound_pagelist);
> > > + result = __collapse_huge_page_isolate(vma, start_addr, pte, cc,
> > > + order, &compound_pagelist);
> > > spin_unlock(pte_ptl);
> > > } else {
> > > result = SCAN_NO_PTE_TABLE;
> > > }
> > >
> > > if (unlikely(result != SCAN_SUCCEED)) {
> > > - if (pte)
> > > - pte_unmap(pte);
> >
> > OK I seem to remember this is because we're holding the anon_vma lock
> > longer. That does imply that on e.g. x86-64 the RCU lock is being held a
> > bit longer also as well as the anon_vma loc.
> >
> > I guess it's also because we need to hold anon_vma and pte lock because
> > we're fiddling around at PTE level for mTHP not just PMD level as 'classic'
> > THP did.
> >
> > (Rememberings going on here :)
> >
> > > spin_lock(pmd_ptl);
> > > - BUG_ON(!pmd_none(*pmd));
> > > + WARN_ON_ONCE(!pmd_none(*pmd));
>
> ...this will get triggered.
>
> I don't know whether we can safely hold the PMD lock across everything here for
> mTHP?
>
> Maybe the solution would have to be to scan through VMAs in the range of the PMD
> and VMA write lock each of them?
I believe we've spoken about this before, but because we always make
sure the VMA spans the full PMD we won't ever hit this issue. If we
wanted to support mTHP collapse on regions smaller than a PMD, the
locking gets tricky (hence the design choice to not do that for now).
This is handled by the HPAGE_ORDER in hugepage_vma_revalidate().
/* Always check the PMD order to ensure its not shared by another VMA */
if (!thp_vma_suitable_order(vma, address, PMD_ORDER))
-- Nico
>
> That could cause some 'interesting' lock contention issues though? Then again,
> we will be releasing the mmap write lock soon enough which will drop the VMA
> write locks.
>
> > > /*
> > > * We can only use set_pmd_at when establishing
> > > * hugepmds and never for establishing regular pmds that
> > > @@ -1321,21 +1321,24 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > */
> > > pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> > > spin_unlock(pmd_ptl);
> > > - anon_vma_unlock_write(vma->anon_vma);
> > > goto out_up_write;
> > > }
> > >
> > > /*
> > > - * All pages are isolated and locked so anon_vma rmap
> > > - * can't run anymore.
> > > + * For PMD collapse all pages are isolated and locked so anon_vma
> > > + * rmap can't run anymore. For mTHP collapse the PMD entry has been
> > > + * removed and not all pages are isolated and locked, so we must hold
> >
> > Right because some PTE entries be unaffected by the change.
> >
> > > + * the lock to prevent neighboring folios from attempting to access
> > > + * this PMD until its reinstalled.
> >
> > OK. This is slightly annoying for my CoW context work as it means there's
> > another case where we need to explicitly hold an anon_vma lock for
> > correctness :)
> >
> > Anyway I will think about that separately, is what it is. And in fact
> > motivates to want this merged earlier so I can work against it :)
> >
> >
> > > */
> > > - anon_vma_unlock_write(vma->anon_vma);
> > > + if (is_pmd_order(order)) {
> > > + anon_vma_unlock_write(vma->anon_vma);
> > > + anon_vma_locked = false;
> > > + }
> > >
> > > result = __collapse_huge_page_copy(pte, folio, pmd, _pmd,
> > > - vma, address, pte_ptl,
> > > - HPAGE_PMD_ORDER,
> > > - &compound_pagelist);
> > > - pte_unmap(pte);
> > > + vma, start_addr, pte_ptl,
> > > + order, &compound_pagelist);
> > > if (unlikely(result != SCAN_SUCCEED))
> > > goto out_up_write;
> > >
> > > @@ -1345,18 +1348,32 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > > * write.
> > > */
> > > __folio_mark_uptodate(folio);
> > > - pgtable = pmd_pgtable(_pmd);
> > > -
> > > spin_lock(pmd_ptl);
> > > - BUG_ON(!pmd_none(*pmd));
> > > - pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > > - map_anon_folio_pmd_nopf(folio, pmd, vma, address);
> > > + WARN_ON_ONCE(!pmd_none(*pmd));
> > > + if (is_pmd_order(order)) {
> > > + pgtable = pmd_pgtable(_pmd);
> > > + pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > > + map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
> > > + } else {
> > > + /*
> > > + * set_ptes is called in map_anon_folio_pte_nopf with the
> > > + * pmd_ptl lock still held; this is safe as the PMD is expected
> >
> > PMD entry you mean?
> >
> > > + * to be none. The pmd entry is then repopulated below.
> > > + */
> > > + map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
> >
> > So here we populate entries in the existing PTE _table_ to point at the new
> > order>0 folio? With arm64 of course doing transparent contpte stuff?
> >
> > > + smp_wmb(); /* make PTEs visible before PMD. See pmd_install() */
> > > + pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> >
> > And then we reinstall the pre-existing PMD _entry_ from none -> what it was
> > before?
> >
> > > + }
> > > spin_unlock(pmd_ptl);
> > >
> > > folio = NULL;
> > >
> > > result = SCAN_SUCCEED;
> > > out_up_write:
> > > + if (anon_vma_locked)
> > > + anon_vma_unlock_write(vma->anon_vma);
> > > + if (pte)
> > > + pte_unmap(pte);
> > > mmap_write_unlock(mm);
> > > out_nolock:
> > > if (folio)
> > > @@ -1536,7 +1553,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> > > /* collapse_huge_page expects the lock to be dropped before calling */
> > > mmap_read_unlock(mm);
> > > result = collapse_huge_page(mm, start_addr, referenced,
> > > - unmapped, cc);
> > > + unmapped, cc, HPAGE_PMD_ORDER);
> > > /* collapse_huge_page will return with the mmap_lock released */
> > > *lock_dropped = true;
> > > }
> > > --
> > > 2.54.0
> > >
>
> Thanks, Lorenzo
>
^ permalink raw reply
* Re: [PATCH mm-unstable v18 06/14] mm/khugepaged: generalize collapse_huge_page for mTHP collapse
From: Lorenzo Stoakes @ 2026-06-04 12:39 UTC (permalink / raw)
To: Nico Pache
Cc: linux-doc, linux-kernel, linux-mm, linux-trace-kernel, aarcange,
akpm, anshuman.khandual, apopple, baohua, baolin.wang, byungchul,
catalin.marinas, cl, corbet, dave.hansen, david, dev.jain, gourry,
hannes, hughd, jack, jackmanb, jannh, jglisse, joshua.hahnjy, kas,
lance.yang, liam, mathieu.desnoyers, matthew.brost, mhiramat,
mhocko, peterx, pfalcato, rakie.kim, raquini, rdunlap,
richard.weiyang, rientjes, rostedt, rppt, ryan.roberts, shivankg,
sunnanyong, surenb, thomas.hellstrom, tiwai, usamaarif642, vbabka,
vishal.moola, wangkefeng.wang, will, willy, yang, ying.huang, ziy,
zokeefe, Usama Arif
In-Reply-To: <aiFTSLb0kkTR7I9A@lucifer>
On Thu, Jun 04, 2026 at 12:38:30PM +0100, Lorenzo Stoakes wrote:
> I will go review the thread about the cache maintenance separately and
> respond about that.
>
> On Fri, May 22, 2026 at 09:00:01AM -0600, Nico Pache wrote:
> > Pass an order and offset to collapse_huge_page to support collapsing anon
> > memory to arbitrary orders within a PMD. order indicates what mTHP size we
> > are attempting to collapse to, and offset indicates were in the PMD to
> > start the collapse attempt.
> >
> > For non-PMD collapse we must leave the anon VMA write locked until after
> > we collapse the mTHP-- in the PMD case all the pages are isolated, but in
> > the mTHP case this is not true, and we must keep the lock to prevent
> > access/changes to the page tables. This can happen if the rmap walkers hit
> > a pmd_none while the PMD entry is currently unavailable due to being
> > temporarily removed during the collapse phase.
> >
> > Acked-by: Usama Arif <usama.arif@linux.dev>
> > Signed-off-by: Nico Pache <npache@redhat.com>
>
> The logic LGTM generally, some questions for understanding below, and of
> course as per above I want to review the Lance/David subthread.
>
> Thanks!
>
> > ---
> > mm/khugepaged.c | 93 +++++++++++++++++++++++++++++--------------------
> > 1 file changed, 55 insertions(+), 38 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index fab35d318641..d64f42f66236 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1214,34 +1214,36 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru
> > * while allocating a THP, as that could trigger direct reclaim/compaction.
> > * Note that the VMA must be rechecked after grabbing the mmap_lock again.
> > */
> > -static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long address,
> > - int referenced, int unmapped, struct collapse_control *cc)
> > +static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long start_addr,
> > + int referenced, int unmapped, struct collapse_control *cc,
> > + unsigned int order)
> > {
> > + const unsigned long pmd_addr = start_addr & HPAGE_PMD_MASK;
> > + const unsigned long end_addr = start_addr + (PAGE_SIZE << order);
> > LIST_HEAD(compound_pagelist);
> > pmd_t *pmd, _pmd;
> > - pte_t *pte;
> > + pte_t *pte = NULL;
>
> As mentioned elsewhere for some reason this was dropped in
> mm-unstable. Maybe a bad conflict resolution?
>
> > pgtable_t pgtable;
> > struct folio *folio;
> > spinlock_t *pmd_ptl, *pte_ptl;
> > enum scan_result result = SCAN_FAIL;
> > struct vm_area_struct *vma;
> > struct mmu_notifier_range range;
> > + bool anon_vma_locked = false;
> >
> > - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> > -
> > - result = alloc_charge_folio(&folio, mm, cc, HPAGE_PMD_ORDER);
> > + result = alloc_charge_folio(&folio, mm, cc, order);
> > if (result != SCAN_SUCCEED)
> > goto out_nolock;
> >
> > mmap_read_lock(mm);
> > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > - HPAGE_PMD_ORDER);
> > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > + &vma, cc, order);
> > if (result != SCAN_SUCCEED) {
> > mmap_read_unlock(mm);
> > goto out_nolock;
> > }
> >
> > - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> > + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> > if (result != SCAN_SUCCEED) {
> > mmap_read_unlock(mm);
> > goto out_nolock;
> > @@ -1253,8 +1255,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > * released when it fails. So we jump out_nolock directly in
> > * that case. Continuing to collapse causes inconsistency.
> > */
> > - result = __collapse_huge_page_swapin(mm, vma, address, pmd,
> > - referenced, HPAGE_PMD_ORDER);
> > + result = __collapse_huge_page_swapin(mm, vma, start_addr, pmd,
> > + referenced, order);
> > if (result != SCAN_SUCCEED)
> > goto out_nolock;
> > }
> > @@ -1269,20 +1271,21 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > * mmap_lock.
> > */
> > mmap_write_lock(mm);
> > - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> > - HPAGE_PMD_ORDER);
> > + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> > + &vma, cc, order);
> > if (result != SCAN_SUCCEED)
> > goto out_up_write;
> > /* check if the pmd is still valid */
> > vma_start_write(vma);
Hmm actually I think we have another problem here.
For PMD THP this is fine. Only a single VMA can span the range we need, and it
will span the entire PMD.
But for mTHP we have an issue...
See below...
> > - result = check_pmd_still_valid(mm, address, pmd);
> > + result = check_pmd_still_valid(mm, pmd_addr, pmd);
> > if (result != SCAN_SUCCEED)
> > goto out_up_write;
> >
> > anon_vma_lock_write(vma->anon_vma);
> > + anon_vma_locked = true;
>
> I worry that we hold this lock a lot longer now? Maybe the algorithmic
> change alters that, but Claude did suggest on the s390 bug that longer lock
> hold might be an issue.
>
> I wonder if we'll observe lock contention as a result?
>
> Correct me if I'm wrong and we're not holding longer than previously,
> however. Just appears that we do.
>
> >
> > - mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, address,
> > - address + HPAGE_PMD_SIZE);
> > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, start_addr,
> > + end_addr);
> > mmu_notifier_invalidate_range_start(&range);
> >
> > pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */
> > @@ -1294,26 +1297,23 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > * Parallel GUP-fast is fine since GUP-fast will back off when
> > * it detects PMD is changed.
> > */
> > - _pmd = pmdp_collapse_flush(vma, address, pmd);
> > + _pmd = pmdp_collapse_flush(vma, pmd_addr, pmd);
...So we exclude VMA locked faults faulting in a new PMD entry for PMD-sized THP
but for mTHP we might have _another_ VMA that spans another part of the range
mapped by the same PMD entry.
So we clear this, but we do not have a write lock on any other VMA, and so
racing VMA read locks can install a new PMD entry.
> > spin_unlock(pmd_ptl);
Especially since you unlock this :)
And...
> > mmu_notifier_invalidate_range_end(&range);
> > tlb_remove_table_sync_one();
> >
> > - pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl);
> > + pte = pte_offset_map_lock(mm, &_pmd, start_addr, &pte_ptl);
> > if (pte) {
> > - result = __collapse_huge_page_isolate(vma, address, pte, cc,
> > - HPAGE_PMD_ORDER,
> > - &compound_pagelist);
> > + result = __collapse_huge_page_isolate(vma, start_addr, pte, cc,
> > + order, &compound_pagelist);
> > spin_unlock(pte_ptl);
> > } else {
> > result = SCAN_NO_PTE_TABLE;
> > }
> >
> > if (unlikely(result != SCAN_SUCCEED)) {
> > - if (pte)
> > - pte_unmap(pte);
>
> OK I seem to remember this is because we're holding the anon_vma lock
> longer. That does imply that on e.g. x86-64 the RCU lock is being held a
> bit longer also as well as the anon_vma loc.
>
> I guess it's also because we need to hold anon_vma and pte lock because
> we're fiddling around at PTE level for mTHP not just PMD level as 'classic'
> THP did.
>
> (Rememberings going on here :)
>
> > spin_lock(pmd_ptl);
> > - BUG_ON(!pmd_none(*pmd));
> > + WARN_ON_ONCE(!pmd_none(*pmd));
...this will get triggered.
I don't know whether we can safely hold the PMD lock across everything here for
mTHP?
Maybe the solution would have to be to scan through VMAs in the range of the PMD
and VMA write lock each of them?
That could cause some 'interesting' lock contention issues though? Then again,
we will be releasing the mmap write lock soon enough which will drop the VMA
write locks.
> > /*
> > * We can only use set_pmd_at when establishing
> > * hugepmds and never for establishing regular pmds that
> > @@ -1321,21 +1321,24 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > */
> > pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> > spin_unlock(pmd_ptl);
> > - anon_vma_unlock_write(vma->anon_vma);
> > goto out_up_write;
> > }
> >
> > /*
> > - * All pages are isolated and locked so anon_vma rmap
> > - * can't run anymore.
> > + * For PMD collapse all pages are isolated and locked so anon_vma
> > + * rmap can't run anymore. For mTHP collapse the PMD entry has been
> > + * removed and not all pages are isolated and locked, so we must hold
>
> Right because some PTE entries be unaffected by the change.
>
> > + * the lock to prevent neighboring folios from attempting to access
> > + * this PMD until its reinstalled.
>
> OK. This is slightly annoying for my CoW context work as it means there's
> another case where we need to explicitly hold an anon_vma lock for
> correctness :)
>
> Anyway I will think about that separately, is what it is. And in fact
> motivates to want this merged earlier so I can work against it :)
>
>
> > */
> > - anon_vma_unlock_write(vma->anon_vma);
> > + if (is_pmd_order(order)) {
> > + anon_vma_unlock_write(vma->anon_vma);
> > + anon_vma_locked = false;
> > + }
> >
> > result = __collapse_huge_page_copy(pte, folio, pmd, _pmd,
> > - vma, address, pte_ptl,
> > - HPAGE_PMD_ORDER,
> > - &compound_pagelist);
> > - pte_unmap(pte);
> > + vma, start_addr, pte_ptl,
> > + order, &compound_pagelist);
> > if (unlikely(result != SCAN_SUCCEED))
> > goto out_up_write;
> >
> > @@ -1345,18 +1348,32 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> > * write.
> > */
> > __folio_mark_uptodate(folio);
> > - pgtable = pmd_pgtable(_pmd);
> > -
> > spin_lock(pmd_ptl);
> > - BUG_ON(!pmd_none(*pmd));
> > - pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > - map_anon_folio_pmd_nopf(folio, pmd, vma, address);
> > + WARN_ON_ONCE(!pmd_none(*pmd));
> > + if (is_pmd_order(order)) {
> > + pgtable = pmd_pgtable(_pmd);
> > + pgtable_trans_huge_deposit(mm, pmd, pgtable);
> > + map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
> > + } else {
> > + /*
> > + * set_ptes is called in map_anon_folio_pte_nopf with the
> > + * pmd_ptl lock still held; this is safe as the PMD is expected
>
> PMD entry you mean?
>
> > + * to be none. The pmd entry is then repopulated below.
> > + */
> > + map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
>
> So here we populate entries in the existing PTE _table_ to point at the new
> order>0 folio? With arm64 of course doing transparent contpte stuff?
>
> > + smp_wmb(); /* make PTEs visible before PMD. See pmd_install() */
> > + pmd_populate(mm, pmd, pmd_pgtable(_pmd));
>
> And then we reinstall the pre-existing PMD _entry_ from none -> what it was
> before?
>
> > + }
> > spin_unlock(pmd_ptl);
> >
> > folio = NULL;
> >
> > result = SCAN_SUCCEED;
> > out_up_write:
> > + if (anon_vma_locked)
> > + anon_vma_unlock_write(vma->anon_vma);
> > + if (pte)
> > + pte_unmap(pte);
> > mmap_write_unlock(mm);
> > out_nolock:
> > if (folio)
> > @@ -1536,7 +1553,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> > /* collapse_huge_page expects the lock to be dropped before calling */
> > mmap_read_unlock(mm);
> > result = collapse_huge_page(mm, start_addr, referenced,
> > - unmapped, cc);
> > + unmapped, cc, HPAGE_PMD_ORDER);
> > /* collapse_huge_page will return with the mmap_lock released */
> > *lock_dropped = true;
> > }
> > --
> > 2.54.0
> >
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH mm-unstable v18 06/14] mm/khugepaged: generalize collapse_huge_page for mTHP collapse
From: Lorenzo Stoakes @ 2026-06-04 12:33 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Lance Yang, npache, linux-doc, linux-kernel, linux-mm,
linux-trace-kernel, aarcange, akpm, anshuman.khandual, apopple,
baohua, baolin.wang, byungchul, catalin.marinas, cl, corbet,
dave.hansen, dev.jain, gourry, hannes, hughd, jack, jackmanb,
jannh, jglisse, joshua.hahnjy, kas, liam, mathieu.desnoyers,
matthew.brost, mhiramat, mhocko, peterx, pfalcato, rakie.kim,
raquini, rdunlap, richard.weiyang, rientjes, rostedt, rppt,
ryan.roberts, shivankg, sunnanyong, surenb, thomas.hellstrom,
tiwai, usamaarif642, vbabka, vishal.moola, wangkefeng.wang, will,
willy, yang, ying.huang, ziy, zokeefe, usama.arif
In-Reply-To: <f5d38f64-ab92-496d-afd3-29ccc17fec2b@kernel.org>
On Mon, Jun 01, 2026 at 08:54:24AM +0200, David Hildenbrand (Arm) wrote:
> On 6/1/26 05:28, Lance Yang wrote:
> >
> > On Sun, May 31, 2026 at 10:00:17PM +0200, David Hildenbrand (Arm) wrote:
> >> On 5/31/26 11:39, Lance Yang wrote:
> >>>
> >>>
> >>> Emm ... is it safe to use map_anon_folio_pte_nopf() here?
> >>>
> >>> At this point pmdp_collapse_flush() has cleared the PMD from the page
> >>> tables. The PTE table we are updating is only reachable through the saved
> >>> old PMD value, _pmd, until pmd_populate() below.
> >>>
> >>> map_anon_folio_pte_nopf() does set_ptes() and then calls
> >>> update_mmu_cache_range(). Documentation/core-api/cachetlb.rst describes
> >>> that hook as:
> >>>
> >>> "
> >>> At the end of every page fault, this routine is invoked to tell
> >>> the architecture specific code that translations now exists
> >>> in the software page tables for address space "vma->vm_mm"
> >>> at virtual address "address" for "nr" consecutive pages.
> >>> "
> >>>
> >>> But that does not seem true here yet, since the PTE table is not
> >>> reachable from vma->vm_mm when update_mmu_cache_range() is called.
> >>>
> >>> Should we avoid calling update_mmu_cache_range() until after the PTE
> >>> table is reinstalled with pmd_populate()?
> >>
> >> I recall that update_mmu_cache* users mostly care about updating folios flags,
> >> for the folio derived from the PTE ... or flushing caches for the user address.
> >>
> >> So intuitively I would say "the architecture code doesn't care that the PMD
> >> table will only be visible to HW shortly after". The important thing should be
> >> that it will definetly happen, and that nothing else is curently there or can be
> >> there?
> >
> > Ah, fair point.
> >
> > I was mostly worried about arch hooks that walk vma->vm_mm again, rather
> > than only using the pte pointer passed in. For example, mips does:
>
> Right, a re-walk would be the real problem.
>
> >
> > update_mmu_cache_range()
> > -> __update_tlb()
> > -> pgd_offset(vma->vm_mm, address)
> > -> pte_offset_map(...)
> >
> > and __update_tlb() has this assumption:
> >
> > /*
> > * update_mmu_cache() is called between pte_offset_map_lock()
> > * and pte_unmap_unlock(), so we can assume that ptep is not
> > * NULL here: and what should be done below if it were NULL?
> > */
> >
> > So if khugepaged happens to run with current->active_mm == vma->vm_mm
> > here, could __update_tlb() hit the none PMD, get NULL from
I really wish people would say Pxx _entry_ :) so confusing.
> > pte_offset_map(), and then dereference it?
>
> Likely yes -- that MIPS code is horrible. And the comment in MIPS code
> even spells that out. :(
>
> Do you know about other code like that, or is MIPS the only one doing a
> re-walk and crossing fingers?
>
> >
> > Just wanted to raise it since some arch code may still have assumptions
> > like this, and the always-enable-mTHP work is getting closer ...
>
> Right. I assume set_pte_at() couldn't trigger something similar (re-walk) in arch code,
> because we simply provide the ptep. update_mmu_cache_range() only consumes the pte.
>
> >
> > Probably very very very hard to hit, though :)
>
> Delaying update_mmu_cache_range() is nasty, as we'd have to make sure that
> nobody can interfere in the meantime ... and the PMD lock will not be sufficient.
>
> Maybe we could reinstall the page table with the cleared (none) entries while
> still holding the PTL?
You mean the cleared PTE entries that are to be updated with the collapsed
larger folio?
>
> Thinking out loud:
After staring at this long enough, this does seems like a viable solution yes.
I hate how subtle this is.
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 5ba298d420b7..e39b750b1e6f 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1413,13 +1413,17 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long s
> map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
> } else {
> /*
> - * set_ptes is called in map_anon_folio_pte_nopf with the
> - * pmd_ptl lock still held; this is safe as the PMD is expected
> - * to be none. The pmd entry is then repopulated below.
> + * Re-insert the page table with the cleared entries, but
> + * hold the PTL, such that no one can mess with the re-installed
> + * page table until we updated the temporarily-cleared entries
> + * through map_anon_folio_pte_nopf().
> */
You may say nit, but, I think we should be clearly stating the problem here. Yes
we want to hold the PTL to stop anybody else messing with it yet, but we're
really doing this because of:
map_anon_folio_pte_nopf
-> update_mmu_cache_range
-> rewalk
-> try to look up an entry that's not yet actually installed
-> bang
Right?
So maybe something like:
Re-insert the PMD entry pointing to the PTE page table with cleared
entries first, because map_anon_folio_pte_nopf() invokes
update_mmu_cache_range() which may cause a rewalk of the page tables and
blow up if the supplied PTE entry belongs to a PTE table that is not yet
present there.
We hold the PTE PTL to avoid anything else messing with this until we're
ready.
> - map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
> - smp_wmb(); /* make PTEs visible before PMD. See pmd_install() */
(I guess better to comment on the smp_wmb() stuff in the other message about
this.)
> + if (pte_ptl != pmd_ptl)
> + spin_lock(pte_ptl);
(Obviously should be spin_lock_nested() as David says later)
It seems a bit weird to me that we acquire the PTE lock:
pte = pte_offset_map_lock(mm, &_pmd, start_addr, &pte_ptl);
Clear out the mTHP entries we're going to remove:
if (pte) {
result = __collapse_huge_page_isolate(vma, start_addr, pte, cc,
order, &compound_pagelist);
THen unlock the PTE:
spin_unlock(pte_ptl);
Before again reacquiring here, especially given this is an unreachable PTE
table.
But then again not doing that would require us to add some error handling logic
to unlock again so it's probably not vital.
> pmd_populate(mm, pmd, pmd_pgtable(_pmd));
So we're protecting against concurrent rmap and fault handlers with the PTL such
that installing this is safe right?
Are we good against GUP fast? I guess a race will be fine with that, or will it?
I suppose before it would have skipped the range entirely because of the missing
PMD entry anyway.
(in any case we also hold anon_vma write lock too.)
> + map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
> + if (pte_ptl != pmd_ptl)
> + spin_unlock(pte_ptl);
> }
> spin_unlock(pmd_ptl);
>
>
>
> --
> Cheers,
>
> David
Thanks, Lorenzo
^ permalink raw reply
* Re: [PATCH v3 01/19] perf cs-etm: Queue context packets for frontend
From: James Clark @ 2026-06-04 12:29 UTC (permalink / raw)
To: Amir Ayupov
Cc: Suzuki K Poulose, Mike Leach, Leo Yan, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Jonathan Corbet, Shuah Khan,
Paschalis Mpeis, coresight, linux-perf-users, linux-kernel,
Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <CAMOD+7+xeh-RNQJUUpGoV6gU3swOcpGX82n2FSNiiewZ2R3vKQ@mail.gmail.com>
On 03/06/2026 9:14 pm, Amir Ayupov wrote:
> Hi James,
>
> I tested the v2 patch series and it looks good. Compared to my
> stamping pid/tid on each packet, there was a minor difference in 2/39
> tested perf data files: the number of brstack samples differs by one,
> however, there was no loss of binary profile. The resulting BOLT
> profile converted from the perf script output was identical, so I'm OK
> with v2 patch as-is.
>
> -Amir
>
Thanks for testing. Seems like Leo might have an explanation for the
branch stack difference on the other thread. If you want to share the
files I can look into it, otherwise I'm ok with ignoring it too.
James
>
>
> On Wed, Jun 3, 2026 at 3:17 AM James Clark <james.clark@linaro.org> wrote:
>>
>>>
>> PE_CONTEXT elements update the context ID and exception level, but the
>> decoder may still have prior packets cached for frontend processing.
>> Updating the context immediately in the decoder backend can make those
>> cached packets get consumed with the wrong thread or EL state.
>>
>> Add a CS_ETM_CONTEXT packet carrying the TID and EL to the frontend,
>> this keeps context changes ordered with the rest of the packet stream
>> and avoids mismatches when synthesizing samples from cached packets.
>>
>> Separate the memory access function into one for the frontend and one
>> for decoding. The frontend also needs memory access to attach the
>> instruction to samples. Because the frontend does memory access for
>> both previous and current packets, change all the frontend memory access
>> function signatures to take both a tidq and packet. But backend always
>> uses the current backend EL and thread from the tidq.
>>
>> Treat context packets as a boundary for branch sample generation and
>> remove tidq->prev_packet_thread because it's not possible to branch to a
>> different thread, so only tracking the current thread is required for
>> sample generation.
>>
>> Fixes: e573e978fb12 ("perf cs-etm: Inject capabilitity for CoreSight traces")
>> Reported-by: Amir Ayupov <aaupov@meta.com>
>> Closes: https://lore.kernel.org/linux-perf-users/20260515021135.1729028-1-aaupov@meta.com/
>> Co-authored-by: James Clark <james.clark@linaro.org>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 21 ++-
>> tools/perf/util/cs-etm.c | 234 ++++++++++++++----------
>> tools/perf/util/cs-etm.h | 8 +-
>> 3 files changed, 162 insertions(+), 101 deletions(-)
>>
>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> index dee3020ceaa9..26940f1f1b0b 100644
>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> @@ -402,6 +402,8 @@ cs_etm_decoder__buffer_packet(struct cs_etm_queue *etmq,
>> packet_queue->packet_buffer[et].flags = 0;
>> packet_queue->packet_buffer[et].exception_number = UINT32_MAX;
>> packet_queue->packet_buffer[et].trace_chan_id = trace_chan_id;
>> + packet_queue->packet_buffer[et].el = ocsd_EL_unknown;
>> + packet_queue->packet_buffer[et].tid = -1;
>>
>> if (packet_queue->packet_count == CS_ETM_PACKET_MAX_BUFFER - 1)
>> return OCSD_RESP_WAIT;
>> @@ -449,6 +451,7 @@ cs_etm_decoder__buffer_range(struct cs_etm_queue *etmq,
>> packet->last_instr_type = elem->last_i_type;
>> packet->last_instr_subtype = elem->last_i_subtype;
>> packet->last_instr_cond = elem->last_instr_cond;
>> + packet->el = elem->context.exception_level;
>>
>> if (elem->last_i_type == OCSD_INSTR_BR || elem->last_i_type == OCSD_INSTR_BR_INDIRECT)
>> packet->last_instr_taken_branch = elem->last_instr_exec;
>> @@ -525,7 +528,9 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
>> const ocsd_generic_trace_elem *elem,
>> const uint8_t trace_chan_id)
>> {
>> + struct cs_etm_packet *packet;
>> pid_t tid = -1;
>> + int ret;
>>
>> /*
>> * Process the PE_CONTEXT packets if we have a valid contextID or VMID.
>> @@ -546,12 +551,18 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
>> break;
>> }
>>
>> - if (cs_etm__etmq_set_tid_el(etmq, tid, trace_chan_id,
>> - elem->context.exception_level))
>> + if (cs_etm__etmq_update_decode_context(etmq, trace_chan_id,
>> + elem->context.exception_level, tid))
>> return OCSD_RESP_FATAL_SYS_ERR;
>>
>> - if (tid == -1)
>> - return OCSD_RESP_CONT;
>> + ret = cs_etm_decoder__buffer_packet(etmq, packet_queue, trace_chan_id,
>> + CS_ETM_CONTEXT);
>> + if (ret != OCSD_RESP_CONT && ret != OCSD_RESP_WAIT)
>> + return ret;
>> +
>> + packet = &packet_queue->packet_buffer[packet_queue->tail];
>> + packet->tid = tid;
>> + packet->el = elem->context.exception_level;
>>
>> /*
>> * A timestamp is generated after a PE_CONTEXT element so make sure
>> @@ -559,7 +570,7 @@ cs_etm_decoder__set_tid(struct cs_etm_queue *etmq,
>> */
>> cs_etm_decoder__reset_timestamp(packet_queue);
>>
>> - return OCSD_RESP_CONT;
>> + return ret;
>> }
>>
>> static ocsd_datapath_resp_t cs_etm_decoder__gen_trace_elem_printer(
>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
>> index 40c6ddfa8c8d..ce570913669c 100644
>> --- a/tools/perf/util/cs-etm.c
>> +++ b/tools/perf/util/cs-etm.c
>> @@ -85,15 +85,22 @@ struct cs_etm_traceid_queue {
>> u64 period_instructions;
>> size_t last_branch_pos;
>> union perf_event *event_buf;
>> - struct thread *thread;
>> - struct thread *prev_packet_thread;
>> - ocsd_ex_level prev_packet_el;
>> - ocsd_ex_level el;
>> struct branch_stack *last_branch;
>> struct branch_stack *last_branch_rb;
>> struct cs_etm_packet *prev_packet;
>> struct cs_etm_packet *packet;
>> struct cs_etm_packet_queue packet_queue;
>> +
>> + struct thread *decode_thread;
>> + ocsd_ex_level decode_el;
>> +
>> + /*
>> + * The frontend accesses the EL from '[prev_]packet' because it needs
>> + * previous EL for branch and current EL for instruction samples. It's
>> + * not possible to change thread in a single branch sample so no need to
>> + * store or access the thread through the packet.
>> + */
>> + struct thread *frontend_thread;
>> };
>>
>> enum cs_etm_format {
>> @@ -614,10 +621,11 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
>>
>> queue = &etmq->etm->queues.queue_array[etmq->queue_nr];
>> tidq->trace_chan_id = trace_chan_id;
>> - tidq->el = tidq->prev_packet_el = ocsd_EL_unknown;
>> - tidq->thread = machine__findnew_thread(&etm->session->machines.host, -1,
>> + tidq->decode_el = ocsd_EL_unknown;
>> + tidq->frontend_thread = machine__findnew_thread(&etm->session->machines.host, -1,
>> + queue->tid);
>> + tidq->decode_thread = machine__findnew_thread(&etm->session->machines.host, -1,
>> queue->tid);
>> - tidq->prev_packet_thread = machine__idle_thread(&etm->session->machines.host);
>>
>> tidq->packet = zalloc(sizeof(struct cs_etm_packet));
>> if (!tidq->packet)
>> @@ -750,21 +758,10 @@ static void cs_etm__packet_swap(struct cs_etm_auxtrace *etm,
>> /*
>> * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
>> * the next incoming packet.
>> - *
>> - * Threads and exception levels are also tracked for both the
>> - * previous and current packets. This is because the previous
>> - * packet is used for the 'from' IP for branch samples, so the
>> - * thread at that time must also be assigned to that sample.
>> - * Across discontinuity packets the thread can change, so by
>> - * tracking the thread for the previous packet the branch sample
>> - * will have the correct info.
>> */
>> tmp = tidq->packet;
>> tidq->packet = tidq->prev_packet;
>> tidq->prev_packet = tmp;
>> - tidq->prev_packet_el = tidq->el;
>> - thread__put(tidq->prev_packet_thread);
>> - tidq->prev_packet_thread = thread__get(tidq->thread);
>> }
>> }
>>
>> @@ -937,8 +934,8 @@ static void cs_etm__free_traceid_queues(struct cs_etm_queue *etmq)
>>
>> /* Free this traceid_queue from the array */
>> tidq = etmq->traceid_queues[idx];
>> - thread__zput(tidq->thread);
>> - thread__zput(tidq->prev_packet_thread);
>> + thread__zput(tidq->frontend_thread);
>> + thread__zput(tidq->decode_thread);
>> zfree(&tidq->event_buf);
>> zfree(&tidq->last_branch);
>> zfree(&tidq->last_branch_rb);
>> @@ -1083,47 +1080,43 @@ static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address,
>> }
>> }
>>
>> -static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> - u64 address, size_t size, u8 *buffer,
>> - const ocsd_mem_space_acc_t mem_space)
>> +static u32 __cs_etm__mem_access(struct cs_etm_queue *etmq,
>> + u64 address, size_t size, u8 *buffer,
>> + const ocsd_mem_space_acc_t mem_space,
>> + ocsd_ex_level el, struct thread *thread)
>> {
>> u8 cpumode;
>> u64 offset;
>> int len;
>> struct addr_location al;
>> struct dso *dso;
>> - struct cs_etm_traceid_queue *tidq;
>> int ret = 0;
>>
>> if (!etmq)
>> return 0;
>>
>> addr_location__init(&al);
>> - tidq = cs_etm__etmq_get_traceid_queue(etmq, trace_chan_id);
>> - if (!tidq)
>> - goto out;
>>
>> /*
>> - * We've already tracked EL along side the PID in cs_etm__set_thread()
>> - * so double check that it matches what OpenCSD thinks as well. It
>> - * doesn't distinguish between EL0 and EL1 for this mem access callback
>> - * so we had to do the extra tracking. Skip validation if it's any of
>> - * the 'any' values.
>> + * We track EL for the frontend and the backend when receiving context
>> + * and range packets. OpenCSD doesn't distinguish between EL0 and EL1
>> + * for this mem access callback so we had to do the extra tracking. Skip
>> + * validation if it's any of the 'any' values.
>> */
>> if (!(mem_space == OCSD_MEM_SPACE_ANY ||
>> mem_space == OCSD_MEM_SPACE_N || mem_space == OCSD_MEM_SPACE_S)) {
>> if (mem_space & OCSD_MEM_SPACE_EL1N) {
>> /* Includes both non secure EL1 and EL0 */
>> - assert(tidq->el == ocsd_EL1 || tidq->el == ocsd_EL0);
>> + assert(el == ocsd_EL1 || el == ocsd_EL0);
>> } else if (mem_space & OCSD_MEM_SPACE_EL2)
>> - assert(tidq->el == ocsd_EL2);
>> + assert(el == ocsd_EL2);
>> else if (mem_space & OCSD_MEM_SPACE_EL3)
>> - assert(tidq->el == ocsd_EL3);
>> + assert(el == ocsd_EL3);
>> }
>>
>> - cpumode = cs_etm__cpu_mode(etmq, address, tidq->el);
>> + cpumode = cs_etm__cpu_mode(etmq, address, el);
>>
>> - if (!thread__find_map(tidq->thread, cpumode, address, &al))
>> + if (!thread__find_map(thread, cpumode, address, &al))
>> goto out;
>>
>> dso = map__dso(al.map);
>> @@ -1138,7 +1131,7 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>>
>> map__load(al.map);
>>
>> - len = dso__data_read_offset(dso, maps__machine(thread__maps(tidq->thread)),
>> + len = dso__data_read_offset(dso, maps__machine(thread__maps(thread)),
>> offset, buffer, size);
>>
>> if (len <= 0) {
>> @@ -1158,6 +1151,30 @@ static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> return ret;
>> }
>>
>> +static u32 cs_etm__frontend_mem_access(struct cs_etm_queue *etmq,
>> + struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet,
>> + u64 address, size_t size, u8 *buffer)
>> +{
>> + return __cs_etm__mem_access(etmq, address, size, buffer, 0, packet->el,
>> + tidq->frontend_thread);
>> +}
>> +
>> +static u32 cs_etm__decoder_mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> + u64 address, size_t size, u8 *buffer,
>> + const ocsd_mem_space_acc_t mem_space)
>> +{
>> + struct cs_etm_traceid_queue *tidq;
>> +
>> + tidq = cs_etm__etmq_get_traceid_queue(etmq, trace_chan_id);
>> + if (!tidq)
>> + return 0;
>> +
>> + return __cs_etm__mem_access(etmq, address, size, buffer,
>> + mem_space, tidq->decode_el,
>> + tidq->decode_thread);
>> +}
>> +
>> static struct cs_etm_queue *cs_etm__alloc_queue(void)
>> {
>> struct cs_etm_queue *etmq = zalloc(sizeof(*etmq));
>> @@ -1333,12 +1350,13 @@ void cs_etm__reset_last_branch_rb(struct cs_etm_traceid_queue *tidq)
>> }
>>
>> static inline int cs_etm__t32_instr_size(struct cs_etm_queue *etmq,
>> - u8 trace_chan_id, u64 addr)
>> + struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet, u64 addr)
>> {
>> u8 instrBytes[2];
>>
>> - cs_etm__mem_access(etmq, trace_chan_id, addr, ARRAY_SIZE(instrBytes),
>> - instrBytes, 0);
>> + cs_etm__frontend_mem_access(etmq, tidq, packet, addr,
>> + ARRAY_SIZE(instrBytes), instrBytes);
>> /*
>> * T32 instruction size is indicated by bits[15:11] of the first
>> * 16-bit word of the instruction: 0b11101, 0b11110 and 0b11111
>> @@ -1371,16 +1389,16 @@ u64 cs_etm__last_executed_instr(const struct cs_etm_packet *packet)
>> }
>>
>> static inline u64 cs_etm__instr_addr(struct cs_etm_queue *etmq,
>> - u64 trace_chan_id,
>> - const struct cs_etm_packet *packet,
>> + struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet,
>> u64 offset)
>> {
>> if (packet->isa == CS_ETM_ISA_T32) {
>> u64 addr = packet->start_addr;
>>
>> while (offset) {
>> - addr += cs_etm__t32_instr_size(etmq,
>> - trace_chan_id, addr);
>> + addr += cs_etm__t32_instr_size(etmq, tidq, packet,
>> + addr);
>> offset--;
>> }
>> return addr;
>> @@ -1490,34 +1508,51 @@ cs_etm__get_trace(struct cs_etm_queue *etmq)
>> return etmq->buf_len;
>> }
>>
>> -static void cs_etm__set_thread(struct cs_etm_queue *etmq,
>> - struct cs_etm_traceid_queue *tidq, pid_t tid,
>> - ocsd_ex_level el)
>> +/*
>> + * Convert a raw thread number to a thread struct and assign it to **thread.
>> + */
>> +static int cs_etm__etmq_update_thread(struct cs_etm_queue *etmq,
>> + ocsd_ex_level el, pid_t tid,
>> + struct thread **thread)
>> {
>> struct machine *machine = cs_etm__get_machine(etmq, el);
>>
>> + if (!machine || !*thread)
>> + return -EINVAL;
>> +
>> if (tid != -1) {
>> - thread__zput(tidq->thread);
>> - tidq->thread = machine__find_thread(machine, -1, tid);
>> + thread__zput(*thread);
>> + *thread = machine__find_thread(machine, -1, tid);
>> }
>>
>> /* Couldn't find a known thread */
>> - if (!tidq->thread)
>> - tidq->thread = machine__idle_thread(machine);
>> + if (!*thread)
>> + *thread = machine__idle_thread(machine);
>>
>> - tidq->el = el;
>> + return 0;
>> }
>>
>> -int cs_etm__etmq_set_tid_el(struct cs_etm_queue *etmq, pid_t tid,
>> - u8 trace_chan_id, ocsd_ex_level el)
>> +/*
>> + * Set the thread and EL of the decode context which is ahead in time of the
>> + * frontend context.
>> + */
>> +int cs_etm__etmq_update_decode_context(struct cs_etm_queue *etmq,
>> + u8 trace_chan_id,
>> + ocsd_ex_level el, pid_t tid)
>> {
>> struct cs_etm_traceid_queue *tidq;
>> + int ret;
>>
>> tidq = cs_etm__etmq_get_traceid_queue(etmq, trace_chan_id);
>> if (!tidq)
>> return -EINVAL;
>>
>> - cs_etm__set_thread(etmq, tidq, tid, el);
>> + ret = cs_etm__etmq_update_thread(etmq, el, tid,
>> + &tidq->decode_thread);
>> + if (ret)
>> + return ret;
>> +
>> + tidq->decode_el = el;
>> return 0;
>> }
>>
>> @@ -1527,8 +1562,8 @@ bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq)
>> }
>>
>> static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
>> - u64 trace_chan_id,
>> - const struct cs_etm_packet *packet,
>> + struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet,
>> struct perf_sample *sample)
>> {
>> /*
>> @@ -1545,14 +1580,14 @@ static void cs_etm__copy_insn(struct cs_etm_queue *etmq,
>> * cs_etm__t32_instr_size().
>> */
>> if (packet->isa == CS_ETM_ISA_T32)
>> - sample->insn_len = cs_etm__t32_instr_size(etmq, trace_chan_id,
>> + sample->insn_len = cs_etm__t32_instr_size(etmq, tidq, packet,
>> sample->ip);
>> /* Otherwise, A64 and A32 instruction size are always 32-bit. */
>> else
>> sample->insn_len = 4;
>>
>> - cs_etm__mem_access(etmq, trace_chan_id, sample->ip, sample->insn_len,
>> - (void *)sample->insn, 0);
>> + cs_etm__frontend_mem_access(etmq, tidq, packet, sample->ip,
>> + sample->insn_len, (void *)sample->insn);
>> }
>>
>> u64 cs_etm__convert_sample_time(struct cs_etm_queue *etmq, u64 cs_timestamp)
>> @@ -1579,6 +1614,7 @@ static inline u64 cs_etm__resolve_sample_time(struct cs_etm_queue *etmq,
>>
>> static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>> struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet,
>> u64 addr, u64 period)
>> {
>> int ret = 0;
>> @@ -1588,15 +1624,15 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>>
>> perf_sample__init(&sample, /*all=*/true);
>> event->sample.header.type = PERF_RECORD_SAMPLE;
>> - event->sample.header.misc = cs_etm__cpu_mode(etmq, addr, tidq->el);
>> + event->sample.header.misc = cs_etm__cpu_mode(etmq, addr, packet->el);
>> event->sample.header.size = sizeof(struct perf_event_header);
>>
>> /* Set time field based on etm auxtrace config. */
>> sample.time = cs_etm__resolve_sample_time(etmq, tidq);
>>
>> sample.ip = addr;
>> - sample.pid = thread__pid(tidq->thread);
>> - sample.tid = thread__tid(tidq->thread);
>> + sample.pid = thread__pid(tidq->frontend_thread);
>> + sample.tid = thread__tid(tidq->frontend_thread);
>> sample.id = etmq->etm->instructions_id;
>> sample.stream_id = etmq->etm->instructions_id;
>> sample.period = period;
>> @@ -1604,7 +1640,7 @@ static int cs_etm__synth_instruction_sample(struct cs_etm_queue *etmq,
>> sample.flags = tidq->prev_packet->flags;
>> sample.cpumode = event->sample.header.misc;
>>
>> - cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->packet, &sample);
>> + cs_etm__copy_insn(etmq, tidq, tidq->packet, &sample);
>>
>> if (etm->synth_opts.last_branch)
>> sample.branch_stack = tidq->last_branch;
>> @@ -1649,15 +1685,15 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq,
>>
>> event->sample.header.type = PERF_RECORD_SAMPLE;
>> event->sample.header.misc = cs_etm__cpu_mode(etmq, ip,
>> - tidq->prev_packet_el);
>> + tidq->prev_packet->el);
>> event->sample.header.size = sizeof(struct perf_event_header);
>>
>> /* Set time field based on etm auxtrace config. */
>> sample.time = cs_etm__resolve_sample_time(etmq, tidq);
>>
>> sample.ip = ip;
>> - sample.pid = thread__pid(tidq->prev_packet_thread);
>> - sample.tid = thread__tid(tidq->prev_packet_thread);
>> + sample.pid = thread__pid(tidq->frontend_thread);
>> + sample.tid = thread__tid(tidq->frontend_thread);
>> sample.addr = cs_etm__first_executed_instr(tidq->packet);
>> sample.id = etmq->etm->branches_id;
>> sample.stream_id = etmq->etm->branches_id;
>> @@ -1666,8 +1702,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq,
>> sample.flags = tidq->prev_packet->flags;
>> sample.cpumode = event->sample.header.misc;
>>
>> - cs_etm__copy_insn(etmq, tidq->trace_chan_id, tidq->prev_packet,
>> - &sample);
>> + cs_etm__copy_insn(etmq, tidq, tidq->prev_packet, &sample);
>>
>> /*
>> * perf report cannot handle events without a branch stack
>> @@ -1788,7 +1823,6 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>> {
>> struct cs_etm_auxtrace *etm = etmq->etm;
>> int ret;
>> - u8 trace_chan_id = tidq->trace_chan_id;
>> u64 instrs_prev;
>>
>> /* Get instructions remainder from previous packet */
>> @@ -1874,10 +1908,10 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>> * been executed, but PC has not advanced to next
>> * instruction)
>> */
>> - addr = cs_etm__instr_addr(etmq, trace_chan_id,
>> - tidq->packet, offset - 1);
>> + addr = cs_etm__instr_addr(etmq, tidq, tidq->packet,
>> + offset - 1);
>> ret = cs_etm__synth_instruction_sample(
>> - etmq, tidq, addr,
>> + etmq, tidq, tidq->packet, addr,
>> etm->instructions_sample_period);
>> if (ret)
>> return ret;
>> @@ -1959,7 +1993,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
>> addr = cs_etm__last_executed_instr(tidq->prev_packet);
>>
>> err = cs_etm__synth_instruction_sample(
>> - etmq, tidq, addr,
>> + etmq, tidq, tidq->prev_packet, addr,
>> tidq->period_instructions);
>> if (err)
>> return err;
>> @@ -2014,7 +2048,7 @@ static int cs_etm__end_block(struct cs_etm_queue *etmq,
>> addr = cs_etm__last_executed_instr(tidq->prev_packet);
>>
>> err = cs_etm__synth_instruction_sample(
>> - etmq, tidq, addr,
>> + etmq, tidq, tidq->prev_packet, addr,
>> tidq->period_instructions);
>> if (err)
>> return err;
>> @@ -2051,9 +2085,9 @@ static int cs_etm__get_data_block(struct cs_etm_queue *etmq)
>> return etmq->buf_len;
>> }
>>
>> -static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> - struct cs_etm_packet *packet,
>> - u64 end_addr)
>> +static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq,
>> + struct cs_etm_traceid_queue *tidq,
>> + struct cs_etm_packet *packet, u64 end_addr)
>> {
>> /* Initialise to keep compiler happy */
>> u16 instr16 = 0;
>> @@ -2075,8 +2109,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> * so below only read 2 bytes as instruction size for T32.
>> */
>> addr = end_addr - 2;
>> - cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr16),
>> - (u8 *)&instr16, 0);
>> + cs_etm__frontend_mem_access(etmq, tidq, packet, addr,
>> + sizeof(instr16), (u8 *)&instr16);
>> if ((instr16 & 0xFF00) == 0xDF00)
>> return true;
>>
>> @@ -2091,8 +2125,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> * +---------+---------+-------------------------+
>> */
>> addr = end_addr - 4;
>> - cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr32),
>> - (u8 *)&instr32, 0);
>> + cs_etm__frontend_mem_access(etmq, tidq, packet, addr,
>> + sizeof(instr32), (u8 *)&instr32);
>> if ((instr32 & 0x0F000000) == 0x0F000000 &&
>> (instr32 & 0xF0000000) != 0xF0000000)
>> return true;
>> @@ -2108,8 +2142,8 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> * +-----------------------+---------+-----------+
>> */
>> addr = end_addr - 4;
>> - cs_etm__mem_access(etmq, trace_chan_id, addr, sizeof(instr32),
>> - (u8 *)&instr32, 0);
>> + cs_etm__frontend_mem_access(etmq, tidq, packet, addr,
>> + sizeof(instr32), (u8 *)&instr32);
>> if ((instr32 & 0xFFE0001F) == 0xd4000001)
>> return true;
>>
>> @@ -2125,7 +2159,6 @@ static bool cs_etm__is_svc_instr(struct cs_etm_queue *etmq, u8 trace_chan_id,
>> static bool cs_etm__is_syscall(struct cs_etm_queue *etmq,
>> struct cs_etm_traceid_queue *tidq, u64 magic)
>> {
>> - u8 trace_chan_id = tidq->trace_chan_id;
>> struct cs_etm_packet *packet = tidq->packet;
>> struct cs_etm_packet *prev_packet = tidq->prev_packet;
>>
>> @@ -2140,7 +2173,7 @@ static bool cs_etm__is_syscall(struct cs_etm_queue *etmq,
>> */
>> if (magic == __perf_cs_etmv4_magic) {
>> if (packet->exception_number == CS_ETMV4_EXC_CALL &&
>> - cs_etm__is_svc_instr(etmq, trace_chan_id, prev_packet,
>> + cs_etm__is_svc_instr(etmq, tidq, prev_packet,
>> prev_packet->end_addr))
>> return true;
>> }
>> @@ -2178,7 +2211,6 @@ static bool cs_etm__is_sync_exception(struct cs_etm_queue *etmq,
>> struct cs_etm_traceid_queue *tidq,
>> u64 magic)
>> {
>> - u8 trace_chan_id = tidq->trace_chan_id;
>> struct cs_etm_packet *packet = tidq->packet;
>> struct cs_etm_packet *prev_packet = tidq->prev_packet;
>>
>> @@ -2204,7 +2236,7 @@ static bool cs_etm__is_sync_exception(struct cs_etm_queue *etmq,
>> * (SMC, HVC) are taken as sync exceptions.
>> */
>> if (packet->exception_number == CS_ETMV4_EXC_CALL &&
>> - !cs_etm__is_svc_instr(etmq, trace_chan_id, prev_packet,
>> + !cs_etm__is_svc_instr(etmq, tidq, prev_packet,
>> prev_packet->end_addr))
>> return true;
>>
>> @@ -2228,7 +2260,6 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq,
>> {
>> struct cs_etm_packet *packet = tidq->packet;
>> struct cs_etm_packet *prev_packet = tidq->prev_packet;
>> - u8 trace_chan_id = tidq->trace_chan_id;
>> u64 magic;
>> int ret;
>>
>> @@ -2309,11 +2340,11 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq,
>> if (prev_packet->flags == (PERF_IP_FLAG_BRANCH |
>> PERF_IP_FLAG_RETURN |
>> PERF_IP_FLAG_INTERRUPT) &&
>> - cs_etm__is_svc_instr(etmq, trace_chan_id,
>> - packet, packet->start_addr))
>> + cs_etm__is_svc_instr(etmq, tidq, packet, packet->start_addr)) {
>> prev_packet->flags = PERF_IP_FLAG_BRANCH |
>> PERF_IP_FLAG_RETURN |
>> PERF_IP_FLAG_SYSCALLRET;
>> + }
>> break;
>> case CS_ETM_DISCONTINUITY:
>> /*
>> @@ -2394,6 +2425,7 @@ static int cs_etm__set_sample_flags(struct cs_etm_queue *etmq,
>> PERF_IP_FLAG_RETURN |
>> PERF_IP_FLAG_INTERRUPT;
>> break;
>> + case CS_ETM_CONTEXT:
>> case CS_ETM_EMPTY:
>> default:
>> break;
>> @@ -2469,6 +2501,19 @@ static int cs_etm__process_traceid_queue(struct cs_etm_queue *etmq,
>> */
>> cs_etm__sample(etmq, tidq);
>> break;
>> + case CS_ETM_CONTEXT:
>> + /*
>> + * Update context but don't swap packet. Keep the
>> + * previous one for branch source address info, if
>> + * tracing the kernel the context packet will be emitted
>> + * between two ranges.
>> + */
>> + ret = cs_etm__etmq_update_thread(etmq, tidq->packet->el,
>> + tidq->packet->tid,
>> + &tidq->frontend_thread);
>> + if (ret)
>> + goto out;
>> + break;
>> case CS_ETM_EXCEPTION:
>> case CS_ETM_EXCEPTION_RET:
>> /*
>> @@ -2497,6 +2542,7 @@ static int cs_etm__process_traceid_queue(struct cs_etm_queue *etmq,
>> }
>> }
>>
>> +out:
>> return ret;
>> }
>>
>> @@ -2620,7 +2666,7 @@ static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm,
>> if (!tidq)
>> continue;
>>
>> - if (tid == -1 || thread__tid(tidq->thread) == tid)
>> + if (tid == -1 || thread__tid(tidq->frontend_thread) == tid)
>> cs_etm__run_per_thread_timeless_decoder(etmq);
>> } else
>> cs_etm__run_per_cpu_timeless_decoder(etmq);
>> @@ -3328,7 +3374,7 @@ static int cs_etm__create_queue_decoders(struct cs_etm_queue *etmq)
>> */
>> if (cs_etm_decoder__add_mem_access_cb(etmq->decoder,
>> 0x0L, ((u64) -1L),
>> - cs_etm__mem_access))
>> + cs_etm__decoder_mem_access))
>> goto out_free_decoder;
>>
>> zfree(&t_params);
>> diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h
>> index aa9bb4a32eca..b81099c2b301 100644
>> --- a/tools/perf/util/cs-etm.h
>> +++ b/tools/perf/util/cs-etm.h
>> @@ -158,6 +158,7 @@ enum cs_etm_sample_type {
>> CS_ETM_DISCONTINUITY,
>> CS_ETM_EXCEPTION,
>> CS_ETM_EXCEPTION_RET,
>> + CS_ETM_CONTEXT,
>> };
>>
>> enum cs_etm_isa {
>> @@ -184,6 +185,8 @@ struct cs_etm_packet {
>> u8 last_instr_size;
>> u8 trace_chan_id;
>> int cpu;
>> + int el;
>> + pid_t tid;
>> };
>>
>> #define CS_ETM_PACKET_MAX_BUFFER 1024
>> @@ -259,8 +262,9 @@ enum cs_etm_pid_fmt {
>> #include <opencsd/ocsd_if_types.h>
>> int cs_etm__get_cpu(struct cs_etm_queue *etmq, u8 trace_chan_id, int *cpu);
>> enum cs_etm_pid_fmt cs_etm__get_pid_fmt(struct cs_etm_queue *etmq);
>> -int cs_etm__etmq_set_tid_el(struct cs_etm_queue *etmq, pid_t tid,
>> - u8 trace_chan_id, ocsd_ex_level el);
>> +int cs_etm__etmq_update_decode_context(struct cs_etm_queue *etmq,
>> + u8 trace_chan_id, ocsd_ex_level el,
>> + pid_t tid);
>> bool cs_etm__etmq_is_timeless(struct cs_etm_queue *etmq);
>> void cs_etm__etmq_set_traceid_queue_timestamp(struct cs_etm_queue *etmq,
>> u8 trace_chan_id);
>>
>> --
>> 2.34.1
>>
^ permalink raw reply
* Re: [PATCH v2 18/18] perf test cs-etm: Move existing tests to coresight folder
From: James Clark @ 2026-06-04 12:22 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Mike Leach, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Amir Ayupov, Jonathan Corbet,
Shuah Khan, Paschalis Mpeis, coresight, linux-perf-users,
linux-kernel, Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <20260603180219.GJ101133@e132581.arm.com>
On 03/06/2026 7:02 pm, Leo Yan wrote:
> On Tue, Jun 02, 2026 at 03:27:00PM +0100, James Clark wrote:
>> There is a subfolder for Coresight tests so might as well keep them all
>> in here.
>
> Now we only have two shell test for CoreSight (I might add one for
That's not right, we have 6. The tests/shell/coresight folder already
existed so it seemed like a mistake to not move these two in there when
it was originally created.
> callchain test), seems to me it is more meaningful to move Arm specific
> tests into a central place (like tools/perf/tests/shell/arm/) for
> easier maintainence.
Maybe, but arm/ and then arm/coresight probably makes sense. There are
some tests that have arm specific blocks in them but are still shared
with other platforms so it could be confusing. I think we can do that
later if we ever end up with a load of arm tests, but right now there
aren't many.
>
> Anyway, this patch is fine as well:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>
^ permalink raw reply
* Re: [PATCH v2 15/18] perf test cs-etm: Make disassembly test use kcore
From: James Clark @ 2026-06-04 12:18 UTC (permalink / raw)
To: Leo Yan
Cc: Suzuki K Poulose, Mike Leach, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Amir Ayupov, Jonathan Corbet,
Shuah Khan, Paschalis Mpeis, coresight, linux-perf-users,
linux-kernel, Arnaldo Carvalho de Melo, linux-doc
In-Reply-To: <20260603173203.GF101133@e132581.arm.com>
On 03/06/2026 6:32 pm, Leo Yan wrote:
> On Tue, Jun 02, 2026 at 03:26:57PM +0100, James Clark wrote:
>> Hits in modules return empty disassembly with vmlinux as an input to
>> objdump. Make the disassembly test more reliable by always using kcore.
>> And update the comments to say that this is supported by the script.
>
> Even without kcore, .ko files should have been saved into buildid
> folder. Before switch to kcore, can we firstly fix the issue that why
> .ko does not work?
>
> Thanks,
> Leo
I think this description of the problem was wrong, the reason for
failures was the alternatives patching mechanism. And the only way to
fix that is by using kcore. I'm not sure what you were thinking of as a fix?
Sashiko commented on V3 that hits in modules would still open the .ko
file and skip kcore anyway. I tried to investigate but I wasn't able to
generate any trace or samples with hits in modules. It might be a
regression but it might have always been like that. I do remember
switching to static when I was tracing Coresight modules before, but
that was to make using address filtering easier, and I inadvertently
avoided this issue.
Seems like fixing this is outside of the scope of this test though. I
didn't see a single failure on N1SDP and the only failures on Orion O6
are because of the wrapping/snapshot bug, and we already knew that you
have to use kcore to get accurate decode. So I'm not sure of a reason to
not do it?
^ permalink raw reply
* Re: [PATCH v19 00/14] crypto/dmaengine: qce: introduce BAM locking and use DMA for register I/O
From: Bartosz Golaszewski @ 2026-06-04 11:50 UTC (permalink / raw)
To: Vinod Koul
Cc: Bartosz Golaszewski, Jonathan Corbet, Thara Gopinath, Herbert Xu,
David S. Miller, Udit Tiwari, Md Sadre Alam, Dmitry Baryshkov,
Manivannan Sadhasivam, Bjorn Andersson, Peter Ujfalusi,
Michal Simek, Frank Li, Andy Gross, Neil Armstrong, dmaengine,
linux-doc, linux-kernel, linux-arm-msm, linux-crypto,
linux-arm-kernel, brgl, Bartosz Golaszewski, Dmitry Baryshkov,
Konrad Dybcio, Stephan Gerhold
In-Reply-To: <aiFScCW_NEY3CsEf@vaman>
On Thu, 4 Jun 2026 12:24:48 +0200, Vinod Koul <vkoul@kernel.org> said:
> On 02-06-26, 18:38, Stephan Gerhold wrote:
>> On Tue, May 26, 2026 at 03:10:48PM +0200, Bartosz Golaszewski wrote:
>> > I feel like I fell into the trap of trying to address pre-existing
>> > issues reported by sashiko and in the process provoking more reports so
>> > let this be the last iteration where I do this. Vinod can we get this
>> > queued for v7.2 now and iron out any previously existing problems in
>> > tree?
>>
>> Thanks a lot for working on fixing all these issues!
>>
>> I agree there is no point addressing all the "pre-existing issues"
>> pointed out by Sashiko, but have you looked through the other comments
>> for new issues pointed out for your patches?
>
> I hope Bart and Qualcomm can fix these driver issues as well
>>
>> Out of curiosity, I was looking a bit at the comments for [PATCH v19
>> 06/14] dmaengine: qcom: bam_dma: add support for BAM locking [1]. There
>> are 8 open comments there (Critical: 1, High: 6 and Medium: 1). From a
>> quick look I would say most of these could be valid. The critical one
>> about the usage of dma_cookie_assign() sounds a bit concerning to me, if
>> it is true we would be basically breaking parts of the dmaengine API for
>> consumers by inserting the lock descriptor in front of everything else.
>
> Yes this seems to be a valid one. Attaching another descriptor for lock
> does not sound right to me, as in this case causes descriptor to be
> marked 'done' prematurely.
>
Yes, I have a fix for this queued.
> Honestly, I am not quite happy with the way lock is being handled here.
> I would hope we can have some better suggestions. Adding a descriptor
> for lock does not look right to me. We are adding odd hardware/firmware
> behaviour on engine apis.
>
> I had earlier suggested to lock always or lock only for hw/sw versions
> supported inside the driver, that might be simplist solution without the
> complexity added here
>
I'm not sure what you mean here. Several iterations ago it was deferred to
consumer drivers. Mani objected and Bjorn and you agreed. I reworked it to move
the locking logic into the DMA driver as requested.
Bart
^ permalink raw reply
* Re: [PATCH] cpufreq: Documentation: fix freq_step description
From: Zhongqiu Han @ 2026-06-04 11:43 UTC (permalink / raw)
To: Pengjie Zhang, rafael, viresh.kumar, corbet, skhan
Cc: linux-pm, linux-doc, linux-kernel, zhanjie9, prime.zeng,
wanghuiqiang, xuwei5, lihuisong, zhenglifeng1, yubowen8,
wangzhi12, zhongqiu.han
In-Reply-To: <2712839c-fb9b-4717-a36f-a0f922115be8@huawei.com>
On 6/1/2026 10:04 AM, Pengjie Zhang wrote:
>
> On 5/30/2026 10:36 PM, Zhongqiu Han wrote:
>> On 5/29/2026 7:11 PM, Pengjie Zhang wrote:
>>> The conservative governor documentation incorrectly states that setting
>>> freq_step to 0 will use the default 5% frequency step. In reality, since
>>> the governor's initial implementation
>>> commit b9170836d1aa ("[CPUFREQ] Conservative cpufreq governer"),
>>> freq_step=0 has always caused the governor to skip frequency updates
>>> entirely.
>>
>> Hi Pengjie,
>>
>> Thanks for the patch.
>>
>> The documentation fix looks correct: in the current code,
>> cs_dbs_update() has an early goto out when freq_step == 0, which skips
>> the call to get_freq_step() and all subsequent frequency change logic.
>>
>> However, the commit message's historical claim appears to be inaccurate.
>> In the original implementation (b9170836d1aa), freq_step=0 had
>> asymmetric behavior: frequency decreases were skipped (early return),
>> but frequency increases still used the hardcoded 5% fallback (freq_step
>> = 5 after the unlikely(freq_step == 0) check).
>>
>> If so, would it make sense to remove/update the historical claim to
>> avoid the incorrect historical claim?
>>
> Thanks for the careful review.
>
> Agreed. The correct commit for the symmetric freq_step=0 behavior
> should be 8e677ce83bf4 ("[CPUFREQ] conservative: fixup governor to
> function more like ondemand logic"), not b9170836d1aa.
>
> I'll fix the commit message in v2.
>
> On a related note, I have a quick question regarding code readability in
> this area. Currently, the code uses the name "freq_step" for two different
> concepts:
>
> 1. `cs_tuners->freq_step`: The tunable exposed via sysfs/documentation,
> which represents a percentage.
> 2. `freq_step = get_freq_step(...)`: The local variable representing the
> actual calculated frequency step (in kHz). The `if
> (unlikely(freq_step == 0))`
> check also applies to this absolute value.
>
> Since mixing a percentage and an absolute kHz value under the same name
> might be slightly confusing for readers, would it make sense to rename the
> local variable (e.g., to `freq_step_khz`) to clearly distinguish the two?
Hi Pengjie,
Agreed, the shadowing is confusing. That said, renaming just the local
variable alone might be a bit too small to warrant a separate patch
-- but while you're looking at this area, you might want to consider
DEF_FREQUENCY_STEP itself as well, which seems to be overloaded:
- In cs_init() it is used as a percentage (the default 5% tunable).
- In get_freq_step() it is assigned to a kHz-valued variable as a
fallback, where "5" means "5 kHz".
As a suggestion, you could fix both while you're at it.
>
> Cheers,
> Pengjie
>
>>>
>>> Correct the documentation to reflect the actual behavior: freq_step=0
>>> disables frequency changes by the governor entirely.
>>>
>>> Fixes: 2a0e49279850 ("cpufreq: User/admin documentation update and
>>> consolidation")
>>> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
>>> ---
>>> Documentation/admin-guide/pm/cpufreq.rst | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/
>>> Documentation/admin-guide/pm/cpufreq.rst
>>> index dbe6d23a5d67..98c724d49047 100644
>>> --- a/Documentation/admin-guide/pm/cpufreq.rst
>>> +++ b/Documentation/admin-guide/pm/cpufreq.rst
>>> @@ -586,8 +586,8 @@ This governor exposes the following tunables:
>>> 100 (5 by default).
>>> This is how much the frequency is allowed to change in one
>>> go. Setting
>>> - it to 0 will cause the default frequency step (5 percent) to be
>>> used
>>> - and setting it to 100 effectively causes the governor to
>>> periodically
>>> + it to 0 disables frequency changes by the governor entirely and
>>> setting
>>> + it to 100 effectively causes the governor to periodically
>>> switch the frequency between the ``scaling_min_freq`` and
>>> ``scaling_max_freq`` policy limits.
>>
>>
--
Thx and BRs,
Zhongqiu Han
^ permalink raw reply
* Re: [PATCH mm-unstable v18 06/14] mm/khugepaged: generalize collapse_huge_page for mTHP collapse
From: Lorenzo Stoakes @ 2026-06-04 11:38 UTC (permalink / raw)
To: Nico Pache
Cc: linux-doc, linux-kernel, linux-mm, linux-trace-kernel, aarcange,
akpm, anshuman.khandual, apopple, baohua, baolin.wang, byungchul,
catalin.marinas, cl, corbet, dave.hansen, david, dev.jain, gourry,
hannes, hughd, jack, jackmanb, jannh, jglisse, joshua.hahnjy, kas,
lance.yang, liam, mathieu.desnoyers, matthew.brost, mhiramat,
mhocko, peterx, pfalcato, rakie.kim, raquini, rdunlap,
richard.weiyang, rientjes, rostedt, rppt, ryan.roberts, shivankg,
sunnanyong, surenb, thomas.hellstrom, tiwai, usamaarif642, vbabka,
vishal.moola, wangkefeng.wang, will, willy, yang, ying.huang, ziy,
zokeefe, Usama Arif
In-Reply-To: <20260522150009.121603-7-npache@redhat.com>
I will go review the thread about the cache maintenance separately and
respond about that.
On Fri, May 22, 2026 at 09:00:01AM -0600, Nico Pache wrote:
> Pass an order and offset to collapse_huge_page to support collapsing anon
> memory to arbitrary orders within a PMD. order indicates what mTHP size we
> are attempting to collapse to, and offset indicates were in the PMD to
> start the collapse attempt.
>
> For non-PMD collapse we must leave the anon VMA write locked until after
> we collapse the mTHP-- in the PMD case all the pages are isolated, but in
> the mTHP case this is not true, and we must keep the lock to prevent
> access/changes to the page tables. This can happen if the rmap walkers hit
> a pmd_none while the PMD entry is currently unavailable due to being
> temporarily removed during the collapse phase.
>
> Acked-by: Usama Arif <usama.arif@linux.dev>
> Signed-off-by: Nico Pache <npache@redhat.com>
The logic LGTM generally, some questions for understanding below, and of
course as per above I want to review the Lance/David subthread.
Thanks!
> ---
> mm/khugepaged.c | 93 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 38 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index fab35d318641..d64f42f66236 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1214,34 +1214,36 @@ static enum scan_result alloc_charge_folio(struct folio **foliop, struct mm_stru
> * while allocating a THP, as that could trigger direct reclaim/compaction.
> * Note that the VMA must be rechecked after grabbing the mmap_lock again.
> */
> -static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long address,
> - int referenced, int unmapped, struct collapse_control *cc)
> +static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long start_addr,
> + int referenced, int unmapped, struct collapse_control *cc,
> + unsigned int order)
> {
> + const unsigned long pmd_addr = start_addr & HPAGE_PMD_MASK;
> + const unsigned long end_addr = start_addr + (PAGE_SIZE << order);
> LIST_HEAD(compound_pagelist);
> pmd_t *pmd, _pmd;
> - pte_t *pte;
> + pte_t *pte = NULL;
As mentioned elsewhere for some reason this was dropped in
mm-unstable. Maybe a bad conflict resolution?
> pgtable_t pgtable;
> struct folio *folio;
> spinlock_t *pmd_ptl, *pte_ptl;
> enum scan_result result = SCAN_FAIL;
> struct vm_area_struct *vma;
> struct mmu_notifier_range range;
> + bool anon_vma_locked = false;
>
> - VM_BUG_ON(address & ~HPAGE_PMD_MASK);
> -
> - result = alloc_charge_folio(&folio, mm, cc, HPAGE_PMD_ORDER);
> + result = alloc_charge_folio(&folio, mm, cc, order);
> if (result != SCAN_SUCCEED)
> goto out_nolock;
>
> mmap_read_lock(mm);
> - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> - HPAGE_PMD_ORDER);
> + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> + &vma, cc, order);
> if (result != SCAN_SUCCEED) {
> mmap_read_unlock(mm);
> goto out_nolock;
> }
>
> - result = find_pmd_or_thp_or_none(mm, address, &pmd);
> + result = find_pmd_or_thp_or_none(mm, pmd_addr, &pmd);
> if (result != SCAN_SUCCEED) {
> mmap_read_unlock(mm);
> goto out_nolock;
> @@ -1253,8 +1255,8 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> * released when it fails. So we jump out_nolock directly in
> * that case. Continuing to collapse causes inconsistency.
> */
> - result = __collapse_huge_page_swapin(mm, vma, address, pmd,
> - referenced, HPAGE_PMD_ORDER);
> + result = __collapse_huge_page_swapin(mm, vma, start_addr, pmd,
> + referenced, order);
> if (result != SCAN_SUCCEED)
> goto out_nolock;
> }
> @@ -1269,20 +1271,21 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> * mmap_lock.
> */
> mmap_write_lock(mm);
> - result = hugepage_vma_revalidate(mm, address, true, &vma, cc,
> - HPAGE_PMD_ORDER);
> + result = hugepage_vma_revalidate(mm, pmd_addr, /*expect_anon=*/ true,
> + &vma, cc, order);
> if (result != SCAN_SUCCEED)
> goto out_up_write;
> /* check if the pmd is still valid */
> vma_start_write(vma);
> - result = check_pmd_still_valid(mm, address, pmd);
> + result = check_pmd_still_valid(mm, pmd_addr, pmd);
> if (result != SCAN_SUCCEED)
> goto out_up_write;
>
> anon_vma_lock_write(vma->anon_vma);
> + anon_vma_locked = true;
I worry that we hold this lock a lot longer now? Maybe the algorithmic
change alters that, but Claude did suggest on the s390 bug that longer lock
hold might be an issue.
I wonder if we'll observe lock contention as a result?
Correct me if I'm wrong and we're not holding longer than previously,
however. Just appears that we do.
>
> - mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, address,
> - address + HPAGE_PMD_SIZE);
> + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, start_addr,
> + end_addr);
> mmu_notifier_invalidate_range_start(&range);
>
> pmd_ptl = pmd_lock(mm, pmd); /* probably unnecessary */
> @@ -1294,26 +1297,23 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> * Parallel GUP-fast is fine since GUP-fast will back off when
> * it detects PMD is changed.
> */
> - _pmd = pmdp_collapse_flush(vma, address, pmd);
> + _pmd = pmdp_collapse_flush(vma, pmd_addr, pmd);
> spin_unlock(pmd_ptl);
> mmu_notifier_invalidate_range_end(&range);
> tlb_remove_table_sync_one();
>
> - pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl);
> + pte = pte_offset_map_lock(mm, &_pmd, start_addr, &pte_ptl);
> if (pte) {
> - result = __collapse_huge_page_isolate(vma, address, pte, cc,
> - HPAGE_PMD_ORDER,
> - &compound_pagelist);
> + result = __collapse_huge_page_isolate(vma, start_addr, pte, cc,
> + order, &compound_pagelist);
> spin_unlock(pte_ptl);
> } else {
> result = SCAN_NO_PTE_TABLE;
> }
>
> if (unlikely(result != SCAN_SUCCEED)) {
> - if (pte)
> - pte_unmap(pte);
OK I seem to remember this is because we're holding the anon_vma lock
longer. That does imply that on e.g. x86-64 the RCU lock is being held a
bit longer also as well as the anon_vma loc.
I guess it's also because we need to hold anon_vma and pte lock because
we're fiddling around at PTE level for mTHP not just PMD level as 'classic'
THP did.
(Rememberings going on here :)
> spin_lock(pmd_ptl);
> - BUG_ON(!pmd_none(*pmd));
> + WARN_ON_ONCE(!pmd_none(*pmd));
> /*
> * We can only use set_pmd_at when establishing
> * hugepmds and never for establishing regular pmds that
> @@ -1321,21 +1321,24 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> */
> pmd_populate(mm, pmd, pmd_pgtable(_pmd));
> spin_unlock(pmd_ptl);
> - anon_vma_unlock_write(vma->anon_vma);
> goto out_up_write;
> }
>
> /*
> - * All pages are isolated and locked so anon_vma rmap
> - * can't run anymore.
> + * For PMD collapse all pages are isolated and locked so anon_vma
> + * rmap can't run anymore. For mTHP collapse the PMD entry has been
> + * removed and not all pages are isolated and locked, so we must hold
Right because some PTE entries be unaffected by the change.
> + * the lock to prevent neighboring folios from attempting to access
> + * this PMD until its reinstalled.
OK. This is slightly annoying for my CoW context work as it means there's
another case where we need to explicitly hold an anon_vma lock for
correctness :)
Anyway I will think about that separately, is what it is. And in fact
motivates to want this merged earlier so I can work against it :)
> */
> - anon_vma_unlock_write(vma->anon_vma);
> + if (is_pmd_order(order)) {
> + anon_vma_unlock_write(vma->anon_vma);
> + anon_vma_locked = false;
> + }
>
> result = __collapse_huge_page_copy(pte, folio, pmd, _pmd,
> - vma, address, pte_ptl,
> - HPAGE_PMD_ORDER,
> - &compound_pagelist);
> - pte_unmap(pte);
> + vma, start_addr, pte_ptl,
> + order, &compound_pagelist);
> if (unlikely(result != SCAN_SUCCEED))
> goto out_up_write;
>
> @@ -1345,18 +1348,32 @@ static enum scan_result collapse_huge_page(struct mm_struct *mm, unsigned long a
> * write.
> */
> __folio_mark_uptodate(folio);
> - pgtable = pmd_pgtable(_pmd);
> -
> spin_lock(pmd_ptl);
> - BUG_ON(!pmd_none(*pmd));
> - pgtable_trans_huge_deposit(mm, pmd, pgtable);
> - map_anon_folio_pmd_nopf(folio, pmd, vma, address);
> + WARN_ON_ONCE(!pmd_none(*pmd));
> + if (is_pmd_order(order)) {
> + pgtable = pmd_pgtable(_pmd);
> + pgtable_trans_huge_deposit(mm, pmd, pgtable);
> + map_anon_folio_pmd_nopf(folio, pmd, vma, pmd_addr);
> + } else {
> + /*
> + * set_ptes is called in map_anon_folio_pte_nopf with the
> + * pmd_ptl lock still held; this is safe as the PMD is expected
PMD entry you mean?
> + * to be none. The pmd entry is then repopulated below.
> + */
> + map_anon_folio_pte_nopf(folio, pte, vma, start_addr, /*uffd_wp=*/ false);
So here we populate entries in the existing PTE _table_ to point at the new
order>0 folio? With arm64 of course doing transparent contpte stuff?
> + smp_wmb(); /* make PTEs visible before PMD. See pmd_install() */
> + pmd_populate(mm, pmd, pmd_pgtable(_pmd));
And then we reinstall the pre-existing PMD _entry_ from none -> what it was
before?
> + }
> spin_unlock(pmd_ptl);
>
> folio = NULL;
>
> result = SCAN_SUCCEED;
> out_up_write:
> + if (anon_vma_locked)
> + anon_vma_unlock_write(vma->anon_vma);
> + if (pte)
> + pte_unmap(pte);
> mmap_write_unlock(mm);
> out_nolock:
> if (folio)
> @@ -1536,7 +1553,7 @@ static enum scan_result collapse_scan_pmd(struct mm_struct *mm,
> /* collapse_huge_page expects the lock to be dropped before calling */
> mmap_read_unlock(mm);
> result = collapse_huge_page(mm, start_addr, referenced,
> - unmapped, cc);
> + unmapped, cc, HPAGE_PMD_ORDER);
> /* collapse_huge_page will return with the mmap_lock released */
> *lock_dropped = true;
> }
> --
> 2.54.0
>
^ permalink raw reply
* Re: configurable block error injection
From: Daniel Gomez @ 2026-06-04 11:06 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Jonathan Corbet, linux-block, linux-doc, bpf, linux-kselftest,
Luis Chamberlain, Masami Hiramatsu, Brendan Gregg, GOST,
Shin'ichiro Kawasaki
In-Reply-To: <20260602150503.GA6887@lst.de>
On 02/06/2026 17.05, Christoph Hellwig wrote:
> On Tue, Jun 02, 2026 at 11:58:25AM +0200, Daniel Gomez wrote:
>> I wonder if the block layer would be interested in moving block error
>> injection off the should_fail() fault injection framework and extending
>> the ALLOW_ERROR_INJECTION annotation instead and offloading all the
>> debugfs configuration logic (block/error-injection.c) into eBPF?
>
> I've looked into plain ALLOW_ERROR_INJECTION-based injection and it
> is not very useful. I didn't even now eBPF could use it,
For context: Josef Bacik introduced it first for BPF only
(BPF_ALLOW_ERROR_INJECTION). Masami Hiramatsu then generalized it into
the error injection framework, renaming it to ALLOW_ERROR_INJECTION
and adding the fail_function debugfs interface (which calls
should_fail()). So annotating a function with ALLOW_ERROR_INJECTION
gives you both backends at once: debugfs (fail_function) and eBPF
(bpf_override_return()).
> but I
> looked into other eBPF injections and at least for my uses cases
> it was a bit of a mess.
Agreed, and I had not looked closely enough at the series
before proposing the wrong primitives. ALLOW_ERROR_INJECTION /
bpf_override_return() are not sufficient here: bpf_override_return()
cannot set bio->bi_status or call bio_endio(), which I think is the
key operation here.
IIU the series correctly, and oversimplifying: when
injection is enabled and a bio matches, the block layer completes the
bio inline with the chosen blk_status_t (the status= rule from debugfs)
via bio_endio_status(). The submission path returns to the caller
immediately, with the bio already in the error state. Nothing is ever
sent to the device, but the completion path sees the injected error.
submit_bio() -> submit_bio_noacct() -> submit_bio_noacct_nocheck()
+-- Path 1: no match -> continue normal IO submission
+-- Path 2: match (diskN) -> blk_error_inject()
-> bio_endio_status(bio, inj->status)
-> bio_endio()
// error injected. bio completed
So this is bio mutating + a bio_endio() call, not a return override.
That can't be solved with ALLOW_ERROR_INJECTION. But we can use
BPF_PROG_TYPE_STRUCT_OPS instead: the kernel keeps ownership of the
bio_endio_status() call, and the eBPF program only drives the policy,
ie. which blk_status_t to return for a given bio, based on whatever
heuristics it implements.
> I'd have to allow access to certain bio
With struct_ops the bio is passed to the ebpf side as read-only, bio
fields can be read to decide the policy but cannot write them. Is
read-only access to bio fields itself a concern?
> fields and would have create a stable UAPI for commands and status
> using the fake BTF struct access which really would not be a good
> idea here as we need to be able to change internals.
That should not be a problem at all. With CO-RE (compile once, run
everywhere) the program resolves the bio field offsets against the
BTF of the kernel it loads on, so it adapts dynamically if the layout
changes. The contract is just the struct_ops callback signature: a
struct bio * argument and a blk_status_t return. And that doesn't imply
any UAPI commitment AFAIK.
> Additionally
> having fully BTF-enabled toolchains in test VMs is not great either.
Are you referring to the old BCC toolchain requirements [1]? This is
solved in CO-RE [2]. The toolchain (Clang/LLVM, pahole) stays on the
build host; the test VM only needs the prebuilt BPF object, libbpf at
runtime, and the kernel's own BTF (CONFIG_DEBUG_INFO_BTF). No compiler
or BTF toolchain is required inside the VM. Clang/LLVM 10+ is enough to
build CO-RE libbpf tools [3].
Link: https://ebpf.io/what-is-ebpf/#how-are-ebpf-programs-written [1]
Link: https://nakryiko.com/posts/bpf-portability-and-co-re/ [2]
Link: https://github.com/libbpf/libbpf#bpf-co-re-compile-once--run-everywhere [1]
>
> I've also not actually found any good map type for range lookups,
> which is kinda essential here.
Are you referring to the bio sector range comparison in
__blk_error_inject()? I don't think that needs to be delegated to a
BPF map (Documentation/bpf/maps.rst). The ebpf side has direct access
to the bio fields, so it can apply the same sector/op filtering
__blk_error_inject() does today. That match is already a linear list
walk, so the ebpf program just runs the same [start, end] condition
check.
In summary: what do you think of evolving this series
into eBPF, but BPF_PROG_TYPE_STRUCT_OPS instead of
ALLOW_ERROR_INJECTION/bpf_override_return()?. The configurable debugfs
injection is a nice enhancement over what we had, but the matching is
static (op/start/nr_sectors/status/chance) per gendisk. A struct_ops
hook would expand the failure model to anything derivable from the bio
(plus any state the program keeps), with the kernel still owning the
bio_endio_status() mutation and only the policy moving out of the tree.
The benefit is the flexibility to express the policy without hard-coding
the model in the kernel.
The open question is whether programmable injection is something we want
to support in-tree, or whether more debugfs knobs suffice. Thoughts?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox