Hi, On 10/20/2022 3:00 AM, sdf@xxxxxxxxxx wrote: > On 10/19, Hou Tao wrote: >> From: Hou Tao <houtao1@xxxxxxxxxx> > >> Except for waiting_for_gp list, there are no concurrent operations on >> free_by_rcu, free_llist and free_llist_extra lists, so use >> __llist_del_all() instead of llist_del_all(). waiting_for_gp list can be >> deleted by RCU callback concurrently, so still use llist_del_all(). > >> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx> >> --- >> kernel/bpf/memalloc.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> diff --git a/kernel/bpf/memalloc.c b/kernel/bpf/memalloc.c >> index 48e606aaacf0..7f45744a09f7 100644 >> --- a/kernel/bpf/memalloc.c >> +++ b/kernel/bpf/memalloc.c >> @@ -422,14 +422,17 @@ static void drain_mem_cache(struct bpf_mem_cache *c) >> /* No progs are using this bpf_mem_cache, but htab_map_free() called >> * bpf_mem_cache_free() for all remaining elements and they can be in >> * free_by_rcu or in waiting_for_gp lists, so drain those lists now. >> + * >> + * Except for waiting_for_gp list, there are no concurrent operations >> + * on these lists, so it is safe to use __llist_del_all(). >> */ >> llist_for_each_safe(llnode, t, __llist_del_all(&c->free_by_rcu)) >> free_one(c, llnode); >> llist_for_each_safe(llnode, t, llist_del_all(&c->waiting_for_gp)) >> free_one(c, llnode); >> - llist_for_each_safe(llnode, t, llist_del_all(&c->free_llist)) >> + llist_for_each_safe(llnode, t, __llist_del_all(&c->free_llist)) >> free_one(c, llnode); >> - llist_for_each_safe(llnode, t, llist_del_all(&c->free_llist_extra)) >> + llist_for_each_safe(llnode, t, __llist_del_all(&c->free_llist_extra)) >> free_one(c, llnode); > > Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx> Thanks for the Acked-by. > > Seems safe even without the previous patch? OTOH, do we really care > about __lllist vs llist in the cleanup path? Might be safer to always > do llist_del_all everywhere? No. free_llist is manipulated by both irq work and memory draining concurrently before patch #1. Using llist_del_all(&c->free_llist) also doesn't help because irq work uses __llist_add/__llist_del helpers. Basically there is no difference between __llist and list helper for cleanup patch, but I think it is better to clarity the possible concurrent accesses and codify these assumption. > >> } > >> -- >> 2.29.2 > > .
