From: Stanislav Fomichev <sdf@google.com>
To: Kui-Feng Lee <kuifeng@meta.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, martin.lau@linux.dev,
song@kernel.org, kernel-team@meta.com, andrii@kernel.org
Subject: Re: [PATCH bpf-next v3 2/8] bpf: Create links for BPF struct_ops maps.
Date: Mon, 6 Mar 2023 12:23:20 -0800 [thread overview]
Message-ID: <ZAZLuF/ZfbX3RTX1@google.com> (raw)
In-Reply-To: <20230303012122.852654-3-kuifeng@meta.com>
On 03/02, Kui-Feng Lee wrote:
> BPF struct_ops maps are employed directly to register TCP Congestion
> Control algorithms. Unlike other BPF programs that terminate when
> their links gone. The link of a BPF struct_ops map provides a uniform
> experience akin to other types of BPF programs.
> bpf_links are responsible for registering their associated
> struct_ops. You can only use a struct_ops that has the BPF_F_LINK flag
> set to create a bpf_link, while a structs without this flag behaves in
> the same manner as before and is registered upon updating its value.
> The BPF_LINK_TYPE_STRUCT_OPS serves a dual purpose. Not only is it
> used to craft the links for BPF struct_ops programs, but also to
> create links for BPF struct_ops them-self. Since the links of BPF
> struct_ops programs are only used to create trampolines internally,
> they are never seen in other contexts. Thus, they can be reused for
> struct_ops themself.
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
> include/linux/bpf.h | 11 +++
> include/uapi/linux/bpf.h | 12 +++-
> kernel/bpf/bpf_struct_ops.c | 118 +++++++++++++++++++++++++++++++--
> kernel/bpf/syscall.c | 26 +++++---
> tools/include/uapi/linux/bpf.h | 12 +++-
> 5 files changed, 164 insertions(+), 15 deletions(-)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cb837f42b99d..b845be719422 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1396,6 +1396,11 @@ struct bpf_link {
> struct work_struct work;
> };
> +struct bpf_struct_ops_link {
> + struct bpf_link link;
> + struct bpf_map __rcu *map;
> +};
> +
> struct bpf_link_ops {
> void (*release)(struct bpf_link *link);
> void (*dealloc)(struct bpf_link *link);
> @@ -1964,6 +1969,7 @@ int bpf_link_new_fd(struct bpf_link *link);
> struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd);
> struct bpf_link *bpf_link_get_from_fd(u32 ufd);
> struct bpf_link *bpf_link_get_curr_or_next(u32 *id);
> +int bpf_struct_ops_link_create(union bpf_attr *attr);
> int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
> int bpf_obj_get_user(const char __user *pathname, int flags);
> @@ -2308,6 +2314,11 @@ static inline void bpf_link_put(struct bpf_link
> *link)
> {
> }
> +static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static inline int bpf_obj_get_user(const char __user *pathname, int
> flags)
> {
> return -EOPNOTSUPP;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 17afd2b35ee5..cd0ff39981e8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1033,6 +1033,7 @@ enum bpf_attach_type {
> BPF_PERF_EVENT,
> BPF_TRACE_KPROBE_MULTI,
> BPF_LSM_CGROUP,
> + BPF_STRUCT_OPS,
> __MAX_BPF_ATTACH_TYPE
> };
> @@ -1266,6 +1267,9 @@ enum {
> /* Create a map that is suitable to be an inner map with dynamic max
> entries */
> BPF_F_INNER_MAP = (1U << 12),
> +
> +/* Create a map that will be registered/unregesitered by the backed
> bpf_link */
> + BPF_F_LINK = (1U << 13),
> };
> /* Flags for BPF_PROG_QUERY. */
> @@ -1507,7 +1511,10 @@ union bpf_attr {
> } task_fd_query;
> struct { /* struct used by BPF_LINK_CREATE command */
> - __u32 prog_fd; /* eBPF program to attach */
> + union {
> + __u32 prog_fd; /* eBPF program to attach */
> + __u32 map_fd; /* eBPF struct_ops to attach */
> + };
> union {
> __u32 target_fd; /* object to attach to */
> __u32 target_ifindex; /* target ifindex */
> @@ -6354,6 +6361,9 @@ struct bpf_link_info {
> struct {
> __u32 ifindex;
> } xdp;
> + struct {
> + __u32 map_id;
> + } struct_ops;
> };
> } __attribute__((aligned(8)));
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index bba03b6b010b..9ec675576d97 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -14,6 +14,7 @@
> enum bpf_struct_ops_state {
> BPF_STRUCT_OPS_STATE_INIT,
> + BPF_STRUCT_OPS_STATE_READY,
> BPF_STRUCT_OPS_STATE_INUSE,
> BPF_STRUCT_OPS_STATE_TOBEFREE,
> };
> @@ -494,11 +495,19 @@ static int bpf_struct_ops_map_update_elem(struct
> bpf_map *map, void *key,
> *(unsigned long *)(udata + moff) = prog->aux->id;
> }
> - bpf_map_inc(map);
> -
> set_memory_rox((long)st_map->image, 1);
> + if (st_map->map.map_flags & BPF_F_LINK) {
> + /* Let bpf_link handle registration & unregistration.
> + *
> + * Pair with smp_load_acquire() during lookup_elem().
> + */
> + smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
Any reason not to create a link here and return to the user? (without
extra link_create API)
> + goto unlock;
> + }
> +
> err = st_ops->reg(kdata);
> if (likely(!err)) {
> + bpf_map_inc(map);
> /* Pair with smp_load_acquire() during lookup_elem().
> * It ensures the above udata updates (e.g. prog->aux->id)
> * can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
> @@ -514,7 +523,6 @@ static int bpf_struct_ops_map_update_elem(struct
> bpf_map *map, void *key,
> */
> set_memory_nx((long)st_map->image, 1);
> set_memory_rw((long)st_map->image, 1);
> - bpf_map_put(map);
> reset_unlock:
> bpf_struct_ops_map_put_progs(st_map);
> @@ -532,10 +540,15 @@ static int bpf_struct_ops_map_delete_elem(struct
> bpf_map *map, void *key)
> struct bpf_struct_ops_map *st_map;
> st_map = (struct bpf_struct_ops_map *)map;
> + if (st_map->map.map_flags & BPF_F_LINK)
> + return -EOPNOTSUPP;
> +
> prev_state = cmpxchg(&st_map->kvalue.state,
> BPF_STRUCT_OPS_STATE_INUSE,
> BPF_STRUCT_OPS_STATE_TOBEFREE);
> switch (prev_state) {
> + case BPF_STRUCT_OPS_STATE_READY:
> + return -EOPNOTSUPP;
> case BPF_STRUCT_OPS_STATE_INUSE:
> st_map->st_ops->unreg(&st_map->kvalue.data);
> bpf_map_put(map);
> @@ -618,7 +631,7 @@ static void bpf_struct_ops_map_free_rcu(struct
> bpf_map *map)
> static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
> {
> if (attr->key_size != sizeof(unsigned int) || attr->max_entries != 1 ||
> - attr->map_flags || !attr->btf_vmlinux_value_type_id)
> + (attr->map_flags & ~BPF_F_LINK) || !attr->btf_vmlinux_value_type_id)
> return -EINVAL;
> return 0;
> }
> @@ -714,3 +727,100 @@ void bpf_struct_ops_put(const void *kdata)
> bpf_map_put(&st_map->map);
> }
> +
> +static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_struct_ops_map *st_map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + if (st_link->map) {
> + st_map = (struct bpf_struct_ops_map *)st_link->map;
> + st_map->st_ops->unreg(&st_map->kvalue.data);
> + bpf_map_put(st_link->map);
> + }
> + kfree(st_link);
> +}
> +
> +static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link
> *link,
> + struct seq_file *seq)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_map *map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + rcu_read_lock_trace();
> + map = rcu_dereference(st_link->map);
> + if (map)
> + seq_printf(seq, "map_id:\t%d\n", map->id);
> + rcu_read_unlock_trace();
> +}
> +
> +static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link
> *link,
> + struct bpf_link_info *info)
> +{
> + struct bpf_struct_ops_link *st_link;
> + struct bpf_map *map;
> +
> + st_link = container_of(link, struct bpf_struct_ops_link, link);
> + rcu_read_lock_trace();
> + map = rcu_dereference(st_link->map);
> + if (map)
> + info->struct_ops.map_id = map->id;
> + rcu_read_unlock_trace();
> + return 0;
> +}
> +
> +static const struct bpf_link_ops bpf_struct_ops_map_lops = {
> + .dealloc = bpf_struct_ops_map_link_dealloc,
> + .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
> + .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
> +};
> +
> +int bpf_struct_ops_link_create(union bpf_attr *attr)
> +{
> + struct bpf_struct_ops_link *link = NULL;
> + struct bpf_link_primer link_primer;
> + struct bpf_struct_ops_map *st_map;
> + struct bpf_map *map;
> + int err;
> +
> + map = bpf_map_get(attr->link_create.map_fd);
> + if (!map)
> + return -EINVAL;
> +
> + st_map = (struct bpf_struct_ops_map *)map;
> +
> + if (map->map_type != BPF_MAP_TYPE_STRUCT_OPS || !(map->map_flags &
> BPF_F_LINK) ||
> + /* Pair with smp_store_release() during map_update */
> + smp_load_acquire(&st_map->kvalue.state) !=
> BPF_STRUCT_OPS_STATE_READY) {
> + err = -EINVAL;
> + goto err_out;
> + }
> +
> + link = kzalloc(sizeof(*link), GFP_USER);
> + if (!link) {
> + err = -ENOMEM;
> + goto err_out;
> + }
> + bpf_link_init(&link->link, BPF_LINK_TYPE_STRUCT_OPS,
> &bpf_struct_ops_map_lops, NULL);
> + link->map = map;
> +
> + err = bpf_link_prime(&link->link, &link_primer);
> + if (err)
> + goto err_out;
> +
> + err = st_map->st_ops->reg(st_map->kvalue.data);
> + if (err) {
> + bpf_link_cleanup(&link_primer);
> + goto err_out;
> + }
> +
> + return bpf_link_settle(&link_primer);
> +
> +err_out:
> + bpf_map_put(map);
> + kfree(link);
> + return err;
> +}
> +
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 358a0e40555e..3db4938212d6 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2743,10 +2743,11 @@ void bpf_link_inc(struct bpf_link *link)
> static void bpf_link_free(struct bpf_link *link)
> {
> bpf_link_free_id(link->id);
> + /* detach BPF program, clean up used resources */
> if (link->prog) {
> - /* detach BPF program, clean up used resources */
> link->ops->release(link);
> bpf_prog_put(link->prog);
> + /* The struct_ops links clean up map by them-selves. */
> }
> /* free bpf_link and its containing memory */
> link->ops->dealloc(link);
> @@ -2802,16 +2803,19 @@ static void bpf_link_show_fdinfo(struct seq_file
> *m, struct file *filp)
> const struct bpf_prog *prog = link->prog;
> char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
> - bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
> seq_printf(m,
> "link_type:\t%s\n"
> - "link_id:\t%u\n"
> - "prog_tag:\t%s\n"
> - "prog_id:\t%u\n",
> + "link_id:\t%u\n",
> bpf_link_type_strs[link->type],
> - link->id,
> - prog_tag,
> - prog->aux->id);
> + link->id);
> + if (prog) {
> + bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
> + seq_printf(m,
> + "prog_tag:\t%s\n"
> + "prog_id:\t%u\n",
> + prog_tag,
> + prog->aux->id);
> + }
> if (link->ops->show_fdinfo)
> link->ops->show_fdinfo(link, m);
> }
> @@ -4286,7 +4290,8 @@ static int bpf_link_get_info_by_fd(struct file
> *file,
> info.type = link->type;
> info.id = link->id;
> - info.prog_id = link->prog->aux->id;
> + if (link->prog)
> + info.prog_id = link->prog->aux->id;
> if (link->ops->fill_link_info) {
> err = link->ops->fill_link_info(link, &info);
> @@ -4549,6 +4554,9 @@ static int link_create(union bpf_attr *attr,
> bpfptr_t uattr)
> if (CHECK_ATTR(BPF_LINK_CREATE))
> return -EINVAL;
> + if (attr->link_create.attach_type == BPF_STRUCT_OPS)
> + return bpf_struct_ops_link_create(attr);
> +
> prog = bpf_prog_get(attr->link_create.prog_fd);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
> diff --git a/tools/include/uapi/linux/bpf.h
> b/tools/include/uapi/linux/bpf.h
> index 17afd2b35ee5..cd0ff39981e8 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1033,6 +1033,7 @@ enum bpf_attach_type {
> BPF_PERF_EVENT,
> BPF_TRACE_KPROBE_MULTI,
> BPF_LSM_CGROUP,
> + BPF_STRUCT_OPS,
> __MAX_BPF_ATTACH_TYPE
> };
> @@ -1266,6 +1267,9 @@ enum {
> /* Create a map that is suitable to be an inner map with dynamic max
> entries */
> BPF_F_INNER_MAP = (1U << 12),
> +
> +/* Create a map that will be registered/unregesitered by the backed
> bpf_link */
> + BPF_F_LINK = (1U << 13),
> };
> /* Flags for BPF_PROG_QUERY. */
> @@ -1507,7 +1511,10 @@ union bpf_attr {
> } task_fd_query;
> struct { /* struct used by BPF_LINK_CREATE command */
> - __u32 prog_fd; /* eBPF program to attach */
> + union {
> + __u32 prog_fd; /* eBPF program to attach */
> + __u32 map_fd; /* eBPF struct_ops to attach */
> + };
> union {
> __u32 target_fd; /* object to attach to */
> __u32 target_ifindex; /* target ifindex */
> @@ -6354,6 +6361,9 @@ struct bpf_link_info {
> struct {
> __u32 ifindex;
> } xdp;
> + struct {
> + __u32 map_id;
> + } struct_ops;
> };
> } __attribute__((aligned(8)));
> --
> 2.30.2
next prev parent reply other threads:[~2023-03-06 20:23 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-03 1:21 [PATCH bpf-next v3 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 1/8] bpf: Maintain the refcount of struct_ops maps directly Kui-Feng Lee
2023-03-06 20:10 ` Stanislav Fomichev
2023-03-06 21:45 ` Kui-Feng Lee
2023-03-06 23:16 ` Martin KaFai Lau
2023-03-06 23:54 ` Kui-Feng Lee
2023-03-07 0:36 ` Martin KaFai Lau
2023-03-03 1:21 ` [PATCH bpf-next v3 2/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-06 20:23 ` Stanislav Fomichev [this message]
2023-03-06 22:02 ` Kui-Feng Lee
2023-03-07 2:11 ` Martin KaFai Lau
2023-03-07 18:04 ` Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 3/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
2023-03-07 2:17 ` Martin KaFai Lau
2023-03-07 19:17 ` Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
2023-03-03 1:21 ` [PATCH bpf-next v3 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
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=ZAZLuF/ZfbX3RTX1@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=kernel-team@meta.com \
--cc=kuifeng@meta.com \
--cc=martin.lau@linux.dev \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.