bpf_local_storage_update() takes the map bucket lock while holding
local_storage->lock. bpf_selem_unlink_map() does the same; its only
caller holds local_storage->lock. The outer lock is acquired with
raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
sites.

Using raw_res_spin_lock_irqsave() for the nested lock saves the already
disabled IRQ state and issues another IRQ disable. The matching unlock
tests that saved state before leaving interrupts disabled. On x86-64,
this adds a pushfq/popq/cli sequence and a test/branch around an
unreachable sti to each acquisition.

Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
preemption nesting, memory ordering and resilient-lock bookkeeping. The
outer unlock remains responsible for restoring the caller's IRQ state.

In the tested clang x86-64 build, this removes five executed instructions
from each uncontended nested acquisition. It also shrinks
bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
from 1030 to 992 bytes. The affected paths are updates that add or replace
an element in existing owner storage and successful unlinks.

Document the owner-lock requirement of bpf_selem_unlink_map() and assert
that interrupts are disabled.

Signed-off-by: Usama Arif <usama.arif@linux.dev>
---
 kernel/bpf/bpf_local_storage.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 6fc6a4b672b55..4642da062f0f8 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -240,24 +240,26 @@ void bpf_selem_link_storage_nolock(struct bpf_local_storage *local_storage,
 	hlist_add_head_rcu(&selem->snode, &local_storage->list);
 }
 
+/* Must be called with the owning local_storage->lock held. */
 static int bpf_selem_unlink_map(struct bpf_local_storage_elem *selem)
 {
 	struct bpf_local_storage *local_storage;
 	struct bpf_local_storage_map *smap;
 	struct bpf_local_storage_map_bucket *b;
-	unsigned long flags;
 	int err;
 
+	lockdep_assert_irqs_disabled();
+
 	local_storage = rcu_dereference_check(selem->local_storage,
 					      bpf_rcu_lock_held());
 	smap = rcu_dereference_check(SDATA(selem)->smap, bpf_rcu_lock_held());
 	b = select_bucket(smap, local_storage);
-	err = raw_res_spin_lock_irqsave(&b->lock, flags);
+	err = raw_res_spin_lock(&b->lock);
 	if (err)
 		return err;
 
 	hlist_del_init_rcu(&selem->map_node);
-	raw_res_spin_unlock_irqrestore(&b->lock, flags);
+	raw_res_spin_unlock(&b->lock);
 
 	return 0;
 }
@@ -552,7 +554,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
 	struct bpf_local_storage *local_storage;
 	struct bpf_local_storage_map_bucket *b;
 	HLIST_HEAD(old_selem_free_list);
-	unsigned long flags, b_flags;
+	unsigned long flags;
 	int err;
 
 	/* BPF_EXIST and BPF_NOEXIST cannot be both set */
@@ -637,7 +639,8 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
 
 	b = select_bucket(smap, local_storage);
 
-	err = raw_res_spin_lock_irqsave(&b->lock, b_flags);
+	/* local_storage->lock is held, so IRQs are already disabled. */
+	err = raw_res_spin_lock(&b->lock);
 	if (err)
 		goto unlock;
 
@@ -655,7 +658,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
 						&old_selem_free_list);
 	}
 
-	raw_res_spin_unlock_irqrestore(&b->lock, b_flags);
+	raw_res_spin_unlock(&b->lock);
 unlock:
 	raw_res_spin_unlock_irqrestore(&local_storage->lock, flags);
 free_selem:
-- 
2.53.0-Meta

+Cc Amery

On Wed Sep 16, 2026 at 8:10 PM CEST, Usama Arif wrote:
> bpf_local_storage_update() takes the map bucket lock while holding
> local_storage->lock. bpf_selem_unlink_map() does the same; its only
> caller holds local_storage->lock. The outer lock is acquired with
> raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
> sites.
>
> Using raw_res_spin_lock_irqsave() for the nested lock saves the already
> disabled IRQ state and issues another IRQ disable. The matching unlock
> tests that saved state before leaving interrupts disabled. On x86-64,
> this adds a pushfq/popq/cli sequence and a test/branch around an
> unreachable sti to each acquisition.
>
> Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
> preemption nesting, memory ordering and resilient-lock bookkeeping. The
> outer unlock remains responsible for restoring the caller's IRQ state.
>
> In the tested clang x86-64 build, this removes five executed instructions
> from each uncontended nested acquisition. It also shrinks
> bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
> from 1030 to 992 bytes. The affected paths are updates that add or replace
> an element in existing owner storage and successful unlinks.
>
> Document the owner-lock requirement of bpf_selem_unlink_map() and assert
> that interrupts are disabled.
>
> Signed-off-by: Usama Arif <usama.arif@linux.dev>
> ---

