Messages in this thread | | | Subject | Re: [PATCH bpf] bpf: Do not grab the bucket spinlock by default on htab batch ops | From | Yonghong Song <> | Date | Tue, 18 Feb 2020 08:34:36 -0800 |
| |
On 2/18/20 7:56 AM, Daniel Borkmann wrote: > On 2/18/20 4:43 PM, Yonghong Song wrote: >> On 2/14/20 2:43 PM, Brian Vazquez wrote: >>> Grabbing the spinlock for every bucket even if it's empty, was causing >>> significant perfomance cost when traversing htab maps that have only a >>> few entries. This patch addresses the issue by checking first the >>> bucket_cnt, if the bucket has some entries then we go and grab the >>> spinlock and proceed with the batching. >>> >>> Tested with a htab of size 50K and different value of populated entries. >>> >>> Before: >>> Benchmark Time(ns) CPU(ns) >>> --------------------------------------------- >>> BM_DumpHashMap/1 2759655 2752033 >>> BM_DumpHashMap/10 2933722 2930825 >>> BM_DumpHashMap/200 3171680 3170265 >>> BM_DumpHashMap/500 3639607 3635511 >>> BM_DumpHashMap/1000 4369008 4364981 >>> BM_DumpHashMap/5k 11171919 11134028 >>> BM_DumpHashMap/20k 69150080 69033496 >>> BM_DumpHashMap/39k 190501036 190226162 >>> >>> After: >>> Benchmark Time(ns) CPU(ns) >>> --------------------------------------------- >>> BM_DumpHashMap/1 202707 200109 >>> BM_DumpHashMap/10 213441 210569 >>> BM_DumpHashMap/200 478641 472350 >>> BM_DumpHashMap/500 980061 967102 >>> BM_DumpHashMap/1000 1863835 1839575 >>> BM_DumpHashMap/5k 8961836 8902540 >>> BM_DumpHashMap/20k 69761497 69322756 >>> BM_DumpHashMap/39k 187437830 186551111 >>> >>> Fixes: 057996380a42 ("bpf: Add batch ops to all htab bpf map") >>> Cc: Yonghong Song <yhs@fb.com> >>> Signed-off-by: Brian Vazquez <brianvv@google.com> >> >> Acked-by: Yonghong Song <yhs@fb.com> > > I must probably be missing something, but how is this safe? Presume we > traverse in the walk with bucket_cnt = 0. Meanwhile a different CPU added > entries to this bucket since not locked. Same reader on the other CPU with > bucket_cnt = 0 then starts to traverse the second > hlist_nulls_for_each_entry_safe() unlocked e.g. deleting entries?
Thanks for pointing this out. Yes, you are correct. If bucket_cnt is 0 and buck->lock is not held, we should skip the hlist_nulls_for_each_entry_safe(l, n, head, hash_node) { ... } as another cpu may traverse the bucket in parallel by adding/deleting the elements.
| |