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 14185CA0EDC for ; Wed, 20 Aug 2025 19:53:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8A1DE6B002D; Wed, 20 Aug 2025 15:53:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 82AD86B002E; Wed, 20 Aug 2025 15:53:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 767FD6B002F; Wed, 20 Aug 2025 15:53:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 643026B002D for ; Wed, 20 Aug 2025 15:53:01 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A0DED1A0108 for ; Wed, 20 Aug 2025 19:53:00 +0000 (UTC) X-FDA: 83798184120.03.A9E060D Received: from out-182.mta1.migadu.com (out-182.mta1.migadu.com [95.215.58.182]) by imf30.hostedemail.com (Postfix) with ESMTP id D9AF38000B for ; Wed, 20 Aug 2025 19:52:58 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Jjkxwx4d; spf=pass (imf30.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1755719579; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=FnJ9wrRv5dajqLvXr4vlqO/w758vq7w+wk4DvzmlM3A=; b=8DGTe2rzBS0XrZWl/kN7/ehPntiSfdy2B0QHl/1bCFHeHLVhPyIcFI3Tj/epfJXOvuEv97 N/NLGLm0znqq6DCtE62OLB8muLp71VOrLj3bd+/VPuLpThsAklPt0gEIosWANFL7g07xMC qxsMUFMI5lRMdcV0ZYxGvpX30gL0Wxs= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Jjkxwx4d; spf=pass (imf30.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.182 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1755719579; a=rsa-sha256; cv=none; b=Hxz8g25JBbZr7CIg8Ga3EsX07qFM7wfEFY8PRN/yK3VxNI5zItx/Od63sbuRfxoq8+sS7t dOzrPknjkPZi6HNRZUXvPbozcoDM4TYRYq5Q4MvY1S0yRsyspwQvMOXEtrxloMiEo/2olL RgtYTrzELVE692ffUMmkS2yZlh0Hq6A= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1755719576; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FnJ9wrRv5dajqLvXr4vlqO/w758vq7w+wk4DvzmlM3A=; b=Jjkxwx4dWFg80pFwdE+3zmafKFCuFfs4hoQeHmtByNZv+OUwEWC6qPC9MQslNVb8GJ5fLL eTKa0tXCfPEpYzrlPtxehPL4GHq+1ffK5Mdk583LWiQCBIdWPgZ1uAQDMmF2v0Q7HdOGy2 pKsTnLonOvIbQawG66nid63+n+TDcj4= From: Roman Gushchin To: Suren Baghdasaryan Cc: linux-mm@kvack.org, bpf@vger.kernel.org, Johannes Weiner , Michal Hocko , David Rientjes , Matt Bobrowski , Song Liu , Kumar Kartikeya Dwivedi , Alexei Starovoitov , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 01/14] mm: introduce bpf struct ops for OOM handling In-Reply-To: (Suren Baghdasaryan's message of "Wed, 20 Aug 2025 12:34:56 -0700") References: <20250818170136.209169-1-roman.gushchin@linux.dev> <20250818170136.209169-2-roman.gushchin@linux.dev> <878qjf13gx.fsf@linux.dev> Date: Wed, 20 Aug 2025 12:52:46 -0700 Message-ID: <87ikihpy7l.fsf@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: D9AF38000B X-Stat-Signature: 4bgn856sm77j1k4zkotfdfqh3ws9jtbm X-Rspam-User: X-HE-Tag: 1755719578-892646 X-HE-Meta: U2FsdGVkX183cKaDCFn469dQtVAMro5lfTl9q5yjfduzaMh5K6ee+OAAIC0LudLxAdOpqsTylalpM+peURprWCEsnwbSwaIGvrgyEb1A5DDlnjmyVAK0w9fcWZ0hFwRzC/+bMEBZ+b9L0rSHyRrVbNFib0YL1MFuA5KDxOGOHKDC4Kb8bvUZC4nZV53X/Cjigcs7vOee+pIqI+V+awO1F+CKQAbXhYBgD1gh0gRcarhp81wjT9AaRqBAMooo9hm41kWg/PxpgPpHdoXytZaMQzImFl0AstLjPTGBF5FTrhscn74mBfaJ4rMAfmtGCntLXIJQPgp9s+kscJFtvsIRKR1s8rNr3QME/1ElKen3REsU+U710ut6tROjA/kAp5szjNmxVX2XkxV3hb86n1BQluR67ipsEO42BrMgKt63xwfgmRLfkcmAYQYyr8BjtObtXYwO2NKrtxCOhZZ0+5M7X93jtjvI4dCKqTUTmiIY95+Ryi9Ub1kF24DY+OBahEGNzNqivwDT5AC4vQqODR9nfdeVqPYz8xZl7Pu2gReOhuQ2ojv2Zjr0QNt4Dqh0xBubwzvcA81Mj5JWc80zLfUorT+R1mumjFuUqByEsxZd7NntfOq0my1laCrAaTxGbBIqYSH653b3KGyzidpKJtV02yGBo1hW83pgGQMopuVNkcBlve24UTNHupGinZxJ6V2Ub/GHIohX9v0VyqI7Z4snJQ8pN0Yjnlfa1cYRvufwHtIarDd7Zwk++rASFkvSgWF5HyFtj0Mxy1kAyUCaGQuYRS+ngHRJbX1cw1cbr+YiK74Gt/OYt6jzcnhLAQCQB1hDVFbl5gs7PKGcrvMw4fP7I2sjslKFCjQ5CRQafzWUvEcUlrtjQqOctA== 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: Suren Baghdasaryan writes: > On Tue, Aug 19, 2025 at 1:06=E2=80=AFPM Roman Gushchin wrote: >> >> Suren Baghdasaryan writes: >> >> > On Mon, Aug 18, 2025 at 10:01=E2=80=AFAM Roman Gushchin >> > wrote: >> >> >> >> Introduce a bpf struct ops for implementing custom OOM handling polic= ies. >> >> >> >> The struct ops provides the bpf_handle_out_of_memory() callback, >> >> which expected to return 1 if it was able to free some memory and 0 >> >> otherwise. >> >> >> >> In the latter case it's guaranteed that the in-kernel OOM killer will >> >> be invoked. Otherwise the kernel also checks the bpf_memory_freed >> >> field of the oom_control structure, which is expected to be set by >> >> kfuncs suitable for releasing memory. It's a safety mechanism which >> >> prevents a bpf program to claim forward progress without actually >> >> releasing memory. The callback program is sleepable to enable using >> >> iterators, e.g. cgroup iterators. >> >> >> >> The callback receives struct oom_control as an argument, so it can >> >> easily filter out OOM's it doesn't want to handle, e.g. global vs >> >> memcg OOM's. >> >> >> >> The callback is executed just before the kernel victim task selection >> >> algorithm, so all heuristics and sysctls like panic on oom, >> >> sysctl_oom_kill_allocating_task and sysctl_oom_kill_allocating_task >> >> are respected. >> >> >> >> The struct ops also has the name field, which allows to define a >> >> custom name for the implemented policy. It's printed in the OOM report >> >> in the oom_policy=3D format. "default" is printed if bpf is n= ot >> >> used or policy name is not specified. >> >> >> >> [ 112.696676] test_progs invoked oom-killer: gfp_mask=3D0xcc0(GFP_KE= RNEL), order=3D0, oom_score_adj=3D0 >> >> oom_policy=3Dbpf_test_policy >> >> [ 112.698160] CPU: 1 UID: 0 PID: 660 Comm: test_progs Not tainted 6.= 16.0-00015-gf09eb0d6badc #102 PREEMPT(full) >> >> [ 112.698165] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),= BIOS 1.17.0-5.fc42 04/01/2014 >> >> [ 112.698167] Call Trace: >> >> [ 112.698177] >> >> [ 112.698182] dump_stack_lvl+0x4d/0x70 >> >> [ 112.698192] dump_header+0x59/0x1c6 >> >> [ 112.698199] oom_kill_process.cold+0x8/0xef >> >> [ 112.698206] bpf_oom_kill_process+0x59/0xb0 >> >> [ 112.698216] bpf_prog_7ecad0f36a167fd7_test_out_of_memory+0x2be/0x= 313 >> >> [ 112.698229] bpf__bpf_oom_ops_handle_out_of_memory+0x47/0xaf >> >> [ 112.698236] ? srso_alias_return_thunk+0x5/0xfbef5 >> >> [ 112.698240] bpf_handle_oom+0x11a/0x1e0 >> >> [ 112.698250] out_of_memory+0xab/0x5c0 >> >> [ 112.698258] mem_cgroup_out_of_memory+0xbc/0x110 >> >> [ 112.698274] try_charge_memcg+0x4b5/0x7e0 >> >> [ 112.698288] charge_memcg+0x2f/0xc0 >> >> [ 112.698293] __mem_cgroup_charge+0x30/0xc0 >> >> [ 112.698299] do_anonymous_page+0x40f/0xa50 >> >> [ 112.698311] __handle_mm_fault+0xbba/0x1140 >> >> [ 112.698317] ? srso_alias_return_thunk+0x5/0xfbef5 >> >> [ 112.698335] handle_mm_fault+0xe6/0x370 >> >> [ 112.698343] do_user_addr_fault+0x211/0x6a0 >> >> [ 112.698354] exc_page_fault+0x75/0x1d0 >> >> [ 112.698363] asm_exc_page_fault+0x26/0x30 >> >> [ 112.698366] RIP: 0033:0x7fa97236db00 >> >> >> >> It's possible to load multiple bpf struct programs. In the case of >> >> oom, they will be executed one by one in the same order they been >> >> loaded until one of them returns 1 and bpf_memory_freed is set to 1 >> >> - an indication that the memory was freed. This allows to have >> >> multiple bpf programs to focus on different types of OOM's - e.g. >> >> one program can only handle memcg OOM's in one memory cgroup. >> >> But the filtering is done in bpf - so it's fully flexible. >> >> >> >> Signed-off-by: Roman Gushchin >> >> --- >> >> include/linux/bpf_oom.h | 49 +++++++++++++ >> >> include/linux/oom.h | 8 ++ >> >> mm/Makefile | 3 + >> >> mm/bpf_oom.c | 157 ++++++++++++++++++++++++++++++++++++++= ++ >> >> mm/oom_kill.c | 22 +++++- >> >> 5 files changed, 237 insertions(+), 2 deletions(-) >> >> create mode 100644 include/linux/bpf_oom.h >> >> create mode 100644 mm/bpf_oom.c >> >> >> >> diff --git a/include/linux/bpf_oom.h b/include/linux/bpf_oom.h >> >> new file mode 100644 >> >> index 000000000000..29cb5ea41d97 >> >> --- /dev/null >> >> +++ b/include/linux/bpf_oom.h >> >> @@ -0,0 +1,49 @@ >> >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> >> + >> >> +#ifndef __BPF_OOM_H >> >> +#define __BPF_OOM_H >> >> + >> >> +struct bpf_oom; >> >> +struct oom_control; >> >> + >> >> +#define BPF_OOM_NAME_MAX_LEN 64 >> >> + >> >> +struct bpf_oom_ops { >> >> + /** >> >> + * @handle_out_of_memory: Out of memory bpf handler, called b= efore >> >> + * the in-kernel OOM killer. >> >> + * @oc: OOM control structure >> >> + * >> >> + * Should return 1 if some memory was freed up, otherwise >> >> + * the in-kernel OOM killer is invoked. >> >> + */ >> >> + int (*handle_out_of_memory)(struct oom_control *oc); >> >> + >> >> + /** >> >> + * @name: BPF OOM policy name >> >> + */ >> >> + char name[BPF_OOM_NAME_MAX_LEN]; >> > >> > Why should the name be a part of ops structure? IMO it's not an >> > attribute of the operations but rather of the oom handler which is >> > represented by bpf_oom here. >> >> The ops structure describes a user-defined oom policy. Currently >> it's just one handler and the policy name. Later additional handlers >> can be added, e.g. a handler to control the dmesg output. >> >> bpf_oom is an implementation detail: it's basically an extension >> to struct bpf_oom_ops which contains "private" fields required >> for the internal machinery. > > Ok. I hope we can come up with some more descriptive naming but I > can't think of something good ATM. > >> >> > >> >> + >> >> + /* Private */ >> >> + struct bpf_oom *bpf_oom; >> >> +}; >> >> + >> >> +#ifdef CONFIG_BPF_SYSCALL >> >> +/** >> >> + * @bpf_handle_oom: handle out of memory using bpf programs >> >> + * @oc: OOM control structure >> >> + * >> >> + * Returns true if a bpf oom program was executed, returned 1 >> >> + * and some memory was actually freed. >> > >> > The above comment is unclear, please clarify. >> >> Fixed, thanks. >> >> /** >> * @bpf_handle_oom: handle out of memory condition using bpf >> * @oc: OOM control structure >> * >> * Returns true if some memory was freed. >> */ >> bool bpf_handle_oom(struct oom_control *oc); >> >> >> > >> >> + */ >> >> +bool bpf_handle_oom(struct oom_control *oc); >> >> + >> >> +#else /* CONFIG_BPF_SYSCALL */ >> >> +static inline bool bpf_handle_oom(struct oom_control *oc) >> >> +{ >> >> + return false; >> >> +} >> >> + >> >> +#endif /* CONFIG_BPF_SYSCALL */ >> >> + >> >> +#endif /* __BPF_OOM_H */ >> >> diff --git a/include/linux/oom.h b/include/linux/oom.h >> >> index 1e0fc6931ce9..ef453309b7ea 100644 >> >> --- a/include/linux/oom.h >> >> +++ b/include/linux/oom.h >> >> @@ -51,6 +51,14 @@ struct oom_control { >> >> >> >> /* Used to print the constraint info. */ >> >> enum oom_constraint constraint; >> >> + >> >> +#ifdef CONFIG_BPF_SYSCALL >> >> + /* Used by the bpf oom implementation to mark the forward pro= gress */ >> >> + bool bpf_memory_freed; >> >> + >> >> + /* Policy name */ >> >> + const char *bpf_policy_name; >> >> +#endif >> >> }; >> >> >> >> extern struct mutex oom_lock; >> >> diff --git a/mm/Makefile b/mm/Makefile >> >> index 1a7a11d4933d..a714aba03759 100644 >> >> --- a/mm/Makefile >> >> +++ b/mm/Makefile >> >> @@ -105,6 +105,9 @@ obj-$(CONFIG_MEMCG) +=3D memcontrol.o vmpressure.o >> >> ifdef CONFIG_SWAP >> >> obj-$(CONFIG_MEMCG) +=3D swap_cgroup.o >> >> endif >> >> +ifdef CONFIG_BPF_SYSCALL >> >> +obj-y +=3D bpf_oom.o >> >> +endif >> >> obj-$(CONFIG_CGROUP_HUGETLB) +=3D hugetlb_cgroup.o >> >> obj-$(CONFIG_GUP_TEST) +=3D gup_test.o >> >> obj-$(CONFIG_DMAPOOL_TEST) +=3D dmapool_test.o >> >> diff --git a/mm/bpf_oom.c b/mm/bpf_oom.c >> >> new file mode 100644 >> >> index 000000000000..47633046819c >> >> --- /dev/null >> >> +++ b/mm/bpf_oom.c >> >> @@ -0,0 +1,157 @@ >> >> +// SPDX-License-Identifier: GPL-2.0-or-later >> >> +/* >> >> + * BPF-driven OOM killer customization >> >> + * >> >> + * Author: Roman Gushchin >> >> + */ >> >> + >> >> +#include >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +DEFINE_STATIC_SRCU(bpf_oom_srcu); >> >> +static DEFINE_SPINLOCK(bpf_oom_lock); >> >> +static LIST_HEAD(bpf_oom_handlers); >> >> + >> >> +struct bpf_oom { >> > >> > Perhaps bpf_oom_handler ? Then bpf_oom_ops->bpf_oom could be called >> > bpf_oom_ops->handler. >> >> I don't think it's a handler, it's more like a private part >> of bpf_oom_ops. Maybe bpf_oom_impl? Idk > > Yeah, we need to come up with some nomenclature and name these structs > accordingly. In my mind ops means a structure that contains only > operations, so current naming does not sit well but maybe that's just > me... > >> >> > >> > >> >> + struct bpf_oom_ops *ops; >> >> + struct list_head node; >> >> + struct srcu_struct srcu; >> >> +}; >> >> + >> >> +bool bpf_handle_oom(struct oom_control *oc) >> >> +{ >> >> + struct bpf_oom_ops *ops; >> >> + struct bpf_oom *bpf_oom; >> >> + int list_idx, idx, ret =3D 0; >> >> + >> >> + oc->bpf_memory_freed =3D false; >> >> + >> >> + list_idx =3D srcu_read_lock(&bpf_oom_srcu); >> >> + list_for_each_entry_srcu(bpf_oom, &bpf_oom_handlers, node, fa= lse) { >> >> + ops =3D READ_ONCE(bpf_oom->ops); >> >> + if (!ops || !ops->handle_out_of_memory) >> >> + continue; >> >> + idx =3D srcu_read_lock(&bpf_oom->srcu); >> >> + oc->bpf_policy_name =3D ops->name[0] ? &ops->name[0] : >> >> + "bpf_defined_policy"; >> >> + ret =3D ops->handle_out_of_memory(oc); >> >> + oc->bpf_policy_name =3D NULL; >> >> + srcu_read_unlock(&bpf_oom->srcu, idx); >> >> + >> >> + if (ret && oc->bpf_memory_freed) >> > >> > IIUC ret and oc->bpf_memory_freed seem to reflect the same state: >> > handler successfully freed some memory. Could you please clarify when >> > they differ? >> >> The idea here is to provide an additional safety measure: >> if the bpf program simple returns 1 without doing anything, >> the system won't deadlock. >> >> oc->bpf_memory_freed is set by the bpf_oom_kill_process() helper >> (and potentially some other helpers in the future, e.g. >> bpf_oom_rm_tmpfs_file()) and can't be modified by the bpf >> program directly. > > I see. Then maybe we use only oc->bpf_memory_freed and > handle_out_of_memory() does not return anything? Idk, I think it's neat to have an ability to pass to the in-kernel OOM killer even after killing a task. Also, I believe, bpf programs have to return an int anyway, so we can ignore it, but I don't necessary see the point.