On Wed, Oct 19, 2022 at 6:18 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > 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. 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.
