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.
Unified · Split
| @@ -674,6 +674,7 @@ struct bpf_iter_seq_sk_storage_map_info { · 673 unmodified lines | |||
| 674 | static struct bpf_local_storage_elem * | 674 | static struct bpf_local_storage_elem * |
| 675 | bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info, | 675 | bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info, |
| 676 | struct bpf_local_storage_elem *prev_selem) | 676 | struct bpf_local_storage_elem *prev_selem) |
| 677 | __acquires(RCU) __releases(RCU) | ||
| 677 | { | 678 | { |
| 678 | struct bpf_local_storage *sk_storage; | 679 | struct bpf_local_storage *sk_storage; |
| 679 | struct bpf_local_storage_elem *selem; | 680 | struct bpf_local_storage_elem *selem; |
| @@ -692,16 +693,16 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info, · 12 unmodified lines | |||
| 692 | selem = prev_selem; | 693 | selem = prev_selem; |
| 693 | count = 0; | 694 | count = 0; |
| 694 | while (selem) { | 695 | while (selem) { |
| 695 | selem = hlist_entry_safe(selem->map_node.next, | 696 | selem = hlist_entry_safe(rcu_dereference(hlist_next_rcu(&selem->map_node)), |
| 696 | struct bpf_local_storage_elem, map_node); | 697 | struct bpf_local_storage_elem, map_node); |
| 697 | if (!selem) { | 698 | if (!selem) { |
| 698 | /* not found, unlock and go to the next bucket */ | 699 | /* not found, unlock and go to the next bucket */ |
| 699 | b = &smap->buckets[bucket_id++]; | 700 | b = &smap->buckets[bucket_id++]; |
| 700 | raw_spin_unlock_bh(&b->lock); | 701 | rcu_read_unlock(); |
| 701 | skip_elems = 0; | 702 | skip_elems = 0; |
| 702 | break; | 703 | break; |
| 703 | } | 704 | } |
| 704 | sk_storage = rcu_dereference_raw(selem->local_storage); | 705 | sk_storage = rcu_dereference(selem->local_storage); |
| 705 | if (sk_storage) { | 706 | if (sk_storage) { |
| 706 | info->skip_elems = skip_elems + count; | 707 | info->skip_elems = skip_elems + count; |
| 707 | return selem; | 708 | return selem; |
| @@ -711,10 +712,10 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info, · 3 unmodified lines | |||
| 711 | 712 | ||
| 712 | for (i = bucket_id; i < (1U << smap->bucket_log); i++) { | 713 | for (i = bucket_id; i < (1U << smap->bucket_log); i++) { |
| 713 | b = &smap->buckets[i]; | 714 | b = &smap->buckets[i]; |
| 714 | raw_spin_lock_bh(&b->lock); | 715 | rcu_read_lock(); |
| 715 | count = 0; | 716 | count = 0; |
| 716 | hlist_for_each_entry(selem, &b->list, map_node) { | 717 | hlist_for_each_entry_rcu(selem, &b->list, map_node) { |
| 717 | sk_storage = rcu_dereference_raw(selem->local_storage); | 718 | sk_storage = rcu_dereference(selem->local_storage); |
| 718 | if (sk_storage && count >= skip_elems) { | 719 | if (sk_storage && count >= skip_elems) { |
| 719 | info->bucket_id = i; | 720 | info->bucket_id = i; |
| 720 | info->skip_elems = count; | 721 | info->skip_elems = count; |
| @@ -722,7 +723,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info, · 1 unmodified line | |||
| 722 | } | 723 | } |
| 723 | count++; | 724 | count++; |
| 724 | } | 725 | } |
| 725 | raw_spin_unlock_bh(&b->lock); | 726 | rcu_read_unlock(); |
| 726 | skip_elems = 0; | 727 | skip_elems = 0; |
| 727 | } | 728 | } |
| 728 | 729 | ||
| @@ -781,7 +782,7 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq, · 52 unmodified lines | |||
| 781 | ctx.meta = &meta; | 782 | ctx.meta = &meta; |
| 782 | ctx.map = info->map; | 783 | ctx.map = info->map; |
| 783 | if (selem) { | 784 | if (selem) { |
| 784 | sk_storage = rcu_dereference_raw(selem->local_storage); | 785 | sk_storage = rcu_dereference(selem->local_storage); |
| 785 | ctx.sk = sk_storage->owner; | 786 | ctx.sk = sk_storage->owner; |
| 786 | ctx.value = SDATA(selem)->data; | 787 | ctx.value = SDATA(selem)->data; |
| 787 | } | 788 | } |
| @@ -797,18 +798,12 @@ static int bpf_sk_storage_map_seq_show(struct seq_file *seq, void *v) · 9 unmodified lines | |||
| 797 | } | 798 | } |
| 798 | 799 | ||
| 799 | static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v) | 800 | static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v) |
| 801 | __releases(RCU) | ||
| 800 | { | 802 | { |
| 801 | struct bpf_iter_seq_sk_storage_map_info *info = seq->private; | 803 | if (!v) |
| 802 | struct bpf_local_storage_map *smap; | ||
| 803 | struct bpf_local_storage_map_bucket *b; | ||
| 804 | |||
| 805 | if (!v) { | ||
| 806 | (void)__bpf_sk_storage_map_seq_show(seq, v); | 804 | (void)__bpf_sk_storage_map_seq_show(seq, v); |
| 807 | } else { | 805 | else |
| 808 | smap = (struct bpf_local_storage_map *)info->map; | 806 | rcu_read_unlock(); |
| 809 | b = &smap->buckets[info->bucket_id]; | ||
| 810 | raw_spin_unlock_bh(&b->lock); | ||
| 811 | } | ||
| 812 | } | 807 | } |
| 813 | 808 | ||
| 814 | static int bpf_iter_init_sk_storage_map(void *priv_data, | 809 | static int bpf_iter_init_sk_storage_map(void *priv_data, |