On Wed, Apr 05, 2023 at 11:01:31AM -0700, ankita@xxxxxxxxxx wrote:
> From: Ankit Agrawal <ankita@xxxxxxxxxx>
>
> The kernel MM does not currently handle ECC errors / poison on a memory
> region that is not backed by struct pages. In this series, mapping request
> from QEMU to the device memory is executed using remap_pfn_range().
> Hence added a new mechanism to handle memory failure on such memory.
>
> Make kernel MM expose a function to allow modules managing the device
> memory to register a failure function and the address space that is
> associated with the device memory. MM maintains this information as
> interval tree. The registered memory failure function is used by MM to
> notify the module of the PFN, so that the module may take any required
> action. The module for example may use the information to track the
> poisoned pages.
>
> In this implementation, kernel MM follows the following sequence (mostly)
> similar to the memory_failure() handler for struct page backed memory:
> 1. memory_failure() is triggered on reception of a poison error. An
> absence of struct page is detected and consequently memory_failure_pfn
> is executed.
> 2. memory_failure_pfn() call the newly introduced failure handler exposed
> by the module managing the poisoned memory to notify it of the problematic
> PFN.
> 3. memory_failure_pfn() unmaps the stage-2 mapping to the PFN.
> 4. memory_failure_pfn() collects the processes mapped to the PFN.
> 5. memory_failure_pfn() sends SIGBUS (BUS_MCEERR_AO) to all the processes
> mapping the faulty PFN using kill_procs().
> 6. An access to the faulty PFN by an operation in VM at a later point of
> time is trapped and user_mem_abort() is called.
> 7. user_mem_abort() calls __gfn_to_pfn_memslot() on the PFN, and the
> following execution path is followed: __gfn_to_pfn_memslot() ->
> hva_to_pfn() -> hva_to_pfn_remapped() -> fixup_user_fault() ->
> handle_mm_fault() -> handle_pte_fault() -> do_fault(). do_fault() is
> expected to return VM_FAULT_HWPOISON on the PFN (it currently does not
> and is fixed as part of another patch in the series).
> 8. __gfn_to_pfn_memslot() then returns KVM_PFN_ERR_HWPOISON, which cause
> the poison with SIGBUS (BUS_MCEERR_AR) to be sent to the QEMU process
> through kvm_send_hwpoison_signal().
>
> Signed-off-by: Ankit Agrawal <ankita@xxxxxxxxxx>
> ---
> include/linux/memory-failure.h | 22 +++++
> include/linux/mm.h | 1 +
> include/ras/ras_event.h | 1 +
> mm/memory-failure.c | 148 +++++++++++++++++++++++++++++----
> 4 files changed, 154 insertions(+), 18 deletions(-)
> create mode 100644 include/linux/memory-failure.h
>
> diff --git a/include/linux/memory-failure.h b/include/linux/memory-failure.h
> new file mode 100644
> index 000000000000..9a579960972a
> --- /dev/null
> +++ b/include/linux/memory-failure.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_MEMORY_FAILURE_H
> +#define _LINUX_MEMORY_FAILURE_H
> +
> +#include <linux/interval_tree.h>
> +
> +struct pfn_address_space;
> +
> +struct pfn_address_space_ops {
> + void (*failure)(struct pfn_address_space *pfn_space, unsigned long pfn);
> +};
> +
> +struct pfn_address_space {
> + struct interval_tree_node node;
> + const struct pfn_address_space_ops *ops;
> + struct address_space *mapping;
> +};
> +
> +int register_pfn_address_space(struct pfn_address_space *pfn_space);
> +void unregister_pfn_address_space(struct pfn_address_space *pfn_space);
> +
> +#endif /* _LINUX_MEMORY_FAILURE_H */
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 1f79667824eb..e3ef52d3d45a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3530,6 +3530,7 @@ enum mf_action_page_type {
> MF_MSG_BUDDY,
> MF_MSG_DAX,
> MF_MSG_UNSPLIT_THP,
> + MF_MSG_PFN,
Personally, the keyword "PFN" looks to me a little too generic, so I prefer
"PFNMAP" or "PFN_MAP" because memory_failure() is anyway called with pfn
regardless of being backed by struct page.
> MF_MSG_UNKNOWN,
> };
>
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index cbd3ddd7c33d..5c62a4d17172 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -373,6 +373,7 @@ TRACE_EVENT(aer_event,
> EM ( MF_MSG_BUDDY, "free buddy page" ) \
> EM ( MF_MSG_DAX, "dax page" ) \
> EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" ) \
> + EM ( MF_MSG_PFN, "non struct page pfn" ) \
> EMe ( MF_MSG_UNKNOWN, "unknown page" )
>
> /*
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index fae9baf3be16..2c1a2ec42f7b 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -38,6 +38,7 @@
>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> +#include <linux/memory-failure.h>
> #include <linux/page-flags.h>
> #include <linux/kernel-page-flags.h>
> #include <linux/sched/signal.h>
> @@ -62,6 +63,7 @@
> #include <linux/page-isolation.h>
> #include <linux/pagewalk.h>
> #include <linux/shmem_fs.h>
> +#include <linux/pfn_t.h>
> #include "swap.h"
> #include "internal.h"
> #include "ras/ras_event.h"
> @@ -122,6 +124,10 @@ const struct attribute_group memory_failure_attr_group = {
> .attrs = memory_failure_attr,
> };
>
> +static struct rb_root_cached pfn_space_itree = RB_ROOT_CACHED;
> +
> +static DEFINE_MUTEX(pfn_space_lock);
> +
> /*
> * Return values:
> * 1: the page is dissolved (if needed) and taken off from buddy,
> @@ -399,15 +405,14 @@ static unsigned long dev_pagemap_mapping_shift(struct vm_area_struct *vma,
> * Schedule a process for later kill.
> * Uses GFP_ATOMIC allocations to avoid potential recursions in the VM.
> *
> - * Note: @fsdax_pgoff is used only when @p is a fsdax page and a
> - * filesystem with a memory failure handler has claimed the
> - * memory_failure event. In all other cases, page->index and
> - * page->mapping are sufficient for mapping the page back to its
> + * Notice: @pgoff is used either when @p is a fsdax page or a PFN is not
> + * backed by struct page and a filesystem with a memory failure handler
> + * has claimed the memory_failure event. In all other cases, page->index
You add a new case using @pgoff, and now we have 3 such cases, so could you
update the comment to itemize them (which makes it easier to read and update)?
> + * and page->mapping are sufficient for mapping the page back to its
> * corresponding user virtual address.
> */
> -static void add_to_kill(struct task_struct *tsk, struct page *p,
> - pgoff_t fsdax_pgoff, struct vm_area_struct *vma,
> - struct list_head *to_kill)
> +static void add_to_kill(struct task_struct *tsk, struct page *p, pgoff_t pgoff,
> + struct vm_area_struct *vma, struct list_head *to_kill)
> {
> struct to_kill *tk;
>
> @@ -417,13 +422,20 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,
> return;
> }
>
> - tk->addr = page_address_in_vma(p, vma);
> - if (is_zone_device_page(p)) {
> - if (fsdax_pgoff != FSDAX_INVALID_PGOFF)
> - tk->addr = vma_pgoff_address(fsdax_pgoff, 1, vma);
> + if (vma->vm_flags | PFN_MAP) {
> + tk->addr =
> + vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> + tk->size_shift = PAGE_SHIFT;
> + } else if (is_zone_device_page(p)) {
> + if (pgoff != FSDAX_INVALID_PGOFF)
> + tk->addr = vma_pgoff_address(pgoff, 1, vma);
> + else
> + tk->addr = page_address_in_vma(p, vma);
> tk->size_shift = dev_pagemap_mapping_shift(vma, tk->addr);
> - } else
> + } else {
> + tk->addr = page_address_in_vma(p, vma);
> tk->size_shift = page_shift(compound_head(p));
> + }
>
> /*
> * Send SIGKILL if "tk->addr == -EFAULT". Also, as
> @@ -617,13 +629,12 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
> i_mmap_unlock_read(mapping);
> }
>
> -#ifdef CONFIG_FS_DAX
It seems that your new driver is built only in limited configuration/architecture,
so loosening the condition instead of simply removing might be better.
> /*
> * Collect processes when the error hit a fsdax page.
> */
> -static void collect_procs_fsdax(struct page *page,
> - struct address_space *mapping, pgoff_t pgoff,
> - struct list_head *to_kill)
> +static void collect_procs_pgoff(struct page *page,
> + struct address_space *mapping, pgoff_t pgoff,
> + struct list_head *to_kill)
> {
> struct vm_area_struct *vma;
> struct task_struct *tsk;
> @@ -643,7 +654,6 @@ static void collect_procs_fsdax(struct page *page,
> read_unlock(&tasklist_lock);
> i_mmap_unlock_read(mapping);
> }
> -#endif /* CONFIG_FS_DAX */
>
> /*
> * Collect the processes who have the corrupted page mapped to kill.
> @@ -835,6 +845,7 @@ static const char * const action_page_types[] = {
> [MF_MSG_BUDDY] = "free buddy page",
> [MF_MSG_DAX] = "dax page",
> [MF_MSG_UNSPLIT_THP] = "unsplit thp",
> + [MF_MSG_PFN] = "non struct page pfn",
> [MF_MSG_UNKNOWN] = "unknown page",
> };
>
> @@ -1745,7 +1756,7 @@ int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
>
> SetPageHWPoison(page);
>
> - collect_procs_fsdax(page, mapping, index, &to_kill);
> + collect_procs_pgoff(page, mapping, index, &to_kill);
> unmap_and_kill(&to_kill, page_to_pfn(page), mapping,
> index, mf_flags);
> unlock:
> @@ -2052,6 +2063,99 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
> return rc;
> }
>
> +/**
> + * register_pfn_address_space - Register PA region for poison notification.
> + * @pfn_space: structure containing region range and callback function on
> + * poison detection.
> + *
> + * This function is called by a kernel module to register a PA region and
> + * a callback function with the kernel. On detection of poison, the
> + * kernel code will go through all registered regions and call the
> + * appropriate callback function associated with the range. The kernel
> + * module is responsible for tracking the poisoned pages.
> + *
> + * Return: 0 if successfully registered,
> + * -EBUSY if the region is already registered
> + */
> +int register_pfn_address_space(struct pfn_address_space *pfn_space)
> +{
> + if (!request_mem_region(pfn_space->node.start << PAGE_SHIFT,
> + (pfn_space->node.last - pfn_space->node.start + 1) << PAGE_SHIFT, ""))
> + return -EBUSY;
> +
> + mutex_lock(&pfn_space_lock);
> + interval_tree_insert(&pfn_space->node, &pfn_space_itree);
> + mutex_unlock(&pfn_space_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_pfn_address_space);
register_pfn_address_space and unregister_pfn_address_space are not compiled if
CONFIG_MEMORY_FAILURE is not set, so maybe your new driver might need to depend
on this config.
> +
> +/**
> + * unregister_pfn_address_space - Unregister a PA region from poison
> + * notification.
> + * @pfn_space: structure containing region range to be unregistered.
> + *
> + * This function is called by a kernel module to unregister the PA region
> + * from the kernel from poison tracking.
> + */
> +void unregister_pfn_address_space(struct pfn_address_space *pfn_space)
> +{
> + mutex_lock(&pfn_space_lock);
> + interval_tree_remove(&pfn_space->node, &pfn_space_itree);
> + mutex_unlock(&pfn_space_lock);
> + release_mem_region(pfn_space->node.start << PAGE_SHIFT,
> + (pfn_space->node.last - pfn_space->node.start + 1) << PAGE_SHIFT);
> +}
> +EXPORT_SYMBOL_GPL(unregister_pfn_address_space);
> +
> +static int memory_failure_pfn(unsigned long pfn, int flags)
> +{
> + struct interval_tree_node *node;
> + int rc = -EBUSY;
> + LIST_HEAD(tokill);
> +
> + mutex_lock(&pfn_space_lock);
> + /*
> + * Modules registers with MM the address space mapping to the device memory they
> + * manage. Iterate to identify exactly which address space has mapped to this
> + * failing PFN.
> + */
> + for (node = interval_tree_iter_first(&pfn_space_itree, pfn, pfn); node;
> + node = interval_tree_iter_next(node, pfn, pfn)) {
> + struct pfn_address_space *pfn_space =
> + container_of(node, struct pfn_address_space, node);
> + rc = 0;
> +
> + /*
> + * Modules managing the device memory needs to be conveyed about the
> + * memory failure so that the poisoned PFN can be tracked.
> + */
> + pfn_space->ops->failure(pfn_space, pfn);
> +
> + collect_procs_pgoff(NULL, pfn_space->mapping, pfn, &tokill);
> +
> + unmap_mapping_range(pfn_space->mapping, pfn << PAGE_SHIFT,
> + PAGE_SIZE, 0);
> + }
> + mutex_unlock(&pfn_space_lock);
If rc == 0 at this point, the given pfn seems to be outside the GPU memory,
so that should be considered as "Invalid address" case whose check is removed
by patch 5/6. So it might be better to sperate the case from "do handling
for non struct page pfn" case.
> +
> + /*
> + * Unlike System-RAM there is no possibility to swap in a different
> + * physical page at a given virtual address, so all userspace
> + * consumption of direct PFN memory necessitates SIGBUS (i.e.
> + * MF_MUST_KILL)
> + */
> + flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;
> + kill_procs(&tokill, true, false, pfn, flags);
> +
> + pr_err("%#lx: recovery action for %s: %s\n",
> + pfn, action_page_types[MF_MSG_PFN],
> + action_name[rc ? MF_FAILED : MF_RECOVERED]);
Could you call action_result() to print out the summary line.
It has some other common things like accounting and tracepoint.
> +
> + return rc;
> +}
> +
> static DEFINE_MUTEX(mf_mutex);
>
> /**
> @@ -2093,6 +2197,11 @@ int memory_failure(unsigned long pfn, int flags)
> if (!(flags & MF_SW_SIMULATED))
> hw_memory_failure = true;
>
> + if (!pfn_valid(pfn) && !arch_is_platform_page(PFN_PHYS(pfn))) {
> + res = memory_failure_pfn(pfn, flags);
> + goto unlock_mutex;
> + }
This might break exisiting corner case about DAX device, so maybe this should
be checked after confirming that pfn_to_online_page returns NULL.
> +
> p = pfn_to_online_page(pfn);
> if (!p) {
> res = arch_memory_failure(pfn, flags);
> @@ -2106,6 +2215,9 @@ int memory_failure(unsigned long pfn, int flags)
> pgmap);
> goto unlock_mutex;
> }
> +
> + res = memory_failure_pfn(pfn, flags);
> + goto unlock_mutex;
This path is chosen when pfn_valid returns true, which cannot happen for GPU
memory's case?
Thanks,
Naoya Horiguchi
> }
> pr_err("%#lx: memory outside kernel control\n", pfn);
> res = -ENXIO;
> --
> 2.17.1
