Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()

[David Hildenbrand <david@redhat.com>]

On 20.05.25 12:36, Lorenzo Stoakes wrote:
> On Tue, May 20, 2025 at 12:30:24PM +0200, David Hildenbrand wrote:
>> On 19.05.25 22:52, Lorenzo Stoakes wrote:
>>> Use a for-loop rather than a while with the update of the start argument at
>>> the end of the while-loop.
>>>
>>> This is in preparation for a subsequent commit which modifies this
>>> function, we therefore separate the refactoring from the actual change
>>> cleanly by separating the two.
>>>
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>>    mm/madvise.c | 39 ++++++++++++++++++++-------------------
>>>    1 file changed, 20 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>> index 8433ac9b27e0..63cc69daa4c7 100644
>>> --- a/mm/madvise.c
>>> +++ b/mm/madvise.c
>>> @@ -967,32 +967,33 @@ static long madvise_populate(struct mm_struct *mm, unsigned long start,
>>>    	int locked = 1;
>>>    	long pages;
>>> -	while (start < end) {
>>> +	for (; start < end; start += pages * PAGE_SIZE) {
>>>    		/* Populate (prefault) page tables readable/writable. */
>>>    		pages = faultin_page_range(mm, start, end, write, &locked);
>>>    		if (!locked) {
>>>    			mmap_read_lock(mm);
>>>    			locked = 1;
>>>    		}
>>> -		if (pages < 0) {
>>> -			switch (pages) {
>>> -			case -EINTR:
>>> -				return -EINTR;
>>> -			case -EINVAL: /* Incompatible mappings / permissions. */
>>> -				return -EINVAL;
>>> -			case -EHWPOISON:
>>> -				return -EHWPOISON;
>>> -			case -EFAULT: /* VM_FAULT_SIGBUS or VM_FAULT_SIGSEGV */
>>> -				return -EFAULT;
>>> -			default:
>>> -				pr_warn_once("%s: unhandled return value: %ld\n",
>>> -					     __func__, pages);
>>> -				fallthrough;
>>> -			case -ENOMEM: /* No VMA or out of memory. */
>>> -				return -ENOMEM;
>>> -			}
>>> +
>>> +		if (pages >= 0)
>>> +			continue;
>>> +
>>> +		switch (pages) {
>>> +		case -EINTR:
>>> +			return -EINTR;
>>> +		case -EINVAL: /* Incompatible mappings / permissions. */
>>> +			return -EINVAL;
>>> +		case -EHWPOISON:
>>> +			return -EHWPOISON;
>>> +		case -EFAULT: /* VM_FAULT_SIGBUS or VM_FAULT_SIGSEGV */
>>> +			return -EFAULT;
>>> +		default:
>>> +			pr_warn_once("%s: unhandled return value: %ld\n",
>>> +				     __func__, pages);
>>> +			fallthrough;
>>> +		case -ENOMEM: /* No VMA or out of memory. */
>>> +			return -ENOMEM;
>>
>> Can we limit it to what the patch description says? "Use a for-loop rather
>> than a while", or will that be a problem for the follow-up patch?
> 
> Well, kind of the point is that we can remove a level of indentation also, which
> then makes life easier in subsequent patch.
> 
> Happy to change description or break into two (but that seems a bit over the top
> maybe? :>)

Probably just mention it, otherwise it looks a bit like unrelated churn :)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb