From: Kui-Feng Lee <sinquersw@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
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,
sdf@google.com
Subject: Re: [PATCH bpf-next v4 6/9] bpf: Update the struct_ops of a bpf_link.
Date: Wed, 8 Mar 2023 08:27:36 -0800 [thread overview]
Message-ID: <23503351-16ee-e84c-33c4-43241614f7fb@gmail.com> (raw)
In-Reply-To: <CAEf4BzbK8s+VFG5HefydD7CRLzkRFKg-Er0PKV_-C2-yttfXzA@mail.gmail.com>
On 3/7/23 16:49, Andrii Nakryiko wrote:
> On Tue, Mar 7, 2023 at 3:33 PM Kui-Feng Lee <kuifeng@meta.com> wrote:
>>
>> By improving the BPF_LINK_UPDATE command of bpf(), it should allow you
>> to conveniently switch between different struct_ops on a single
>> bpf_link. This would enable smoother transitions from one struct_ops
>> to another.
>>
>> The struct_ops maps passing along with BPF_LINK_UPDATE should have the
>> BPF_F_LINK flag.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>> include/linux/bpf.h | 1 +
>> include/uapi/linux/bpf.h | 8 ++++--
>> kernel/bpf/bpf_struct_ops.c | 46 ++++++++++++++++++++++++++++++++++
>> kernel/bpf/syscall.c | 43 ++++++++++++++++++++++++++++---
>> tools/include/uapi/linux/bpf.h | 7 +++++-
>> 5 files changed, 98 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 047d2c6aba88..2b5f150e370e 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1405,6 +1405,7 @@ struct bpf_link_ops {
>> void (*show_fdinfo)(const struct bpf_link *link, struct seq_file *seq);
>> int (*fill_link_info)(const struct bpf_link *link,
>> struct bpf_link_info *info);
>> + int (*update_map)(struct bpf_link *link, struct bpf_map *new_map);
>> };
>>
>> struct bpf_tramp_link {
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index eb3e435c5303..999e199ebe06 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1555,8 +1555,12 @@ union bpf_attr {
>>
>> struct { /* struct used by BPF_LINK_UPDATE command */
>> __u32 link_fd; /* link fd */
>> - /* new program fd to update link with */
>> - __u32 new_prog_fd;
>> + union {
>> + /* new program fd to update link with */
>> + __u32 new_prog_fd;
>> + /* new struct_ops map fd to update link with */
>> + __u32 new_map_fd;
>> + };
>> __u32 flags; /* extra flags */
>> /* expected link's program fd; is specified only if
>> * BPF_F_REPLACE flag is set in flags */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index c71c8d73c7ad..2b850ce11617 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -759,10 +759,56 @@ static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>> return 0;
>> }
>>
>> +static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map *new_map)
>> +{
>> + struct bpf_struct_ops_value *kvalue;
>> + struct bpf_struct_ops_map *st_map, *old_st_map;
>> + struct bpf_struct_ops_link *st_link;
>> + struct bpf_map *old_map;
>> + int err = 0;
>> +
>> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
>> + !(new_map->map_flags & BPF_F_LINK))
>> + return -EINVAL;
>> +
>> + mutex_lock(&update_mutex);
>> +
>> + st_link = container_of(link, struct bpf_struct_ops_link, link);
>> +
>> + /* The new and old struct_ops must be the same type. */
>> + st_map = container_of(new_map, struct bpf_struct_ops_map, map);
>> +
>> + old_map = st_link->map;
>> + old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
>> + if (st_map->st_ops != old_st_map->st_ops ||
>> + /* 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;
>> + }
>> +
>> + kvalue = &st_map->kvalue;
>> +
>> + err = st_map->st_ops->update(kvalue->data, old_st_map->kvalue.data);
>> + if (err)
>> + goto err_out;
>> +
>> + bpf_map_inc(new_map);
>> + rcu_assign_pointer(st_link->map, new_map);
>> +
>> + bpf_map_put(old_map);
>> +
>> +err_out:
>> + mutex_unlock(&update_mutex);
>> +
>> + return err;
>> +}
>> +
>> 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,
>> + .update_map = bpf_struct_ops_map_link_update,
>> };
>>
>> int bpf_struct_ops_link_create(union bpf_attr *attr)
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 25b044fdd82b..94ab1336ff41 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -4646,6 +4646,30 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>> return ret;
>> }
>>
>> +static int link_update_map(struct bpf_link *link, union bpf_attr *attr)
>> +{
>> + struct bpf_map *new_map;
>> + int ret = 0;
>> +
>> + new_map = bpf_map_get(attr->link_update.new_map_fd);
>> + if (IS_ERR(new_map))
>> + return -EINVAL;
>> +
>> + if (new_map->map_type != BPF_MAP_TYPE_STRUCT_OPS) {
>> + ret = -EINVAL;
>> + goto out_put_map;
>> + }
>> +
>> + if (link->ops->update_map)
>> + ret = link->ops->update_map(link, new_map);
>> + else
>> + ret = -EINVAL;
>> +
>> +out_put_map:
>> + bpf_map_put(new_map);
>> + return ret;
>> +}
>> +
>> #define BPF_LINK_UPDATE_LAST_FIELD link_update.old_prog_fd
>>
>> static int link_update(union bpf_attr *attr)
>> @@ -4658,14 +4682,25 @@ static int link_update(union bpf_attr *attr)
>> if (CHECK_ATTR(BPF_LINK_UPDATE))
>> return -EINVAL;
>>
>> - flags = attr->link_update.flags;
>> - if (flags & ~BPF_F_REPLACE)
>> - return -EINVAL;
>> -
>> link = bpf_link_get_from_fd(attr->link_update.link_fd);
>> if (IS_ERR(link))
>> return PTR_ERR(link);
>>
>> + flags = attr->link_update.flags;
>> +
>> + if (link->ops->update_map) {
>> + if (flags) /* always replace the existing one */
>> + ret = -EINVAL;
>> + else
>> + ret = link_update_map(link, attr);
>> + goto out_put_link;
>
> umm... BPF_F_REPLACE for link_update is specifying "update only if
> current prog fd matches what I specify", let's not ignore it for
> struct_ops. This will create a deviation in behavior unnecessarily.
> Please keep it consistent.
Ok!
>
>
>> + }
>> +
>> + if (flags & ~BPF_F_REPLACE) {
>> + ret = -EINVAL;
>> + goto out_put_link;
>> + }
>> +
>> new_prog = bpf_prog_get(attr->link_update.new_prog_fd);
>> if (IS_ERR(new_prog)) {
>> ret = PTR_ERR(new_prog);
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index cd0ff39981e8..259b8ab4f54e 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -1556,7 +1556,12 @@ union bpf_attr {
>> struct { /* struct used by BPF_LINK_UPDATE command */
>> __u32 link_fd; /* link fd */
>> /* new program fd to update link with */
>> - __u32 new_prog_fd;
>> + union {
>> + /* new program fd to update link with */
>> + __u32 new_prog_fd;
>> + /* new struct_ops map fd to update link with */
>> + __u32 new_map_fd;
>> + };
>> __u32 flags; /* extra flags */
>> /* expected link's program fd; is specified only if
>> * BPF_F_REPLACE flag is set in flags */
>> --
>> 2.34.1
>>
next prev parent reply other threads:[~2023-03-08 16:28 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 23:32 [PATCH bpf-next v4 0/9] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-07 23:32 ` [PATCH bpf-next v4 1/9] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 2/9] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-08 0:37 ` Andrii Nakryiko
2023-03-08 1:11 ` Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 3/9] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 4/9] bpf: Validate kdata of a struct_ops before transiting to READY Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 5/9] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
2023-03-08 0:46 ` Andrii Nakryiko
2023-03-08 3:33 ` Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 6/9] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
2023-03-08 0:49 ` Andrii Nakryiko
2023-03-08 16:27 ` Kui-Feng Lee [this message]
2023-03-07 23:33 ` [PATCH bpf-next v4 7/9] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
2023-03-08 0:53 ` Andrii Nakryiko
2023-03-08 1:45 ` Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 8/9] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
2023-03-08 1:07 ` Andrii Nakryiko
2023-03-08 4:23 ` Kui-Feng Lee
2023-03-07 23:33 ` [PATCH bpf-next v4 9/9] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
2023-03-08 1:10 ` Andrii Nakryiko
2023-03-08 15:58 ` Kui-Feng Lee
2023-03-08 17:18 ` Andrii Nakryiko
2023-03-08 18:10 ` Kui-Feng Lee
2023-03-08 18:43 ` Andrii Nakryiko
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=23503351-16ee-e84c-33c4-43241614f7fb@gmail.com \
--to=sinquersw@gmail.com \
--cc=andrii.nakryiko@gmail.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=sdf@google.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