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

[Amery Hung]

On Wed, Sep 16, 2026 at 12:22 PM Usama Arif <usama.arif@xxxxxxxxx> 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@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.
>

Reviewed-by: Amery Hung <ameryhung@xxxxxxxxx>

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