Re: [PATCH] bpf: local_storage: avoid redundant IRQ save on bucket locks

[Usama Arif]

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@xxxxxxxxx>
>> ---
> 
> 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.


> 
>> [...]