[PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Ilya Leoshkevich]

Hi,

I tried running the arena_spin_lock test on s390x and ran into the
following issues:

* Changing the header file does not lead to rebuilding the test.
* The checked for number of CPUs and the actually required number of
  CPUs are different.
* Endianness issue in spinlock definition.

This series fixes all three.

Best regards,
Ilya

Ilya Leoshkevich (3):
  selftests/bpf: Fix arena_spin_lock.c build dependency
  selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
  selftests/bpf: Fix endianness issue in __qspinlock declaration

 .../selftests/bpf/prog_tests/arena_spin_lock.c     | 14 ++++++++------
 .../bpf/{ => progs}/bpf_arena_spin_lock.h          | 12 ++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)
 rename tools/testing/selftests/bpf/{ => progs}/bpf_arena_spin_lock.h (98%)

-- 
2.49.0

[PATCH 1/3] selftests/bpf: Fix arena_spin_lock.c build dependency

[Ilya Leoshkevich]

Changing bpf_arena_spin_lock.h does not lead to recompiling
arena_spin_lock.c. By convention, all BPF progs depend on all
header files in progs/, so move this header file there. There
are no other users besides arena_spin_lock.c.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/{ => progs}/bpf_arena_spin_lock.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename tools/testing/selftests/bpf/{ => progs}/bpf_arena_spin_lock.h (100%)

diff --git a/tools/testing/selftests/bpf/bpf_arena_spin_lock.h b/tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h
similarity index 100%
rename from tools/testing/selftests/bpf/bpf_arena_spin_lock.h
rename to tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h
-- 
2.49.0

