From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 74814C48BF8 for ; Thu, 22 Feb 2024 16:11:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AD72A6B007B; Thu, 22 Feb 2024 11:11:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A86EB6B007D; Thu, 22 Feb 2024 11:11:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 94F0B6B007E; Thu, 22 Feb 2024 11:11:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 854506B007B for ; Thu, 22 Feb 2024 11:11:52 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0D7D31C0EC4 for ; Thu, 22 Feb 2024 16:11:52 +0000 (UTC) X-FDA: 81819930864.11.B0DE31B Received: from mail-qv1-f52.google.com (mail-qv1-f52.google.com [209.85.219.52]) by imf14.hostedemail.com (Postfix) with ESMTP id 46ED6100010 for ; Thu, 22 Feb 2024 16:11:50 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XsuXM3u7; spf=pass (imf14.hostedemail.com: domain of mcassell411@gmail.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=mcassell411@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708618310; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=OvUO+rCWPwr6DKZmlJGue1pvsOXF5+tjx+NxO1AdZ8g=; b=COzG8AmzuSuOcrh9izr1L66lqsOod8eASMGp35n3kA5xOs3lsnl91MfoWUlEHh7qjazC0U wXfJj9X9V6qIgx5rSl1IZQvOeQkwlU1oa//iUX3pBE5FyDePITGCPBPP9OLBv65b0139Fw fPT2c33G1pr1CQeAiMJ3far+SEN9hfc= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XsuXM3u7; spf=pass (imf14.hostedemail.com: domain of mcassell411@gmail.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=mcassell411@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708618310; a=rsa-sha256; cv=none; b=YjShiFK4/Qrij20pcU98rLoC60sCqE8BmPahqtm9vY9ajnKdhH+/nRSQUiCL+r5MyKujRi 1aZXEvpYcnzqsBKkZRz8yvPb9cYzs9tEnS/WmfLT2tzDkrt4yjZgOK+2WArxDMQ6b00tGy QaxE3KCe4EDCh8vznxg3kS5aFGpITig= Received: by mail-qv1-f52.google.com with SMTP id 6a1803df08f44-686a92a8661so10090066d6.0 for ; Thu, 22 Feb 2024 08:11:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708618309; x=1709223109; darn=kvack.org; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=OvUO+rCWPwr6DKZmlJGue1pvsOXF5+tjx+NxO1AdZ8g=; b=XsuXM3u7V5rbIFfQUOK8K0LJ2Bkq8QSWaps9RG5XbhHi4Yd/s9gGDb/Y9b7qw9rlkZ Ae0T2TIH59cjy3EyU01WJXqfK0zuy0WqCvRiZIovvKI7YZacTdss8XqmScwZBRIjgkTC b5z7YoIVCcUZeoOTYOKHEGUgJYB5PSMZcjjOE0c97KUFaHqMKvEjF077h9XXrJJg5RbC H9bNXpgjX9QLW+DstEWbDnhouoAbsok7tK7uxASxee1ccw19kY2pMlXRvqG7Rc9o4oci 2PkEt/2xd0LhVoFDUAGO7uAYusjrVvu/NOFNUzyq8DcD3EyHtVI6cNpdo75hJIQegXVr lTAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708618309; x=1709223109; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OvUO+rCWPwr6DKZmlJGue1pvsOXF5+tjx+NxO1AdZ8g=; b=bai0kzYEteOiMplgGBZZsaRp0icb7sYsuh04hz7cGxhyYD7sPwrnGjlFEjjiH7XybY pu+ULwIMg2auYIQZj04swaUbk0RqKIrS3/EmQEn/2kRwRrdo59fuzxPmmONZqEYCjrVh J1N8yEWZsgE0uNTU6GdQBopmBQmdKpo/OokkVvayU0hAV9vYBxqx+0aI2SNreOtgn4Vn 2G2Xlz5zyQs9gin2GqeMRR8Mv9qXfR4zNEJ+CGFbLKIULc1vRBI36g+zli66hyFU8B9w urniQFSefwWUaJlEXW4U0caruYzzHWgn0fUdogx9rCeab3U8fFTQmSfmJ9OxkXgLAf1O fnfg== X-Forwarded-Encrypted: i=1; AJvYcCVD01mPOCsibMriL/LrcxwqiG0fg6n7tgcr3ry+4TAP6tgaxfGxmmsGHBhLZbOCHNj41CD6+6mKhp3/qv+zRZkG7RA= X-Gm-Message-State: AOJu0YzNK3MjWknRwZNAyP7jv7ddi5JHQtG4toZQ/MWEcy5UrGh5WbX+ bIdZPguWxNhb3yo+MjZRtZcpjRkS8ZT/dTM99ZtHNGKdWxFNZe8w X-Google-Smtp-Source: AGHT+IFJfU+V4Q+jfqAhV6jiR1TUL7nTDGGEy5JkjQr6LPKKIV/owOXHqU6iu7ZA5IKqQW5ZQlDyCw== X-Received: by 2002:a0c:e2c2:0:b0:68f:20b0:5b21 with SMTP id t2-20020a0ce2c2000000b0068f20b05b21mr21845247qvl.4.1708618309249; Thu, 22 Feb 2024 08:11:49 -0800 (PST) Received: from smtpclient.apple (2600-6c40-4200-1e28-2098-f4e7-aacd-f156.inf6.spectrum.com. [2600:6c40:4200:1e28:2098:f4e7:aacd:f156]) by smtp.gmail.com with ESMTPSA id ks16-20020a056214311000b0068f30a0dc5esm1860653qvb.80.2024.02.22.08.11.48 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Feb 2024 08:11:49 -0800 (PST) From: Matt Cassell Message-Id: <3EF82DF0-D2E0-4517-99B3-9E480836F30E@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_4E752357-0079-4E65-9F2E-47A301999EE1" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.300.61.1.2\)) Subject: Re: [PATCH] mm/util.c: Added page count to __vm_enough_memory failure warning Date: Thu, 22 Feb 2024 10:11:38 -0600 In-Reply-To: <1547a955-cee6-40e1-8231-0bd1229de0f3@redhat.com> Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org To: David Hildenbrand References: <20240221160235.1771-1-mcassell411@gmail.com> <1547a955-cee6-40e1-8231-0bd1229de0f3@redhat.com> X-Mailer: Apple Mail (2.3774.300.61.1.2) X-Rspamd-Queue-Id: 46ED6100010 X-Rspam-User: X-Stat-Signature: hbhkcgto8xj398q77qi68nqowqbbqs9w X-Rspamd-Server: rspam01 X-HE-Tag: 1708618310-394277 X-HE-Meta: U2FsdGVkX19UvPee2+onOXKIZ0ofiNh1crizlAsWfPKL8Rm5yitz6aEMmr6tnd3YgMLa1i22Q3uwj8DYST71qb0z+AYxTR7zgsmyye5LcBtTUCapJXC4FAv68D5rf4/KANYAZ+CcgIxFds0ujbDzeKVLemhQeDfSrdL5yL2muN3kzkhKIrSCCzquMXspF/1HYRX0G+8XJNK7vF8JaSlAdZdgR/XuIs+6VFJTlxK9kL615dGNzeIe34k4NRV4GQiMn+GwUsKmVCmR3wuMIg0j5O3JC3XbNYz2glkkTZ6eO6R0Hwep3vgVnYyL2irJWYJUMUHimAKT0/4VcINy7VO5UhKvmryHJ8QSygo7Un7FGoA47FwPvySQR751KYfX46K+WVL66DFSbYCTILvVEStwjI+hXF0Q7iRLg+aDzgwTN4yNxTspFFsgjgnIEYJNiLjLVJUaqKbm7usylkOvaQneoaoNRZrBYzj23QsiNPdhFeIeACF+9Zl7LKe79iMG1KSzFal5mTfr87RKyZQul3toEfd0gmRqsMONqpEnH8PZdzrzvbGoQAgTr80RE+bAHv+ARR9CTpGMew28OSV1b4drK1bYy7lTk369/SiJHHsKTQcmlIzWxWaYYzpnbvwVP6bCSYzk0LRlFoGbZAYE6XHMiFwE3fjOkJaDcjLNWR/76EAqi+iBjYlyuVDFwICIydXM0svjWx5QmW5Ut3W7Zxme3o1xYsczAHcIeqatSp+zgBXDtV5rkQUY5nBPZe40OlVq72FPniHvWg7MO0Si1WKPCnwsuYqrous2Oz/wA/Zn49Khxz6Nw1aw+RKXFvt8mnen5dpPmGnTznNBbOIpp1UWS94jGLgcYd5BnBMaCIJKYwALsB4mQ6BLnbpkMvAGoEnqN3lBdKAUGG4G0sy34mEGBaqMfq+FujNvuOQ8fkLISb1CDEQ8ictam9QaJcNEYJNJpBupVwoHmZ7EILT1JfC DBDjmheL mTnOkNzhi8vBa1utrO0+sN1LTJvBc1bkB+WsHvHWcdOKa1J9YVsj4QQ1+uAiTYKTRbUHHCi10ze7TYqf59a4Vx4GwaABGJ9FIZ0M5A5d59nYYUtNIaWBJfwl7Y3xeocKjuyd2hW0GRj5N1SLYKZWSDR28sF2La6iivUnOJE0T6SFOZ7YcjgMEbPnGdbvFvxlG5113 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --Apple-Mail=_4E752357-0079-4E65-9F2E-47A301999EE1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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: <=E2=80=A6 usual mmap() stuff =E2=80=A6> 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 =3D pages << (PAGE_SHIFT); > On Feb 22, 2024, at 6:18=E2=80=AFAM, David Hildenbrand = wrote: >=20 > 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 >> #include >> int main(int argc, char **argv) { >> for(;;) { >> if(malloc(1<<30) =3D=3D 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 >> --- >> 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; >=20 > I wonder if "bytes"/"kbytes" instead of pages would be more = appropriate here. >=20 > Often, this will fail due to mmap() [where we pass a size from user = space] and also "vm.overcommit_kbytes" is not in pages. >=20 > --=20 > Cheers, >=20 > David / dhildenb --Apple-Mail=_4E752357-0079-4E65-9F2E-47A301999EE1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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:

<=E2=80=A6 usual mmap() stuff = =E2=80=A6>
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 =3D = pages << = (PAGE_SHIFT);


On Feb 22, 2024, at 6:18=E2=80=AFAM, 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) =3D=3D 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

= --Apple-Mail=_4E752357-0079-4E65-9F2E-47A301999EE1--