linux-kernel - Re: [PATCH v3] mm/filemap: Allow arch to request folio size for exec memory

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: <Z-b1FmZ5nHzh5huL@casper.infradead.org>
Date: Fri, 28 Mar 2025 19:14:30 +0000
From: Matthew Wilcox <willy@...radead.org>
To: Ryan Roberts <ryan.roberts@....com>
Cc: Kalesh Singh <kaleshsingh@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Hildenbrand <david@...hat.com>,
	Dave Chinner <david@...morbit.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v3] mm/filemap: Allow arch to request folio size for exec
 memory

On Thu, Mar 27, 2025 at 04:23:14PM -0400, Ryan Roberts wrote:
> + Kalesh
> 
> On 27/03/2025 12:44, Matthew Wilcox wrote:
> > On Thu, Mar 27, 2025 at 04:06:58PM +0000, Ryan Roberts wrote:
> >> So let's special-case the read(ahead) logic for executable mappings. The
> >> trade-off is performance improvement (due to more efficient storage of
> >> the translations in iTLB) vs potential read amplification (due to
> >> reading too much data around the fault which won't be used), and the
> >> latter is independent of base page size. I've chosen 64K folio size for
> >> arm64 which benefits both the 4K and 16K base page size configs and
> >> shouldn't lead to any read amplification in practice since the old
> >> read-around path was (usually) reading blocks of 128K. I don't
> >> anticipate any write amplification because text is always RO.
> > 
> > Is there not also the potential for wasted memory due to ELF alignment?
> 
> I think this is an orthogonal issue? My change isn't making that any worse.

To a certain extent, it is.  If readahead was doing order-2 allocations
before and is now doing order-4, you're tying up 0-12 extra pages which
happen to be filled with zeroes due to being used to cache the contents
of a hole.

> > Kalesh talked about it in the MM BOF at the same time that Ted and I
> > were discussing it in the FS BOF.  Some coordination required (like
> > maybe Kalesh could have mentioned it to me rathere than assuming I'd be
> > there?)
> 
> I was at Kalesh's talk. David H suggested that a potential solution might be for
> readahead to ask the fs where the next hole is and then truncate readahead to
> avoid reading the hole. Given it's padding, nothing should directly fault it in
> so it never ends up in the page cache. Not sure if you discussed anything like
> that if you were talking in parallel?

Ted said that he and Kalesh had talked about that solution.  I have a
more bold solution in mind which lifts the ext4 extent cache to the
VFS inode so that the readahead code can interrogate it.

> Anyway, I'm not sure if you're suggesting these changes need to be considered as
> one somehow or if you're just mentioning it given it is loosely related? My view
> is that this change is an improvement indepently and could go in much sooner.

This is not a reason to delay this patch.  It's just a downside which
should be mentioned in the commit message.

> >> +static inline int arch_exec_folio_order(void)
> >> +{
> >> +	return -1;
> >> +}
> > 
> > This feels a bit fragile.  I often expect to be able to store an order
> > in an unsigned int.  Why not return 0 instead?
> 
> Well 0 is a valid order, no? I think we have had the "is order signed or
> unsigned" argument before. get_order() returns a signed int :)

But why not always return a valid order?  I don't think we need a
sentinel.  The default value can be 0 to do what we do today.