1. Patchew
  2. linux
  3. [RFC PATCH 0/5] add process_madvise() flags to modify behaviour
  4. View patch

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

Lorenzo Stoakes posted 5 patches 7 months ago
[RFC PATCH 1/5] mm: madvise: refactor madvise_populate()
Posted by Lorenzo Stoakes 7 months ago 
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;
 		}
-		start += pages * PAGE_SIZE;
 	}
 	return 0;
 }
-- 
2.49.0
Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()
Posted by David Hildenbrand 7 months ago 
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?

-- 
Cheers,

David / dhildenb
Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()
Posted by Lorenzo Stoakes 7 months ago 
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? :>)

Idea is that we clearly separate out the refactoring bit from the actual change
to the logic so it's not a pain to bisect/review.

>
> --
> Cheers,
>
> David / dhildenb
>
Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()
Posted by David Hildenbrand 7 months ago 
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
Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()
Posted by Mike Rapoport 7 months ago 
On Tue, May 20, 2025 at 12:42:33PM +0200, David Hildenbrand wrote:
> 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 :)

And for refactoring patches it's always useful to mention "no functional
change" ;-)

Acked-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 
> 

-- 
Sincerely yours,
Mike.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.