From: Martin KaFai Lau <martin.lau@linux.dev>
To: Song Liu <song@kernel.org>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
kernel-team@meta.com, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline
Date: Thu, 14 Sep 2023 14:14:45 -0700 [thread overview]
Message-ID: <208035ba-3016-c9ba-92e4-fe2cee797ca8@linux.dev> (raw)
In-Reply-To: <20230913222632.3312183-1-song@kernel.org>
On 9/13/23 3:26 PM, Song Liu wrote:
> Current code charges modmem for regular trampoline, but not for struct_ops
> trampoline. Add bpf_jit_[charge|uncharge]_modmem() to struct_ops so the
> trampoline is charged in both cases.
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> kernel/bpf/bpf_struct_ops.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fdc3e8705a3c..ea6ca87a2ed9 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -615,7 +615,10 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
> if (st_map->links)
> bpf_struct_ops_map_put_progs(st_map);
> bpf_map_area_free(st_map->links);
> - bpf_jit_free_exec(st_map->image);
> + if (st_map->image) {
> + bpf_jit_free_exec(st_map->image);
> + bpf_jit_uncharge_modmem(PAGE_SIZE);
> + }
> bpf_map_area_free(st_map->uvalue);
> bpf_map_area_free(st_map);
> }
> @@ -657,6 +660,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> struct bpf_struct_ops_map *st_map;
> const struct btf_type *t, *vt;
> struct bpf_map *map;
> + int ret;
>
> st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
> if (!st_ops)
> @@ -681,6 +685,12 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> st_map->st_ops = st_ops;
> map = &st_map->map;
>
> + ret = bpf_jit_charge_modmem(PAGE_SIZE);
> + if (ret) {
> + __bpf_struct_ops_map_free(map);
> + return ERR_PTR(ret);
> + }
This just came to my mind when reading it again.
It will miss a bpf_jit_uncharge_modmem() if the bpf_jit_alloc_exec() at a few
lines below did fail (meaning st_map->image is NULL). It is because the
__bpf_struct_ops_map_free() only uncharge if st_map->image is not NULL.
How above moving the bpf_jit_alloc_exec() to here (immediately after
bpf_jit_charge_modem succeeded). Like,
st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
if (!st_map->image) {
bpf_jit_uncharge_modmem(PAGE_SIZE);
__bpf_struct_ops_map_free(map);
return ERR_PTR(-ENOMEM);
}
Then there is also no need to test 'if (st_map->image)' in
__bpf_struct_ops_map_free().
> +
> st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
> st_map->links =
> bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
> @@ -907,4 +917,3 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
> kfree(link);
> return err;
> }
> -
next prev parent reply other threads:[~2023-09-14 21:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-13 22:26 [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline Song Liu
2023-09-14 21:14 ` Martin KaFai Lau [this message]
2023-09-14 21:28 ` Song Liu
2023-09-14 22:16 ` Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=208035ba-3016-c9ba-92e4-fe2cee797ca8@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@meta.com \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox