linux-kernel - Re: [PATCH 2/2] mm/khugepaged: return EAGAIN for transient dirty pages in MADV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67325651-b7d5-40d0-a9f1-3009b61c8d3a@lucifer.local>
Date: Mon, 10 Nov 2025 11:56:18 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Shivank Garg <shivankg@....com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>, Zi Yan <ziy@...dia.com>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
        Dev Jain <dev.jain@....com>, Barry Song <baohua@...nel.org>,
        Lance Yang <lance.yang@...ux.dev>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Zach O'Keefe <zokeefe@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mm/khugepaged: return EAGAIN for transient dirty
 pages in MADV_COLLAPSE

Please, please, please send a cover letter when there's > 1 patch :)

This 2/2 replying to 1/2 is a pain (not your fault that perhaps you're not aware
of typical mm series style but FYI :P)

Also there is some tiny conflict on khugepaged.c in mm-new, but it's literally 1
#include so probably nothing to worry about.


On Mon, Nov 10, 2025 at 11:32:55AM +0000, Shivank Garg wrote:
> When MADV_COLLAPSE encounters dirty file-backed pages, it currently
> returns -EINVAL, this is misleading as EINVAL suggests invalid arguments,
> whereas dirty pages are a transient condition that may resolve on retry.
>
> Introduce SCAN_PAGE_DIRTY and map it to -EAGAIN. For khugepaged, this
> is harmless as it will revisit the range after async writeback completes.
>
> Signed-off-by: Shivank Garg <shivankg@....com>

With comments below addressed, LGTM so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> ---
>  include/trace/events/huge_memory.h | 3 ++-
>  mm/khugepaged.c                    | 4 +++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> index dd94d14a2427..9014a9bbe64c 100644
> --- a/include/trace/events/huge_memory.h
> +++ b/include/trace/events/huge_memory.h
> @@ -38,7 +38,8 @@
>  	EM( SCAN_PAGE_HAS_PRIVATE,	"page_has_private")		\
>  	EM( SCAN_STORE_FAILED,		"store_failed")			\
>  	EM( SCAN_COPY_MC,		"copy_poisoned_page")		\
> -	EMe(SCAN_PAGE_FILLED,		"page_filled")
> +	EM(SCAN_PAGE_FILLED,		"page_filled")			\
> +	EMe(SCAN_PAGE_DIRTY,		"page_dirty")
>
>  #undef EM
>  #undef EMe
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index d08ed6eb9ce1..7df329c9c87d 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -60,6 +60,7 @@ enum scan_result {
>  	SCAN_STORE_FAILED,
>  	SCAN_COPY_MC,
>  	SCAN_PAGE_FILLED,
> +	SCAN_PAGE_DIRTY,

it feels like a lot to add a scan result for this, but I mean... probably
actually valid.

>  };
>
>  #define CREATE_TRACE_POINTS
> @@ -1967,7 +1968,7 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,
>  				 */
>  				xas_unlock_irq(&xas);
>  				filemap_flush(mapping);
> -				result = SCAN_FAIL;
> +				result = SCAN_PAGE_DIRTY;
>  				goto xa_unlocked;

Hmmm shmem dirty is going to be weird but we also have:

		if (!is_shmem && (folio_test_dirty(folio) ||
				  folio_test_writeback(folio))) {
			/*
			 * khugepaged only works on read-only fd, so this
			 * folio is dirty because it hasn't been flushed
			 * since first write.
			 */
			result = SCAN_FAIL;
			goto out_unlock;
		}

It's weird though, why would we have writeback, surely handled by swap, and
won't it be like anon, i.e. pretty well always dirty? This comment seems
copy/pasta wrong.

We do need to at least mention in commit message that shmem is explicitly
excluded.


>  			} else if (folio_test_writeback(folio)) {
>  				xas_unlock_irq(&xas);
> @@ -2747,6 +2748,7 @@ static int madvise_collapse_errno(enum scan_result r)
>  	case SCAN_PAGE_LRU:
>  	case SCAN_DEL_PAGE_LRU:
>  	case SCAN_PAGE_FILLED:
> +	case SCAN_PAGE_DIRTY:
>  		return -EAGAIN;
>  	/*
>  	 * Other: Trying again likely not to succeed / error intrinsic to
> --
> 2.43.0
>