* [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
@ 2020-09-16 22:46 Yonghong Song
2020-09-16 23:00 ` Martin KaFai Lau
2020-09-21 22:47 ` Alexei Starovoitov
0 siblings, 2 replies; 3+ messages in thread
From: Yonghong Song @ 2020-09-16 22:46 UTC (permalink / raw)
To: bpf, netdev
Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team,
Martin KaFai Lau, Song Liu
If a bucket contains a lot of sockets, during bpf_iter traversing
a bucket, concurrent userspace bpf_map_update_elem() and
bpf program bpf_sk_storage_{get,delete}() may experience
some undesirable delays as they will compete with bpf_iter
for bucket lock.
Note that the number of buckets for bpf_sk_storage_map
is roughly the same as the number of cpus. So if there
are lots of sockets in the system, each bucket could
contain lots of sockets.
Different actual use cases may experience different delays.
Here, using selftest bpf_iter subtest bpf_sk_storage_map,
I hacked the kernel with ktime_get_mono_fast_ns()
to collect the time when a bucket was locked
during bpf_iter prog traversing that bucket. This way,
the maximum incurred delay was measured w.r.t. the
number of elements in a bucket.
# elems in each bucket delay(ns)
64 17000
256 72512
2048 875246
The potential delays will be further increased if
we have even more elemnts in a bucket. Using rcu_read_lock()
is a reasonable compromise here. It may lose some precision, e.g.,
access stale sockets, but it will not hurt performance of
bpf program or user space application which also tries
to get/delete or update map elements.
Cc: Martin KaFai Lau <kafai@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
net/core/bpf_sk_storage.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
Changelog:
v3 -> v4:
- use rcu_dereference/hlist_next_rcu for hlist_entry_safe. (Martin)
v2 -> v3:
- fix a bug hlist_for_each_entry() => hlist_for_each_entry_rcu(). (Martin)
- use rcu_dereference() instead of rcu_dereference_raw() for lockdep checking. (Martin)
v1 -> v2:
- added some performance number. (Song)
- tried to silence some sparse complains. but still has some left like
context imbalance in "..." - different lock contexts for basic block
which the code is too hard for sparse to analyze. (Jakub)
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 4a86ea34f29e..6b6ba874061c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -678,6 +678,7 @@ struct bpf_iter_seq_sk_storage_map_info {
static struct bpf_local_storage_elem *
bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
struct bpf_local_storage_elem *prev_selem)
+ __acquires(RCU) __releases(RCU)
{
struct bpf_local_storage *sk_storage;
struct bpf_local_storage_elem *selem;
@@ -696,16 +697,16 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
selem = prev_selem;
count = 0;
while (selem) {
- selem = hlist_entry_safe(selem->map_node.next,
+ selem = hlist_entry_safe(rcu_dereference(hlist_next_rcu(&selem->map_node)),
struct bpf_local_storage_elem, map_node);
if (!selem) {
/* not found, unlock and go to the next bucket */
b = &smap->buckets[bucket_id++];
- raw_spin_unlock_bh(&b->lock);
+ rcu_read_unlock();
skip_elems = 0;
break;
}
- sk_storage = rcu_dereference_raw(selem->local_storage);
+ sk_storage = rcu_dereference(selem->local_storage);
if (sk_storage) {
info->skip_elems = skip_elems + count;
return selem;
@@ -715,10 +716,10 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
for (i = bucket_id; i < (1U << smap->bucket_log); i++) {
b = &smap->buckets[i];
- raw_spin_lock_bh(&b->lock);
+ rcu_read_lock();
count = 0;
- hlist_for_each_entry(selem, &b->list, map_node) {
- sk_storage = rcu_dereference_raw(selem->local_storage);
+ hlist_for_each_entry_rcu(selem, &b->list, map_node) {
+ sk_storage = rcu_dereference(selem->local_storage);
if (sk_storage && count >= skip_elems) {
info->bucket_id = i;
info->skip_elems = count;
@@ -726,7 +727,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
}
count++;
}
- raw_spin_unlock_bh(&b->lock);
+ rcu_read_unlock();
skip_elems = 0;
}
@@ -785,7 +786,7 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
ctx.meta = &meta;
ctx.map = info->map;
if (selem) {
- sk_storage = rcu_dereference_raw(selem->local_storage);
+ sk_storage = rcu_dereference(selem->local_storage);
ctx.sk = sk_storage->owner;
ctx.value = SDATA(selem)->data;
}
@@ -801,18 +802,12 @@ static int bpf_sk_storage_map_seq_show(struct seq_file *seq, void *v)
}
static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v)
+ __releases(RCU)
{
- struct bpf_iter_seq_sk_storage_map_info *info = seq->private;
- struct bpf_local_storage_map *smap;
- struct bpf_local_storage_map_bucket *b;
-
- if (!v) {
+ if (!v)
(void)__bpf_sk_storage_map_seq_show(seq, v);
- } else {
- smap = (struct bpf_local_storage_map *)info->map;
- b = &smap->buckets[info->bucket_id];
- raw_spin_unlock_bh(&b->lock);
- }
+ else
+ rcu_read_unlock();
}
static int bpf_iter_init_sk_storage_map(void *priv_data,
--
2.24.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
2020-09-16 22:46 [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator Yonghong Song
@ 2020-09-16 23:00 ` Martin KaFai Lau
2020-09-21 22:47 ` Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Martin KaFai Lau @ 2020-09-16 23:00 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, kernel-team,
Song Liu
On Wed, Sep 16, 2020 at 03:46:45PM -0700, Yonghong Song wrote:
> If a bucket contains a lot of sockets, during bpf_iter traversing
> a bucket, concurrent userspace bpf_map_update_elem() and
> bpf program bpf_sk_storage_{get,delete}() may experience
> some undesirable delays as they will compete with bpf_iter
> for bucket lock.
>
> Note that the number of buckets for bpf_sk_storage_map
> is roughly the same as the number of cpus. So if there
> are lots of sockets in the system, each bucket could
> contain lots of sockets.
>
> Different actual use cases may experience different delays.
> Here, using selftest bpf_iter subtest bpf_sk_storage_map,
> I hacked the kernel with ktime_get_mono_fast_ns()
> to collect the time when a bucket was locked
> during bpf_iter prog traversing that bucket. This way,
> the maximum incurred delay was measured w.r.t. the
> number of elements in a bucket.
> # elems in each bucket delay(ns)
> 64 17000
> 256 72512
> 2048 875246
>
> The potential delays will be further increased if
> we have even more elemnts in a bucket. Using rcu_read_lock()
> is a reasonable compromise here. It may lose some precision, e.g.,
> access stale sockets, but it will not hurt performance of
> bpf program or user space application which also tries
> to get/delete or update map elements.
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator
2020-09-16 22:46 [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator Yonghong Song
2020-09-16 23:00 ` Martin KaFai Lau
@ 2020-09-21 22:47 ` Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2020-09-21 22:47 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Martin KaFai Lau, Song Liu
On Thu, Sep 17, 2020 at 8:46 AM Yonghong Song <yhs@fb.com> wrote:
>
> If a bucket contains a lot of sockets, during bpf_iter traversing
> a bucket, concurrent userspace bpf_map_update_elem() and
> bpf program bpf_sk_storage_{get,delete}() may experience
> some undesirable delays as they will compete with bpf_iter
> for bucket lock.
>
> Note that the number of buckets for bpf_sk_storage_map
> is roughly the same as the number of cpus. So if there
> are lots of sockets in the system, each bucket could
> contain lots of sockets.
>
> Different actual use cases may experience different delays.
> Here, using selftest bpf_iter subtest bpf_sk_storage_map,
> I hacked the kernel with ktime_get_mono_fast_ns()
> to collect the time when a bucket was locked
> during bpf_iter prog traversing that bucket. This way,
> the maximum incurred delay was measured w.r.t. the
> number of elements in a bucket.
> # elems in each bucket delay(ns)
> 64 17000
> 256 72512
> 2048 875246
>
> The potential delays will be further increased if
> we have even more elemnts in a bucket. Using rcu_read_lock()
> is a reasonable compromise here. It may lose some precision, e.g.,
> access stale sockets, but it will not hurt performance of
> bpf program or user space application which also tries
> to get/delete or update map elements.
>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> net/core/bpf_sk_storage.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> Changelog:
> v3 -> v4:
> - use rcu_dereference/hlist_next_rcu for hlist_entry_safe. (Martin)
Applied. Thanks
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-21 22:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-16 22:46 [PATCH bpf-next v4] bpf: using rcu_read_lock for bpf_sk_storage_map iterator Yonghong Song
2020-09-16 23:00 ` Martin KaFai Lau
2020-09-21 22:47 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).