Re: [PATCH v2 06/23] mm: introduce BPF struct ops for OOM handling

Martin KaFai Lau
2025-10-30T00:21:15Z

On 10/27/25 4:17 PM, Roman Gushchin wrote:
> diff --git a/include/linux/bpf_oom.h b/include/linux/bpf_oom.h
> new file mode 100644
> index 000000000000..18c32a5a068b
> --- /dev/null
> +++ b/include/linux/bpf_oom.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __BPF_OOM_H
> +#define __BPF_OOM_H
> +
> +struct oom_control;
> +
> +#define BPF_OOM_NAME_MAX_LEN 64
> +
> +struct bpf_oom_ctx {
> + /*
> + * If bpf_oom_ops is attached to a cgroup, id of this cgroup.
> + * 0 otherwise.
> + */
> + u64 cgroup_id;
> +};

A function argument can be added to the ops (e.g. handle_out_of_memory)
in the future. afaict, I don't see it will disrupt the existing bpf prog
as long as it does not change the ordering of the existing arguments.

If it goes down the 'struct bpf_oom_ctx" abstraction path, all future
new members of the 'struct bpf_oom_ctx' will need to be initialized even
they may not be useful for most of the existing ops.

For networking use case, I am quite sure the wrapping is unnecessary. I
will leave it as fruit of thoughts here for this use case.

> +static int bpf_oom_ops_reg(void *kdata, struct bpf_link *link)
> +{
> + struct bpf_struct_ops_link *ops_link = container_of(link, struct bpf_struct_ops_link, link);

link could be NULL here. "return -EOPNOTSUPP" for the legacy kdata reg
that does not use the link api.

In the future, we should enforce link must be used in the
bpf_struct_ops.c except for a few of the existing struct_ops kernel users.

> + struct bpf_oom_ops **bpf_oom_ops_ptr = NULL;
> + struct bpf_oom_ops *bpf_oom_ops = kdata;
> + struct mem_cgroup *memcg = NULL;
> + int err = 0;
> +
> + if (IS_ENABLED(CONFIG_MEMCG) && ops_link->cgroup_id) {
> + /* Attach to a memory cgroup? */
> + memcg = mem_cgroup_get_from_ino(ops_link->cgroup_id);
> + if (IS_ERR_OR_NULL(memcg))
> + return PTR_ERR(memcg);
> + bpf_oom_ops_ptr = bpf_oom_memcg_ops_ptr(memcg);
> + } else {
> + /* System-wide OOM handler */
> + bpf_oom_ops_ptr = &system_bpf_oom;
> + }
> +
> + /* Another struct ops attached? */
> + if (READ_ONCE(*bpf_oom_ops_ptr)) {
> + err = -EBUSY;
> + goto exit;
> + }
> +
> + /* Expose bpf_oom_ops structure */
> + WRITE_ONCE(*bpf_oom_ops_ptr, bpf_oom_ops);
> +exit:
> + mem_cgroup_put(memcg);
> + return err;
> +}
> +
> +static void bpf_oom_ops_unreg(void *kdata, struct bpf_link *link)
> +{
> + struct bpf_struct_ops_link *ops_link = container_of(link, struct bpf_struct_ops_link, link);
> + struct bpf_oom_ops **bpf_oom_ops_ptr = NULL;
> + struct bpf_oom_ops *bpf_oom_ops = kdata;
> + struct mem_cgroup *memcg = NULL;
> +
> + if (IS_ENABLED(CONFIG_MEMCG) && ops_link->cgroup_id) {
> + /* Detach from a memory cgroup? */
> + memcg = mem_cgroup_get_from_ino(ops_link->cgroup_id);
> + if (IS_ERR_OR_NULL(memcg))
> + goto exit;
> + bpf_oom_ops_ptr = bpf_oom_memcg_ops_ptr(memcg);
> + } else {
> + /* System-wide OOM handler */
> + bpf_oom_ops_ptr = &system_bpf_oom;
> + }
> +
> + /* Hide bpf_oom_ops from new callers */
> + if (!WARN_ON(READ_ONCE(*bpf_oom_ops_ptr) != bpf_oom_ops))
> + WRITE_ONCE(*bpf_oom_ops_ptr, NULL);
> +
> + mem_cgroup_put(memcg);
> +
> +exit:
> + /* Release bpf_oom_ops after a srcu grace period */
> + synchronize_srcu(&bpf_oom_srcu);
> +}
> +
> +#ifdef CONFIG_MEMCG
> +void bpf_oom_memcg_offline(struct mem_cgroup *memcg)

Is it when the memcg/cgroup is going away? I think it should also call
bpf_struct_ops_map_link_detach (through link->ops->detach [1]). It will
notify the user space which may poll on the link fd. This will also call
the bpf_oom_ops_unreg above.

[1]
https://lore.kernel.org/all/20240530065946.979330-7-thinker.li@gmail.com/

> +{
> + struct bpf_oom_ops *bpf_oom_ops;
> + struct bpf_oom_ctx exec_ctx;
> + u64 cgrp_id;
> + int idx;
> +
> + /* All bpf_oom_ops structures are protected using bpf_oom_srcu */
> + idx = srcu_read_lock(&bpf_oom_srcu);
> +
> + bpf_oom_ops = READ_ONCE(memcg->bpf_oom);
> + WRITE_ONCE(memcg->bpf_oom, NULL);
> +
> + if (bpf_oom_ops && bpf_oom_ops->handle_cgroup_offline) {
> + cgrp_id = cgroup_id(memcg->css.cgroup);
> + exec_ctx.cgroup_id = cgrp_id;
> + bpf_oom_ops->handle_cgroup_offline(&exec_ctx, cgrp_id);
> + }
> +
> + srcu_read_unlock(&bpf_oom_srcu, idx);
> +}