Makes sense. But did you observe any measurable improvement with this change?

> [...]

On 16/09/2026 19:22, Kumar Kartikeya Dwivedi wrote:
> +Cc Amery
> 
> On Wed Sep 16, 2026 at 8:10 PM CEST, Usama Arif wrote:
>> bpf_local_storage_update() takes the map bucket lock while holding
>> local_storage->lock. bpf_selem_unlink_map() does the same; its only
>> caller holds local_storage->lock. The outer lock is acquired with
>> raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
>> sites.
>>
>> Using raw_res_spin_lock_irqsave() for the nested lock saves the already
>> disabled IRQ state and issues another IRQ disable. The matching unlock
>> tests that saved state before leaving interrupts disabled. On x86-64,
>> this adds a pushfq/popq/cli sequence and a test/branch around an
>> unreachable sti to each acquisition.
>>
>> Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
>> preemption nesting, memory ordering and resilient-lock bookkeeping. The
>> outer unlock remains responsible for restoring the caller's IRQ state.
>>
>> In the tested clang x86-64 build, this removes five executed instructions
>> from each uncontended nested acquisition. It also shrinks
>> bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
>> from 1030 to 992 bytes. The affected paths are updates that add or replace
>> an element in existing owner storage and successful unlinks.
>>
>> Document the owner-lock requirement of bpf_selem_unlink_map() and assert
>> that interrupts are disabled.
>>
>> Signed-off-by: Usama Arif <usama.arif@linux.dev>
>> ---
> 
> Makes sense. But did you observe any measurable improvement with this change?

Meta fleet wide profile shows bpf_local_storage_update() as one of the more expensive
bpf functions in the fleet. I saw it in production when profiling a hhvm workload as
well.

The main argument for the patch was reduced number of instructions executed in
this expensive path that I see in disassembly (which is mentioned in the 2nd paragraph)
and better code hygiene as it doesnt make sense to to save irq again. I would imagine
this patch alone wont move the needle in application metrics, but would make this function
cheaper (hopefully :)) fleetwide.


> 
>> [...]

On Wed, Sep 16, 2026 at 12:22 PM Usama Arif <usama.arif@linux.dev> wrote:
>
>
>
> On 16/09/2026 19:22, Kumar Kartikeya Dwivedi wrote:
> > +Cc Amery
> >
> > On Wed Sep 16, 2026 at 8:10 PM CEST, Usama Arif wrote:
> >> bpf_local_storage_update() takes the map bucket lock while holding
> >> local_storage->lock. bpf_selem_unlink_map() does the same; its only
> >> caller holds local_storage->lock. The outer lock is acquired with
> >> raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
> >> sites.
> >>
> >> Using raw_res_spin_lock_irqsave() for the nested lock saves the already
> >> disabled IRQ state and issues another IRQ disable. The matching unlock
> >> tests that saved state before leaving interrupts disabled. On x86-64,
> >> this adds a pushfq/popq/cli sequence and a test/branch around an
> >> unreachable sti to each acquisition.
> >>
> >> Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
> >> preemption nesting, memory ordering and resilient-lock bookkeeping. The
> >> outer unlock remains responsible for restoring the caller's IRQ state.
> >>
> >> In the tested clang x86-64 build, this removes five executed instructions
> >> from each uncontended nested acquisition. It also shrinks
> >> bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
> >> from 1030 to 992 bytes. The affected paths are updates that add or replace
> >> an element in existing owner storage and successful unlinks.
> >>
> >> Document the owner-lock requirement of bpf_selem_unlink_map() and assert
> >> that interrupts are disabled.
> >>
> >> Signed-off-by: Usama Arif <usama.arif@linux.dev>
> >> ---
> >
> > Makes sense. But did you observe any measurable improvement with this change?
>
> Meta fleet wide profile shows bpf_local_storage_update() as one of the more expensive
> bpf functions in the fleet. I saw it in production when profiling a hhvm workload as
> well.
>
> The main argument for the patch was reduced number of instructions executed in
> this expensive path that I see in disassembly (which is mentioned in the 2nd paragraph)
> and better code hygiene as it doesnt make sense to to save irq again. I would imagine
> this patch alone wont move the needle in application metrics, but would make this function
> cheaper (hopefully :)) fleetwide.
>

