From: Kui-Feng Lee <sinquersw@gmail.com>
To: Martin KaFai Lau <martin.lau@linux.dev>, Kui-Feng Lee <kuifeng@meta.com>
Cc: bpf@vger.kernel.org, ast@kernel.org, song@kernel.org,
kernel-team@meta.com, andrii@kernel.org, sdf@google.com
Subject: Re: [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt.
Date: Wed, 8 Mar 2023 14:30:49 -0800 [thread overview]
Message-ID: <1bea11ec-4046-67ec-8e6b-e5c9baaf3082@gmail.com> (raw)
In-Reply-To: <0686c1bc-c216-6e76-a9e7-65f1c102d4ab@linux.dev>
On 3/8/23 10:30, Martin KaFai Lau wrote:
> On 3/7/23 4:50 PM, Kui-Feng Lee wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 6792a7940e1e..50cfc2388cbc 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -78,6 +78,7 @@ struct bpf_map_ops {
>> struct bpf_map *(*map_alloc)(union bpf_attr *attr);
>> void (*map_release)(struct bpf_map *map, struct file *map_file);
>> void (*map_free)(struct bpf_map *map);
>> + void (*map_free_rcu)(struct bpf_map *map);
>
> This is no longer needed...
>
>> int (*map_get_next_key)(struct bpf_map *map, void *key, void
>> *next_key);
>> void (*map_release_uref)(struct bpf_map *map);
>> void *(*map_lookup_elem_sys_only)(struct bpf_map *map, void *key);
>> @@ -1934,6 +1935,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd);
>> struct bpf_map *__bpf_map_get(struct fd f);
>> void bpf_map_inc(struct bpf_map *map);
>> void bpf_map_inc_with_uref(struct bpf_map *map);
>> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
>> struct bpf_map * __must_check bpf_map_inc_not_zero(struct bpf_map
>> *map);
>> void bpf_map_put_with_uref(struct bpf_map *map);
>> void bpf_map_put(struct bpf_map *map);
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 38903fb52f98..9e097fcc9cf4 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -58,6 +58,8 @@ struct bpf_struct_ops_map {
>> struct bpf_struct_ops_value kvalue;
>> };
>> +static DEFINE_MUTEX(update_mutex);
>
> Please address or reply to the earlier review comments. Stan had
> mentioned in v3 that this mutex is unused in this patch.
>
> This is only used in patch 5 of this set. Please move it there.
I will address this.
>
>> +
>> #define VALUE_PREFIX "bpf_struct_ops_"
>> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>> @@ -249,6 +251,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct
>> bpf_map *map, void *key,
>> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> struct bpf_struct_ops_value *uvalue, *kvalue;
>> enum bpf_struct_ops_state state;
>> + s64 refcnt;
>> if (unlikely(*(u32 *)key != 0))
>> return -ENOENT;
>> @@ -267,7 +270,9 @@ int bpf_struct_ops_map_sys_lookup_elem(struct
>> bpf_map *map, void *key,
>> uvalue = value;
>> memcpy(uvalue, st_map->uvalue, map->value_size);
>> uvalue->state = state;
>> - refcount_set(&uvalue->refcnt, refcount_read(&kvalue->refcnt));
>> +
>> + refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
>> + refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
>
> Please explain a few words that why it will work good enough and no need
> to be very accurate (eg. it is for introspection purpose to give an idea
> on how many refcnts are held by a subsystem, eg tcp_sock ...). This was
> also a comment given in v3.
Sure
>
>> return 0;
>> }
>> @@ -491,7 +496,6 @@ static int bpf_struct_ops_map_update_elem(struct
>> bpf_map *map, void *key,
>> *(unsigned long *)(udata + moff) = prog->aux->id;
>> }
>> - refcount_set(&kvalue->refcnt, 1);
>> bpf_map_inc(map);
>> set_memory_rox((long)st_map->image, 1);
>> @@ -536,8 +540,7 @@ static int bpf_struct_ops_map_delete_elem(struct
>> bpf_map *map, void *key)
>> switch (prev_state) {
>> case BPF_STRUCT_OPS_STATE_INUSE:
>> st_map->st_ops->unreg(&st_map->kvalue.data);
>> - if (refcount_dec_and_test(&st_map->kvalue.refcnt))
>> - bpf_map_put(map);
>> + bpf_map_put(map);
>> return 0;
>> case BPF_STRUCT_OPS_STATE_TOBEFREE:
>> return -EINPROGRESS;
>> @@ -574,6 +577,19 @@ static void bpf_struct_ops_map_free(struct
>> bpf_map *map)
>> {
>> struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> + /* The struct_ops's function may switch to another struct_ops.
>> + *
>> + * For example, bpf_tcp_cc_x->init() may switch to
>> + * another tcp_cc_y by calling
>> + * setsockopt(TCP_CONGESTION, "tcp_cc_y").
>> + * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
>> + * and its refcount may reach 0 which then free its
>> + * trampoline image while tcp_cc_x is still running.
>> + *
>> + * Thus, a rcu grace period is needed here.
>> + */
>> + synchronize_rcu();
>> +
>> if (st_map->links)
>> bpf_struct_ops_map_put_progs(st_map);
>> bpf_map_area_free(st_map->links);
>> @@ -676,41 +692,23 @@ const struct bpf_map_ops bpf_struct_ops_map_ops = {
>> bool bpf_struct_ops_get(const void *kdata)
>> {
>> struct bpf_struct_ops_value *kvalue;
>> + struct bpf_struct_ops_map *st_map;
>> + struct bpf_map *map;
>> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
>> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>> - return refcount_inc_not_zero(&kvalue->refcnt);
>> -}
>> -
>> -static void bpf_struct_ops_put_rcu(struct rcu_head *head)
>> -{
>> - struct bpf_struct_ops_map *st_map;
>> -
>> - st_map = container_of(head, struct bpf_struct_ops_map, rcu);
>> - bpf_map_put(&st_map->map);
>> + map = __bpf_map_inc_not_zero(&st_map->map, false);
>> + return !IS_ERR(map);
>> }
>> void bpf_struct_ops_put(const void *kdata)
>> {
>> struct bpf_struct_ops_value *kvalue;
>> + struct bpf_struct_ops_map *st_map;
>> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
>> - if (refcount_dec_and_test(&kvalue->refcnt)) {
>> - struct bpf_struct_ops_map *st_map;
>> -
>> - st_map = container_of(kvalue, struct bpf_struct_ops_map,
>> - kvalue);
>> - /* The struct_ops's function may switch to another struct_ops.
>> - *
>> - * For example, bpf_tcp_cc_x->init() may switch to
>> - * another tcp_cc_y by calling
>> - * setsockopt(TCP_CONGESTION, "tcp_cc_y").
>> - * During the switch, bpf_struct_ops_put(tcp_cc_x) is called
>> - * and its map->refcnt may reach 0 which then free its
>> - * trampoline image while tcp_cc_x is still running.
>> - *
>> - * Thus, a rcu grace period is needed here.
>> - */
>> - call_rcu(&st_map->rcu, bpf_struct_ops_put_rcu);
>> - }
>> + st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>> +
>> + bpf_map_put(&st_map->map);
>> }
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index f406dfa13792..03273cddd6bd 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1288,7 +1288,7 @@ struct bpf_map *bpf_map_get_with_uref(u32 ufd)
>> }
>> /* map_idr_lock should have been held */
>
> This comment needs to change, eg.
> map_idr_lock should have been held or the map is protected by rcu gp....
>
Sure.
>> -static struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map,
>> bool uref)
>> +struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref)
>> {
>> int refold;
>
next prev parent reply other threads:[~2023-03-08 22:30 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-08 0:50 [PATCH bpf-next v5 0/8] Transit between BPF TCP congestion controls Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 1/8] bpf: Retire the struct_ops map kvalue->refcnt Kui-Feng Lee
2023-03-08 18:30 ` Martin KaFai Lau
2023-03-08 22:30 ` Kui-Feng Lee [this message]
2023-03-08 0:50 ` [PATCH bpf-next v5 2/8] net: Update an existing TCP congestion control algorithm Kui-Feng Lee
2023-03-08 18:59 ` Martin KaFai Lau
2023-03-08 22:51 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 3/8] bpf: Create links for BPF struct_ops maps Kui-Feng Lee
2023-03-08 15:01 ` kernel test robot
2023-03-08 15:32 ` kernel test robot
2023-03-08 16:03 ` kernel test robot
2023-03-08 20:04 ` Martin KaFai Lau
2023-03-08 23:46 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 4/8] libbpf: Create a bpf_link in bpf_map__attach_struct_ops() Kui-Feng Lee
2023-03-08 21:42 ` Martin KaFai Lau
2023-03-09 0:22 ` Kui-Feng Lee
2023-03-09 17:09 ` Martin KaFai Lau
2023-03-09 18:16 ` Kui-Feng Lee
2023-03-09 18:19 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 5/8] bpf: Update the struct_ops of a bpf_link Kui-Feng Lee
2023-03-08 22:21 ` Martin KaFai Lau
2023-03-09 3:09 ` Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 6/8] libbpf: Update a bpf_link with another struct_ops Kui-Feng Lee
2023-03-08 22:32 ` Martin KaFai Lau
2023-03-08 0:50 ` [PATCH bpf-next v5 7/8] libbpf: Use .struct_ops.link section to indicate a struct_ops with a link Kui-Feng Lee
2023-03-08 0:50 ` [PATCH bpf-next v5 8/8] selftests/bpf: Test switching TCP Congestion Control algorithms Kui-Feng Lee
2023-03-08 23:10 ` Martin KaFai Lau
2023-03-09 3:34 ` 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=1bea11ec-4046-67ec-8e6b-e5c9baaf3082@gmail.com \
--to=sinquersw@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