Hi, On 10/21/2022 1:52 AM, Stanislav Fomichev wrote: > On Wed, Oct 19, 2022 at 6:18 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: SNIP >>>> 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. > But this is still mostly relevant only for the preemt_rt/has_interrupt > case, right? > For non-preempt, irq should've finished long before we got to drain_mem_cache. Yes. The concurrent access on free_llist is only possible for preempt_rt/does_not_has_interrupt cases.
