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@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
>
