Re: [PATCH v8 1/2] mm/khugepaged: recover from poisoned anonymous memory

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 30 Nov 2022 16:59:30 -0800 Jiaqi Yan <jiaqiyan@google.com> wrote:

> Make __collapse_huge_page_copy return whether copying anonymous pages
> succeeded, and make collapse_huge_page handle the return status.
> 
> Break existing PTE scan loop into two for-loops. The first loop copies
> source pages into target huge page, and can fail gracefully when running
> into memory errors in source pages. If copying all pages succeeds, the
> second loop releases and clears up these normal pages. Otherwise, the
> second loop rolls back the page table and page states by:
> - re-establishing the original PTEs-to-PMD connection.
> - releasing source pages back to their LRU list.
> 
> Tested manually:
> 0. Enable khugepaged on system under test.
> 1. Start a two-thread application. Each thread allocates a chunk of
>    non-huge anonymous memory buffer.
> 2. Pick 4 random buffer locations (2 in each thread) and inject
>    uncorrectable memory errors at corresponding physical addresses.
> 3. Signal both threads to make their memory buffer collapsible, i.e.
>    calling madvise(MADV_HUGEPAGE).
> 4. Wait and check kernel log: khugepaged is able to recover from poisoned
>    pages and skips collapsing them.
> 5. Signal both threads to inspect their buffer contents and make sure no
>    data corruption.

Looks like a nice patchset.  I'd like to give it a run in linux-next
but we're at -rc7 and we have no review/ack tags.  So it should be a
post-6.2-rc1 thing.

I have a quibble.

> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -361,6 +361,27 @@ static inline void copy_highpage(struct page *to, struct page *from)
>  
>  #endif
>  
> +/*
> + * Machine check exception handled version of copy_highpage. Return number
> + * of bytes not copied if there was an exception; otherwise 0 for success.
> + * Note handling #MC requires arch opt-in.
> + */
> +static inline int copy_mc_highpage(struct page *to, struct page *from)
> +{
> +	char *vfrom, *vto;
> +	unsigned long ret;
> +
> +	vfrom = kmap_local_page(from);
> +	vto = kmap_local_page(to);
> +	ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
> +	if (ret == 0)
> +		kmsan_copy_page_meta(to, from);
> +	kunmap_local(vto);
> +	kunmap_local(vfrom);
> +
> +	return ret;
> +}

Why inlined?  It's large, it's slow, it's called only from
khugepaged.c.  A regular out-of-line function which is static to
khugepaged.c seems more appropriate?