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

[Waiman Long]

On 1/9/25 4:13 PM, Kumar Kartikeya Dwivedi wrote:
> On Thu, 9 Jan 2025 at 19:29, Waiman Long <llong@xxxxxxxxxx> wrote:
> > On 1/8/25 3:12 PM, Kumar Kartikeya Dwivedi wrote:
> > > On Wed, 8 Jan 2025 at 14:48, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxx> 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