[PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[Matthew Cassell]

Commit 44b414c8715c5dcf53288 ("mm/util.c: add warning if __vm_enough_memory
fails") adds debug information which gives the process id and executable name
should __vm_enough_memory() fail. Adding the number of pages to the failure
message would benefit application developers and system administrators in
debugging overambitious memory requests by providing a point of reference to
the amount of memory causing __vm_enough_memory() to fail.

1. Set appropriate kernel tunable to reach code path for failure
   message:

	# echo 2 > /proc/sys/vm/overcommit_memory

2. Test program to generate failure - requests 1 gibibyte per iteration:

	#include <stdlib.h>
	#include <stdio.h>

	int main(int argc, char **argv) {
		for(;;) {
			if(malloc(1<<30) == NULL)
				break;

			printf("allocated 1 GiB\n");
		}

		return 0;
	}

3. Output:

	Before:

	__vm_enough_memory: pid: 1218, comm: a.out, not enough
memory for the allocation

	After:

	__vm_enough_memory: pid: 1141, comm: a.out, pages: 262145, not
enough memory for the allocation

Signed-off-by: Matthew Cassell <mcassell411@gmail.com>
---
 mm/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index 5a6a9802583b..c0afb56f16ea 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -976,8 +976,8 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
 	if (percpu_counter_read_positive(&vm_committed_as) < allowed)
 		return 0;
 error:
-	pr_warn_ratelimited("%s: pid: %d, comm: %s, not enough memory for the allocation\n",
-			    __func__, current->pid, current->comm);
+	pr_warn_ratelimited("%s: pid: %d, comm: %s, pages: %ld, not enough memory for the allocation\n",
+			    __func__, current->pid, current->comm, pages);
 	vm_unacct_memory(pages);
 
 	return -ENOMEM;
-- 
2.43.2

Re: [PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[David Hildenbrand]

On 21.02.24 17:02, Matthew Cassell wrote:
> Commit 44b414c8715c5dcf53288 ("mm/util.c: add warning if __vm_enough_memory
> fails") adds debug information which gives the process id and executable name
> should __vm_enough_memory() fail. Adding the number of pages to the failure
> message would benefit application developers and system administrators in
> debugging overambitious memory requests by providing a point of reference to
> the amount of memory causing __vm_enough_memory() to fail.
> 
> 1. Set appropriate kernel tunable to reach code path for failure
>     message:
> 
> 	# echo 2 > /proc/sys/vm/overcommit_memory
> 
> 2. Test program to generate failure - requests 1 gibibyte per iteration:
> 
> 	#include <stdlib.h>
> 	#include <stdio.h>
> 
> 	int main(int argc, char **argv) {
> 		for(;;) {
> 			if(malloc(1<<30) == NULL)
> 				break;
> 
> 			printf("allocated 1 GiB\n");
> 		}
> 
> 		return 0;
> 	}
> 
> 3. Output:
> 
> 	Before:
> 
> 	__vm_enough_memory: pid: 1218, comm: a.out, not enough
> memory for the allocation
> 
> 	After:
> 
> 	__vm_enough_memory: pid: 1141, comm: a.out, pages: 262145, not
> enough memory for the allocation
> 
> Signed-off-by: Matthew Cassell <mcassell411@gmail.com>
> ---
>   mm/util.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 5a6a9802583b..c0afb56f16ea 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -976,8 +976,8 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
>   	if (percpu_counter_read_positive(&vm_committed_as) < allowed)
>   		return 0;
>   error:
> -	pr_warn_ratelimited("%s: pid: %d, comm: %s, not enough memory for the allocation\n",
> -			    __func__, current->pid, current->comm);
> +	pr_warn_ratelimited("%s: pid: %d, comm: %s, pages: %ld, not enough memory for the allocation\n",
> +			    __func__, current->pid, current->comm, pages);
>   	vm_unacct_memory(pages);
>   
>   	return -ENOMEM;

I wonder if "bytes"/"kbytes" instead of pages would be more appropriate 
here.

Often, this will fail due to mmap() [where we pass a size from user 
space] and also "vm.overcommit_kbytes" is not in pages.

-- 
Cheers,

David / dhildenb

Re: [PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[Matt Cassell]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

Thank you for the feedback. I agree with you and would prefer to use bytes/kbytes. Here are the 2 concerns that led to me keeping it as pages:


1. Reduce the impact of the patch. Here is the call trace to reach the failure warning:

	<… usual mmap() stuff …>
	mmap_region() -> security_enough_memory_mm() -> __vm_enough_memory()

	Within mmap_region(), the length variable originally passed to mmap() gets right-shifted to get the page count. My first thought was to add an additional an additional argument to security_enough_memory_mm() of type 	unsigned long to keep that variable, but saw a handful of calls to it that would have to conform to the change. Not that I do not think this debug statement does not warrant that, I felt the less impact, the better.


2. Concerned about losing bits. When converting back to bytes I was worried about the loss of precision and printing that number back to users:

	unsigned long bytes_failed = pages << (PAGE_SHIFT);


> On Feb 22, 2024, at 6:18 AM, David Hildenbrand <david@redhat.com> wrote:
> 
> On 21.02.24 17:02, Matthew Cassell wrote:
>> Commit 44b414c8715c5dcf53288 ("mm/util.c: add warning if __vm_enough_memory
>> fails") adds debug information which gives the process id and executable name
>> should __vm_enough_memory() fail. Adding the number of pages to the failure
>> message would benefit application developers and system administrators in
>> debugging overambitious memory requests by providing a point of reference to
>> the amount of memory causing __vm_enough_memory() to fail.
>> 1. Set appropriate kernel tunable to reach code path for failure
>>    message:
>> 	# echo 2 > /proc/sys/vm/overcommit_memory
>> 2. Test program to generate failure - requests 1 gibibyte per iteration:
>> 	#include <stdlib.h>
>> 	#include <stdio.h>
>> 	int main(int argc, char **argv) {
>> 		for(;;) {
>> 			if(malloc(1<<30) == NULL)
>> 				break;
>> 			printf("allocated 1 GiB\n");
>> 		}
>> 		return 0;
>> 	}
>> 3. Output:
>> 	Before:
>> 	__vm_enough_memory: pid: 1218, comm: a.out, not enough
>> memory for the allocation
>> 	After:
>> 	__vm_enough_memory: pid: 1141, comm: a.out, pages: 262145, not
>> enough memory for the allocation
>> Signed-off-by: Matthew Cassell <mcassell411@gmail.com>
>> ---
>>  mm/util.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> diff --git a/mm/util.c b/mm/util.c
>> index 5a6a9802583b..c0afb56f16ea 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -976,8 +976,8 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
>>  	if (percpu_counter_read_positive(&vm_committed_as) < allowed)
>>  		return 0;
>>  error:
>> -	pr_warn_ratelimited("%s: pid: %d, comm: %s, not enough memory for the allocation\n",
>> -			    __func__, current->pid, current->comm);
>> +	pr_warn_ratelimited("%s: pid: %d, comm: %s, pages: %ld, not enough memory for the allocation\n",
>> +			    __func__, current->pid, current->comm, pages);
>>  	vm_unacct_memory(pages);
>>    	return -ENOMEM;
> 
> I wonder if "bytes"/"kbytes" instead of pages would be more appropriate here.
> 
> Often, this will fail due to mmap() [where we pass a size from user space] and also "vm.overcommit_kbytes" is not in pages.
> 
> -- 
> Cheers,
> 
> David / dhildenb


[-- Attachment #2: Type: text/html, Size: 12198 bytes --]

Re: [PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[Matthew Cassell]

Resending due to plain-text email issue that caused mailing list to get skipped.

Thank you for the feedback. I agree with you and would prefer to use
bytes/kbytes. Here are the 2 concerns that led to me keeping it as
pages:


1. Reduce the impact of the patch. Here is the call trace to reach the
failure warning:

<… usual mmap() stuff …>
mmap_region() -> security_enough_memory_mm() -> __vm_enough_memory()

Within mmap_region(), the length variable originally passed to mmap()
gets right-shifted to get the page count. My first thought was to add
an additional an additional argument to security_enough_memory_mm() of
type unsigned long to keep that variable, but saw a handful of calls
to it that would have to conform to the change. Not that I do not
think this debug statement does not warrant that, I felt the less
impact, the better.


2. Concerned about losing bits. When converting back to bytes I was
worried about the loss of precision and printing that number back to
users:

unsigned long bytes_failed = pages << (PAGE_SHIFT);


On Thu, Feb 22, 2024 at 6:18 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 21.02.24 17:02, Matthew Cassell wrote:
> > Commit 44b414c8715c5dcf53288 ("mm/util.c: add warning if __vm_enough_memory
> > fails") adds debug information which gives the process id and executable name
> > should __vm_enough_memory() fail. Adding the number of pages to the failure
> > message would benefit application developers and system administrators in
> > debugging overambitious memory requests by providing a point of reference to
> > the amount of memory causing __vm_enough_memory() to fail.
> >
> > 1. Set appropriate kernel tunable to reach code path for failure
> >     message:
> >
> >       # echo 2 > /proc/sys/vm/overcommit_memory
> >
> > 2. Test program to generate failure - requests 1 gibibyte per iteration:
> >
> >       #include <stdlib.h>
> >       #include <stdio.h>
> >
> >       int main(int argc, char **argv) {
> >               for(;;) {
> >                       if(malloc(1<<30) == NULL)
> >                               break;
> >
> >                       printf("allocated 1 GiB\n");
> >               }
> >
> >               return 0;
> >       }
> >
> > 3. Output:
> >
> >       Before:
> >
> >       __vm_enough_memory: pid: 1218, comm: a.out, not enough
> > memory for the allocation
> >
> >       After:
> >
> >       __vm_enough_memory: pid: 1141, comm: a.out, pages: 262145, not
> > enough memory for the allocation
> >
> > Signed-off-by: Matthew Cassell <mcassell411@gmail.com>
> > ---
> >   mm/util.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/util.c b/mm/util.c
> > index 5a6a9802583b..c0afb56f16ea 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -976,8 +976,8 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
> >       if (percpu_counter_read_positive(&vm_committed_as) < allowed)
> >               return 0;
> >   error:
> > -     pr_warn_ratelimited("%s: pid: %d, comm: %s, not enough memory for the allocation\n",
> > -                         __func__, current->pid, current->comm);
> > +     pr_warn_ratelimited("%s: pid: %d, comm: %s, pages: %ld, not enough memory for the allocation\n",
> > +                         __func__, current->pid, current->comm, pages);
> >       vm_unacct_memory(pages);
> >
> >       return -ENOMEM;
>
> I wonder if "bytes"/"kbytes" instead of pages would be more appropriate
> here.
>
> Often, this will fail due to mmap() [where we pass a size from user
> space] and also "vm.overcommit_kbytes" is not in pages.
>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning

[David Hildenbrand]

On 22.02.24 17:24, Matthew Cassell wrote:
> Resending due to plain-text email issue that caused mailing list to get skipped.
> 
> Thank you for the feedback. I agree with you and would prefer to use
> bytes/kbytes. Here are the 2 concerns that led to me keeping it as
> pages:
> 
> 
> 1. Reduce the impact of the patch. Here is the call trace to reach the
> failure warning:
> 
> <… usual mmap() stuff …>
> mmap_region() -> security_enough_memory_mm() -> __vm_enough_memory()
> 
> Within mmap_region(), the length variable originally passed to mmap()
> gets right-shifted to get the page count. My first thought was to add
> an additional an additional argument to security_enough_memory_mm() of
> type unsigned long to keep that variable, but saw a handful of calls
> to it that would have to conform to the change. Not that I do not
> think this debug statement does not warrant that, I felt the less
> impact, the better.
> 
> 
> 2. Concerned about losing bits. When converting back to bytes I was
> worried about the loss of precision and printing that number back to
> users:
> 
> unsigned long bytes_failed = pages << (PAGE_SHIFT);
> 

In which scenario would you imagine that we lose precision? In other 
words, how would someone be able to create a VMA that is larger than 
what we can fit into an unsigned long in bytes?

I'd simply print "pages << PAGE_SHIFT" here and not worry about that :)

-- 
Cheers,

David / dhildenb