Reviewed-by: Amery Hung <ameryhung@gmail.com>

>
> >
> >> [...]
>

On 16/09/2026 19:22, Kumar Kartikeya Dwivedi wrote:
> +Cc Amery
> 
> On Wed Sep 16, 2026 at 8:10 PM CEST, Usama Arif wrote:
>> bpf_local_storage_update() takes the map bucket lock while holding
>> local_storage->lock. bpf_selem_unlink_map() does the same; its only
>> caller holds local_storage->lock. The outer lock is acquired with
>> raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
>> sites.
>>
>> Using raw_res_spin_lock_irqsave() for the nested lock saves the already
>> disabled IRQ state and issues another IRQ disable. The matching unlock
>> tests that saved state before leaving interrupts disabled. On x86-64,
>> this adds a pushfq/popq/cli sequence and a test/branch around an
>> unreachable sti to each acquisition.
>>
>> Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
>> preemption nesting, memory ordering and resilient-lock bookkeeping. The
>> outer unlock remains responsible for restoring the caller's IRQ state.
>>
>> In the tested clang x86-64 build, this removes five executed instructions
>> from each uncontended nested acquisition. It also shrinks
>> bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
>> from 1030 to 992 bytes. The affected paths are updates that add or replace
>> an element in existing owner storage and successful unlinks.
>>
>> Document the owner-lock requirement of bpf_selem_unlink_map() and assert
>> that interrupts are disabled.
>>
>> Signed-off-by: Usama Arif <usama.arif@linux.dev>
>> ---
> 
> Makes sense. But did you observe any measurable improvement with this change?

Meta fleet wide profile shows bpf_local_storage_update() as one of the more expensive
bpf functions in the fleet. I saw it in production when profiling a hhvm workload as
well.

The main argument for the patch was reduced number of instructions executed in
this expensive path that I see in disassembly (which is mentioned in the 2nd paragraph)
and better code hygiene as it doesnt make sense to to save irq again. I would imagine
this patch alone wont move the needle in application metrics, but would make this function
cheaper (hopefully :)) fleetwide.


> 
>> [...]

On Wed, Sep 16, 2026 at 11:22 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> +Cc Amery
>
> On Wed Sep 16, 2026 at 8:10 PM CEST, Usama Arif wrote:
> > bpf_local_storage_update() takes the map bucket lock while holding
> > local_storage->lock. bpf_selem_unlink_map() does the same; its only
> > caller holds local_storage->lock. The outer lock is acquired with
> > raw_res_spin_lock_irqsave(), so interrupts are already disabled at both
> > sites.
> >
> > Using raw_res_spin_lock_irqsave() for the nested lock saves the already
> > disabled IRQ state and issues another IRQ disable. The matching unlock
> > tests that saved state before leaving interrupts disabled. On x86-64,
> > this adds a pushfq/popq/cli sequence and a test/branch around an
> > unreachable sti to each acquisition.
> >
> > Use raw_res_spin_lock() and raw_res_spin_unlock() instead. They retain
> > preemption nesting, memory ordering and resilient-lock bookkeeping. The
> > outer unlock remains responsible for restoring the caller's IRQ state.
> >
> > In the tested clang x86-64 build, this removes five executed instructions
> > from each uncontended nested acquisition. It also shrinks
> > bpf_local_storage_update() from 1732 to 1702 bytes and bpf_selem_unlink()
> > from 1030 to 992 bytes. The affected paths are updates that add or replace
> > an element in existing owner storage and successful unlinks.
> >
> > Document the owner-lock requirement of bpf_selem_unlink_map() and assert
> > that interrupts are disabled.
> >
> > Signed-off-by: Usama Arif <usama.arif@linux.dev>
> > ---
>
> Makes sense. But did you observe any measurable improvement with this change?
>

Same question, but the change looks right to me.

> > [...]