[PATCH 2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs

[Ilya Leoshkevich]

test_arena_spin_lock_size() explicitly requires having at least 2 CPUs,
but if the machine has less than 16, then pthread_setaffinity_np() call
in spin_lock_thread() fails.

Cap threads to the number of CPUs.

Alternative solutions are raising the number of required CPUs to 16, or
pinning multiple threads to the same CPU, but they are not that useful.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/prog_tests/arena_spin_lock.c     | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/arena_spin_lock.c b/tools/testing/selftests/bpf/prog_tests/arena_spin_lock.c
index 7565fc7690c2..0223fce4db2b 100644
--- a/tools/testing/selftests/bpf/prog_tests/arena_spin_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/arena_spin_lock.c
@@ -51,9 +51,11 @@ static void test_arena_spin_lock_size(int size)
 	struct arena_spin_lock *skel;
 	pthread_t thread_id[16];
 	int prog_fd, i, err;
+	int nthreads;
 	void *ret;
 
-	if (get_nprocs() < 2) {
+	nthreads = MIN(get_nprocs(), ARRAY_SIZE(thread_id));
+	if (nthreads < 2) {
 		test__skip();
 		return;
 	}
@@ -66,25 +68,25 @@ static void test_arena_spin_lock_size(int size)
 		goto end;
 	}
 	skel->bss->cs_count = size;
-	skel->bss->limit = repeat * 16;
+	skel->bss->limit = repeat * nthreads;
 
-	ASSERT_OK(pthread_barrier_init(&barrier, NULL, 16), "barrier init");
+	ASSERT_OK(pthread_barrier_init(&barrier, NULL, nthreads), "barrier init");
 
 	prog_fd = bpf_program__fd(skel->progs.prog);
-	for (i = 0; i < 16; i++) {
+	for (i = 0; i < nthreads; i++) {
 		err = pthread_create(&thread_id[i], NULL, &spin_lock_thread, &prog_fd);
 		if (!ASSERT_OK(err, "pthread_create"))
 			goto end_barrier;
 	}
 
-	for (i = 0; i < 16; i++) {
+	for (i = 0; i < nthreads; i++) {
 		if (!ASSERT_OK(pthread_join(thread_id[i], &ret), "pthread_join"))
 			goto end_barrier;
 		if (!ASSERT_EQ(ret, &prog_fd, "ret == prog_fd"))
 			goto end_barrier;
 	}
 
-	ASSERT_EQ(skel->bss->counter, repeat * 16, "check counter value");
+	ASSERT_EQ(skel->bss->counter, repeat * nthreads, "check counter value");
 
 end_barrier:
 	pthread_barrier_destroy(&barrier);
-- 
2.49.0

[PATCH 3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration

[Ilya Leoshkevich]

Copy the big-endian field declarations from qspinlock_types.h,
otherwise some properties won't hold on big-endian systems. For
example, assigning lock->val = 1 should result in lock->locked == 1,
which is not the case there.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/progs/bpf_arena_spin_lock.h        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h b/tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h
index 4e29c31c4ef8..d67466c1ff77 100644
--- a/tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h
+++ b/tools/testing/selftests/bpf/progs/bpf_arena_spin_lock.h
@@ -32,6 +32,7 @@ extern unsigned long CONFIG_NR_CPUS __kconfig;
 struct __qspinlock {
 	union {
 		atomic_t val;
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 		struct {
 			u8 locked;
 			u8 pending;
@@ -40,6 +41,17 @@ struct __qspinlock {
 			u16 locked_pending;
 			u16 tail;
 		};
+#else
+		struct {
+			u16 tail;
+			u16 locked_pending;
+		};
+		struct {
+			u8 reserved[2];
+			u8 pending;
+			u8 locked;
+		};
+#endif
 	};
 };
 
-- 
2.49.0

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> Hi,
> 
> I tried running the arena_spin_lock test on s390x and ran into the
> following issues:
> 
> * Changing the header file does not lead to rebuilding the test.
> * The checked for number of CPUs and the actually required number of
>   CPUs are different.
> * Endianness issue in spinlock definition.
> 
> [...]

Here is the summary with links:
  - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
    https://git.kernel.org/netdev/net-next/c/4fe09ff1a54a
  - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
    (no matching commit)
  - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Alexei Starovoitov]

On Thu, Apr 24, 2025 at 11:32 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This series was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > Hi,
> >
> > I tried running the arena_spin_lock test on s390x and ran into the
> > following issues:
> >
> > * Changing the header file does not lead to rebuilding the test.
> > * The checked for number of CPUs and the actually required number of
> >   CPUs are different.
> > * Endianness issue in spinlock definition.
> >
> > [...]
>
> Here is the summary with links:
>   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
>     https://git.kernel.org/netdev/net-next/c/4fe09ff1a54a
>   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
>     (no matching commit)
>   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
>     (no matching commit)

Hmm. Looks like pw-bot had too much influence from AI bots
and started hallucinating itself :)


Ilya,

no worries. Your patches are still under review.
I unmarked it in patchwork.

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Konstantin Ryabitsev]

On Thu, Apr 24, 2025 at 11:41:16AM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 24, 2025 at 11:32 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> >
> > Hello:
> >
> > This series was applied to netdev/net-next.git (main)
> > by Jakub Kicinski <kuba@kernel.org>:
> >
> > On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > > Hi,
> > >
> > > I tried running the arena_spin_lock test on s390x and ran into the
> > > following issues:
> > >
> > > * Changing the header file does not lead to rebuilding the test.
> > > * The checked for number of CPUs and the actually required number of
> > >   CPUs are different.
> > > * Endianness issue in spinlock definition.
> > >
> > > [...]
> >
> > Here is the summary with links:
> >   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
> >     https://git.kernel.org/netdev/net-next/c/4fe09ff1a54a
> >   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
> >     (no matching commit)
> >   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
> >     (no matching commit)
> 
> Hmm. Looks like pw-bot had too much influence from AI bots
> and started hallucinating itself :)

I'll look into what happened here.

-K

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> Hi,
> 
> I tried running the arena_spin_lock test on s390x and ran into the
> following issues:
> 
> * Changing the header file does not lead to rebuilding the test.
> * The checked for number of CPUs and the actually required number of
>   CPUs are different.
> * Endianness issue in spinlock definition.
> 
> [...]

Here is the summary with links:
  - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
    https://git.kernel.org/netdev/net-next/c/6fdc754b922b
  - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
    (no matching commit)
  - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Alexei Starovoitov]

