Re: [PATCH bpf 1/2] bpf: Don't allocate and free prog pack memory with pack_mutex held

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

On Tue, Nov 03, 2026 at 06:28 AM Puranjay Mohan <puranjay@kernel.org> wrote:
> Progs are freed from system_percpu_wq too, and bpf_prog_free_deferred()
> sleeps on pack_mutex. When enough progs are freed at once, these work
> items take all active slots of the pool that purge_work was queued to.
> purge_work never runs and everything waits for pack_mutex:

Thanks for taking over.

Turned out Emil is fixing the same issue I was seeing:
https://lore.kernel.org/bpf/20261102073439.6822-1-emil@etsalapatis.com/

Thousands of work items that sleep don't belong in system_percpu_wq.
commit 117932eea99b fixed it for cgroup/bpf.

On Emil's patch I proposed to move frees into their own wq
with max_active = 1.

                                         this patch    own wq
  -------------------------------------  ------------  ----------
  work items per cpu that sleep on       up to 1024    1
  pack_mutex
  same deadlock on vmap_purge_lock       still there   gone
  N tasks find no room in the packs      N new packs   1 new pack
  pack_mutex held over execmem calls     no            yes

Row 1 also bounds the time that a prog load waits for pack_mutex.

Row 2: the s390 JIT and the interpreter call vfree() of
a VM_FLUSH_RESET_PERMS area for every prog.
vm_reset_perms() calls _vm_unmap_aliases(), so these work items sleep
on vmap_purge_lock. Its owner waits for purge_work in the same wq.

Row 3: see below.

Row 4 is fine. purge_work gets its slot, so the owner of pack_mutex
makes progress.

Then no need for the retry in bpf_prog_pack_alloc().

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fdbf638..6ce2e31 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -70,6 +70,7 @@ extern spinlock_t btf_idr_lock;
 extern struct kobject *btf_kobj;
 extern struct bpf_mem_alloc bpf_global_ma, bpf_global_percpu_ma;
 extern bool bpf_global_ma_set;
+extern struct workqueue_struct *bpf_free_wq;

 typedef u64 (*bpf_callback_t)(u64, u64, u64, u64, u64);
 typedef int (*bpf_iter_init_seq_priv_t)(void *private_data,
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 831f73f..e83edea 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3198,7 +3198,7 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 	/* The symbols are looked up under rcu_read_lock() */
 	INIT_RCU_WORK(&aux->rwork, bpf_prog_free_images);
 	if (ksyms)
-		queue_rcu_work(system_percpu_wq, &aux->rwork);
+		queue_rcu_work(bpf_free_wq, &aux->rwork);
 	else
 		bpf_prog_free_images(&aux->rwork.work);
 }
@@ -3211,9 +3211,26 @@ void bpf_prog_free(struct bpf_prog *fp)
 		bpf_prog_put(aux->dst_prog);
 	bpf_token_put(aux->token);
 	INIT_WORK(&aux->work, bpf_prog_free_deferred);
-	schedule_work(&aux->work);
+	queue_work(bpf_free_wq, &aux->work);
 }

+/*
+ * Many progs and trampolines can be freed at once and their work items sleep
+ * on pack_mutex. Its owner can wait for a work item of system_percpu_wq.
+ * Use a separate workqueue, so that they don't fill up max_active of
+ * system_percpu_wq.
+ */
+struct workqueue_struct *bpf_free_wq;
+
+static int __init bpf_free_wq_init(void)
+{
+	bpf_free_wq = alloc_workqueue("bpf_free", WQ_PERCPU, 1);
+	if (!bpf_free_wq)
+		panic("Failed to alloc workqueue for bpf free.\n");
+	return 0;
+}
+pure_initcall(bpf_free_wq_init);
+
 /* RNG for unprivileged user space with separated state from prandom_u32(). */
 static DEFINE_PER_CPU(struct rnd_state, bpf_user_rnd_state);

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 21d4608..a34e8a2 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -584,7 +584,7 @@ static void __bpf_tramp_image_put_rcu(struct rcu_head *rcu)

 	im = container_of(rcu, struct bpf_tramp_image, rcu);
 	INIT_WORK(&im->work, __bpf_tramp_image_put_deferred);
-	schedule_work(&im->work);
+	queue_work(bpf_free_wq, &im->work);
 }

 /* callback, fexit step 2. Called after percpu_ref_kill confirms. */

> +	mutex_unlock(&pack_mutex);
> +	new_pack = alloc_new_pack(bpf_fill_ill_insns);
> +	if (!new_pack)
> +		return NULL;
> +
> +	mutex_lock(&pack_mutex);
> +	goto again;

Every task that finds no room before task 1 adds its pack allocates
one more:

  task 1                     task 2 ... task N
  no room, unlock
  alloc_new_pack()           no room, unlock
                             alloc_new_pack()
  lock, list_add_tail()
                             lock, room in the pack of task 1
                             unlock, free the new pack

A pack is 2 Mbytes * num_possible_nodes(). Nobody is charged for it.
N is the number of tasks, not cpus, since alloc_new_pack() sleeps.
If we go with your approach the memory needs to be bound somehow.

pw-bot: cr