From: Usama Arif <usama.arif@linux.dev>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>,
ast@kernel.org, andrii@kernel.org, bpf@vger.kernel.org,
daniel@iogearbox.net, eddyz87@gmail.com, emil@etsalapatis.com,
ihor.solodrai@linux.dev, jolsa@kernel.org,
justinstitt@google.com, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev, martin.lau@linux.dev, morbo@google.com,
nathan@kernel.org, ndesaulniers@google.com, song@kernel.org,
yonghong.song@linux.dev, yatsenko@meta.com, kernel-team@meta.com,
Amery Hung <ameryhung@gmail.com>
Subject: Re: [PATCH] bpf: local_storage: avoid redundant IRQ save on bucket locks
Date: Wed, 16 Sep 2026 20:22:01 +0100 [thread overview]
Message-ID: <874fc50f-4764-433f-8a87-595bc70c4559@linux.dev> (raw)
In-Reply-To: <DLGY7IFP8MOG.YFISSWRDGKR3@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.
>
>> [...]
next prev parent reply other threads:[~2026-09-16 19:22 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-16 18:10 [PATCH] bpf: local_storage: avoid redundant IRQ save on bucket locks Usama Arif
2026-09-16 18:22 ` Kumar Kartikeya Dwivedi
2026-09-16 18:30 ` Amery Hung
2026-09-16 19:20 ` Usama Arif
2026-09-16 19:22 ` Usama Arif [this message]
2026-09-16 19:29 ` Amery Hung
2026-09-16 20:00 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874fc50f-4764-433f-8a87-595bc70c4559@linux.dev \
--to=usama.arif@linux.dev \
--cc=ameryhung@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=emil@etsalapatis.com \
--cc=ihor.solodrai@linux.dev \
--cc=jolsa@kernel.org \
--cc=justinstitt@google.com \
--cc=kernel-team@meta.com \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=morbo@google.com \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=song@kernel.org \
--cc=yatsenko@meta.com \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.