On Thu, Apr 24, 2025 at 5:09 PM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This series was applied to netdev/net-next.git (main)
> by Jakub Kicinski <kuba@kernel.org>:
>
> On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > Hi,
> >
> > I tried running the arena_spin_lock test on s390x and ran into the
> > following issues:
> >
> > * Changing the header file does not lead to rebuilding the test.
> > * The checked for number of CPUs and the actually required number of
> >   CPUs are different.
> > * Endianness issue in spinlock definition.
> >
> > [...]
>
> Here is the summary with links:
>   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
>     https://git.kernel.org/netdev/net-next/c/6fdc754b922b
>   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
>     (no matching commit)
>   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
>     (no matching commit)

Konstantin,

pw-bot is going berserk.

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> Hi,
> 
> I tried running the arena_spin_lock test on s390x and ran into the
> following issues:
> 
> * Changing the header file does not lead to rebuilding the test.
> * The checked for number of CPUs and the actually required number of
>   CPUs are different.
> * Endianness issue in spinlock definition.
> 
> [...]

Here is the summary with links:
  - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
    https://git.kernel.org/bpf/bpf-next/c/ddfd1f30b5ba
  - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
    https://git.kernel.org/bpf/bpf-next/c/0240e5a9431c
  - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
    https://git.kernel.org/bpf/bpf-next/c/be5521991506

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Alexei Starovoitov]

On Thu, Apr 24, 2025 at 5:39 PM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This series was applied to bpf/bpf-next.git (master)
> by Alexei Starovoitov <ast@kernel.org>:
>
> On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > Hi,
> >
> > I tried running the arena_spin_lock test on s390x and ran into the
> > following issues:
> >
> > * Changing the header file does not lead to rebuilding the test.
> > * The checked for number of CPUs and the actually required number of
> >   CPUs are different.
> > * Endianness issue in spinlock definition.
> >
> > [...]
>
> Here is the summary with links:
>   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
>     https://git.kernel.org/bpf/bpf-next/c/ddfd1f30b5ba
>   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
>     https://git.kernel.org/bpf/bpf-next/c/0240e5a9431c
>   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
>     https://git.kernel.org/bpf/bpf-next/c/be5521991506
>
> You are awesome, thank you!

It was applied to bpf-next for real.

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Jakub Kicinski]

On Thu, 24 Apr 2025 14:51:51 -0400 Konstantin Ryabitsev wrote:
> > Hmm. Looks like pw-bot had too much influence from AI bots
> > and started hallucinating itself :)  
> 
> I'll look into what happened here.

Alexei mentioned that the bot was stopped, I presume to avoid further
mistakes. I'm 100% sure I've seen the bot be confused by merge commits
before. It happens occasionally, IMHO there is no need to take the bot
offline. Is there an ETA on it coming back?

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Konstantin Ryabitsev]

On Fri, Apr 25, 2025 at 09:25:51AM -0700, Jakub Kicinski wrote:
> On Thu, 24 Apr 2025 14:51:51 -0400 Konstantin Ryabitsev wrote:
> > > Hmm. Looks like pw-bot had too much influence from AI bots
> > > and started hallucinating itself :)  
> > 
> > I'll look into what happened here.
> 
> Alexei mentioned that the bot was stopped, I presume to avoid further
> mistakes. I'm 100% sure I've seen the bot be confused by merge commits
> before. It happens occasionally, IMHO there is no need to take the bot
> offline. Is there an ETA on it coming back?

Yes, I'm poking at it right now and I'm hoping to bring it back up soon.

-K

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Konstantin Ryabitsev]

