Re: [PATCH bpf-next v1 00/22] Resilient Queued Spin Lock

[Waiman Long <llong@redhat.com>]

On 1/9/25 4:13 PM, Kumar Kartikeya Dwivedi wrote:
> On Thu, 9 Jan 2025 at 19:29, Waiman Long <llong@redhat.com> wrote:
>> On 1/8/25 3:12 PM, Kumar Kartikeya Dwivedi wrote:
>>> On Wed, 8 Jan 2025 at 14:48, Peter Zijlstra <peterz@infradead.org> wrote:
>>>> On Tue, Jan 07, 2025 at 03:54:36PM -0800, Linus Torvalds wrote:
>>>>> On Tue, 7 Jan 2025 at 06:00, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>>>>>> This patch set introduces Resilient Queued Spin Lock (or rqspinlock with
>>>>>> res_spin_lock() and res_spin_unlock() APIs).
>>>>> So when I see people doing new locking mechanisms, I invariably go "Oh no!".
>>>>>
>>>>> But this series seems reasonable to me. I see that PeterZ had a couple
>>>>> of minor comments (well, the arm64 one is more fundamental), which
>>>>> hopefully means that it seems reasonable to him too. Peter?
>>>> I've not had time to fully read the whole thing yet, I only did a quick
>>>> once over. I'll try and get around to doing a proper reading eventually,
>>>> but I'm chasing a regression atm, and then I need to go review a ton of
>>>> code Andrew merged over the xmas/newyears holiday :/
>>>>
>>>> One potential issue is that qspinlock isn't suitable for all
>>>> architectures -- and I've yet to figure out widely BPF is planning on
>>>> using this.
>>> For architectures where qspinlock is not available, I think we can
>>> have a fallback to a test and set lock with timeout and deadlock
>>> checks, like patch 12.
>>> We plan on using this in BPF core and BPF maps, so the usage will be
>>> pervasive, and we have atleast one architecture in CI (s390) which
>>> doesn't have ARCH_USER_QUEUED_SPINLOCK selected, so we should have
>>> coverage for both cases. For now the fallback is missing, but I will
>>> add one in v2.
>> Event though ARCH_USE_QUEUED_SPINLOCK isn't set for s390, it is actually
>> using its own variant of qspinlock which encodes in the lock word
>> additional information needed by the architecture. Similary for PPC.
> Thanks, I see that now. It seems it is pretty similar to the paravirt
> scenario, where the algorithm would require changes to accommodate
> rqspinlock bits.
> For this series, I am planning to stick to a default TAS fallback, but
> we can tackle these cases together in a follow up.
> This series is already quite big and it would be better to focus on
> the base rqspinlock bits to keep things reviewable.
> Given we're only using this in BPF right now (in specific places where
> we're mindful we may fall back to TAS on some arches), we won't be
> regressing any other users.

I am not saying that you have to deal with that for the current patch 
series. However, it is something we need to tackle in the long run.

Cheers,
Longman