Re: [RFC PATCH v4 5/5] mm/filemap: Allow arch to request folio size for exec memory

[Ryan Roberts <ryan.roberts@arm.com>]

On 14/05/2025 16:14, Will Deacon wrote:
> On Tue, May 13, 2025 at 01:46:06PM +0100, Ryan Roberts wrote:
>> On 09/05/2025 14:52, Will Deacon wrote:
>>> On Wed, Apr 30, 2025 at 03:59:18PM +0100, Ryan Roberts wrote:
>>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>>> index e61f374068d4..37fe4a55c00d 100644
>>>> --- a/mm/filemap.c
>>>> +++ b/mm/filemap.c
>>>> @@ -3252,14 +3252,40 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>>>>  	if (mmap_miss > MMAP_LOTSAMISS)
>>>>  		return fpin;
>>>>  
>>>> -	/*
>>>> -	 * mmap read-around
>>>> -	 */
>>>>  	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>>> -	ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2);
>>>> -	ra->size = ra->ra_pages;
>>>> -	ra->async_size = ra->ra_pages / 4;
>>>> -	ra->order = 0;
>>>> +	if (vm_flags & VM_EXEC) {
>>>> +		/*
>>>> +		 * Allow arch to request a preferred minimum folio order for
>>>> +		 * executable memory. This can often be beneficial to
>>>> +		 * performance if (e.g.) arm64 can contpte-map the folio.
>>>> +		 * Executable memory rarely benefits from readahead, due to its
>>>> +		 * random access nature, so set async_size to 0.
>>>
>>> In light of this observation (about randomness of instruction fetch), do
>>> you think it's worth ignoring VM_RAND_READ for VM_EXEC?
>>
>> Hmm, yeah that makes sense. Something like:
>>
>> ---8<---
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 7b90cbeb4a1a..6c8bf5116c54 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -3233,7 +3233,8 @@ static struct file *do_sync_mmap_readahead(struct vm_fault
>> *vmf)
>>         if (!ra->ra_pages)
>>                 return fpin;
>>
>> -       if (vm_flags & VM_SEQ_READ) {
>> +       /* VM_EXEC case below is already intended for random access */
>> +       if ((vm_flags & (VM_SEQ_READ | VM_EXEC)) == VM_SEQ_READ) {
>>                 fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>                 page_cache_sync_ra(&ractl, ra->ra_pages);
>>                 return fpin;
>> ---8<---
> 
> I was thinking about the:
> 
> 	if (vm_flags & VM_RAND_READ)
> 		return fpin;

Yes sorry, I lost my mind when doing that patch... I intended to do it for the
VM_RAND_READ as you suggested, but my fingers did something completely different.

> 
> code above this which bails if VM_RAND_READ is set. That seems contrary
> to the code you're adding which says that, even for random access
> patterns where readahead doesn't help, it's still worth sizing the folio
> appropriately for contpte mappings.

Anyway, I totally agree with this. So I'll avoid the early return VM_RAND_READ
if VM_EXEC is also set.

Thanks,
Ryan

> 
> Will