On Thu, Apr 24, 2025 at 11:41:16AM -0700, Alexei Starovoitov wrote:
> > On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > > Hi,
> > >
> > > I tried running the arena_spin_lock test on s390x and ran into the
> > > following issues:
> > >
> > > * Changing the header file does not lead to rebuilding the test.
> > > * The checked for number of CPUs and the actually required number of
> > >   CPUs are different.
> > > * Endianness issue in spinlock definition.
> > >
> > > [...]
> >
> > Here is the summary with links:
> >   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
> >     https://git.kernel.org/netdev/net-next/c/4fe09ff1a54a
> >   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
> >     (no matching commit)
> >   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
> >     (no matching commit)
> 
> Hmm. Looks like pw-bot had too much influence from AI bots
> and started hallucinating itself :)

Looks like it's a mix of bad assumptions and the usual difficulty of
recognizing fast-forward merges that came in through a different tree.

If you look at the commit mentioned above, it has:

| Note that the first patch in this series is a leftover from an
| earlier patchset that was abandoned:
| Link: https://lore.kernel.org/netdev/20250129004337.36898-2-shannon.nelson@amd.com/

This confuses the bot into thinking that the linked message is the source of
the patch (which is why we started using patch.msgid.link to disambiguate
links aimed at cross-referencing and links aimed at indicating commit
provenance -- but we aren't relying on this disambiguation in the bot itself
yet).

The other replies are the usual mess when fast-forward tree updates confuse
things. It's a long-standing hard bug to fix.

I am going to re-enable the bot for now -- in general it's not any more wrong
than usual. I'm scheduling some time next week to try to tackle the
fast-forwards problem.

-K

Re: [PATCH 0/3] selftests/bpf: Fix a few issues in arena_spin_lock

[Alexei Starovoitov]

On Fri, Apr 25, 2025 at 11:58 AM Konstantin Ryabitsev
<konstantin@linuxfoundation.org> wrote:
>
> On Thu, Apr 24, 2025 at 11:41:16AM -0700, Alexei Starovoitov wrote:
> > > On Thu, 24 Apr 2025 18:41:24 +0200 you wrote:
> > > > Hi,
> > > >
> > > > I tried running the arena_spin_lock test on s390x and ran into the
> > > > following issues:
> > > >
> > > > * Changing the header file does not lead to rebuilding the test.
> > > > * The checked for number of CPUs and the actually required number of
> > > >   CPUs are different.
> > > > * Endianness issue in spinlock definition.
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > >   - [1/3] selftests/bpf: Fix arena_spin_lock.c build dependency
> > >     https://git.kernel.org/netdev/net-next/c/4fe09ff1a54a
> > >   - [2/3] selftests/bpf: Fix arena_spin_lock on systems with less than 16 CPUs
> > >     (no matching commit)
> > >   - [3/3] selftests/bpf: Fix endianness issue in __qspinlock declaration
> > >     (no matching commit)
> >
> > Hmm. Looks like pw-bot had too much influence from AI bots
> > and started hallucinating itself :)
>
> Looks like it's a mix of bad assumptions and the usual difficulty of
> recognizing fast-forward merges that came in through a different tree.
>
> If you look at the commit mentioned above, it has:
>
> | Note that the first patch in this series is a leftover from an
> | earlier patchset that was abandoned:
> | Link: https://lore.kernel.org/netdev/20250129004337.36898-2-shannon.nelson@amd.com/
>
> This confuses the bot into thinking that the linked message is the source of
> the patch (which is why we started using patch.msgid.link to disambiguate
> links aimed at cross-referencing and links aimed at indicating commit
> provenance -- but we aren't relying on this disambiguation in the bot itself
> yet).

Thanks for investigating. The above part is clear,
but I still don't understand what was so special about Ilya's
patch that only his first patch in the series became a victim.
msgid-s are completely different.

> The other replies are the usual mess when fast-forward tree updates confuse
> things. It's a long-standing hard bug to fix.
>
> I am going to re-enable the bot for now -- in general it's not any more wrong
> than usual.

Makes sense. Better to have it flaky than none at all.

> I'm scheduling some time next week to try to tackle the
> fast-forwards problem.

Thanks. That would be great.