NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: port-sparc/56788 Rump kernel panic: kernel diagnostic assertion "old == LOCK_LOCKED" failed
The following reply was made to PR port-sparc/56788; it has been noted by GNATS.
From: Tom Lane <tgl%sss.pgh.pa.us@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: port-hppa-maintainer%netbsd.org@localhost
Subject: Re: port-sparc/56788 Rump kernel panic: kernel diagnostic assertion "old == LOCK_LOCKED" failed
Date: Sun, 19 Jun 2022 22:36:31 -0400
------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2324668.1655692526.1%sss.pgh.pa.us@localhost>
I wrote:
> I'm wondering about arranging for atomic_cas_generic.c's spinlock
> array to be kept in shared memory somehow.
Attached is a very dirty proof-of-concept patch that does the above.
The good news is that it seems to completely resolve the t_ping test
failures for me. The bad news is that (1) there's a lot of work
left to make this committable, and (2) for me, it doesn't seem to
fix any other tests besides t_ping. (2) suggests that the path of
least resistance is to just disable that test on affected arches.
OTOH, it seems likely that more tests will hit this in future,
so maybe it's worth doing the requisite amount of work for.
I don't have the ability to take this any further, but if someone
else wants to, what remains to be done includes:
* It's not clear to me how to handle the fact that
make_atomic_cas_globally_atomic() is only needed on some arches.
I'm sure there's a convention for that, I just don't know it.
* This probably violates a lot of other NetBSD style as well.
* I've not done anything about the SPARC situation. Probably
atomic_cas.S could be adapted to use a similar mechanism, but
maybe it'd be better to start using atomic_cas_generic.c
in the SPARC rump kernel?
regards, tom lane
------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="global-atomic-cas.patch";
charset="us-ascii"
Content-ID: <2324668.1655692526.2%sss.pgh.pa.us@localhost>
Content-Description: global-atomic-cas.patch
Content-Transfer-Encoding: quoted-printable
Index: lib/librumpuser/rumpuser_mem.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvsroot/src/lib/librumpuser/rumpuser_mem.c,v
retrieving revision 1.2
diff -u -r1.2 rumpuser_mem.c
--- lib/librumpuser/rumpuser_mem.c 24 Aug 2014 14:37:31 -0000 1.2
+++ lib/librumpuser/rumpuser_mem.c 20 Jun 2022 02:16:10 -0000
@@ -102,6 +102,44 @@
ET(rv);
}
=
+int
+rumpuser_filemmap(void *prefaddr, int fd, size_t size, int alignbit,
+ int exec, void **memp)
+{
+ void *mem;
+ int prot, rv;
+
+ if (ftruncate(fd, size) =3D=3D -1) {
+ rv =3D errno;
+ ET(rv);
+ }
+
+#ifndef MAP_ALIGNED
+#define MAP_ALIGNED(a) 0
+ if (alignbit)
+ fprintf(stderr, "rumpuser_filemmap: warning, requested "
+ "alignment not supported by hypervisor\n");
+#endif
+
+#if defined(__sun__) && !defined(MAP_FILE)
+#define MAP_FILE 0
+#endif
+
+ prot =3D PROT_READ|PROT_WRITE;
+ if (exec)
+ prot |=3D PROT_EXEC;
+ mem =3D mmap(prefaddr, size, prot,
+ MAP_FILE | MAP_SHARED | MAP_ALIGNED(alignbit), fd, 0);
+ if (mem =3D=3D MAP_FAILED) {
+ rv =3D errno;
+ } else {
+ *memp =3D mem;
+ rv =3D 0;
+ }
+
+ ET(rv);
+}
+
void
rumpuser_unmap(void *addr, size_t len)
{
Index: sys/rump/include/rump/rumpuser.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvsroot/src/sys/rump/include/rump/rumpuser.h,v
retrieving revision 1.116
diff -u -r1.116 rumpuser.h
--- sys/rump/include/rump/rumpuser.h 22 Mar 2020 13:30:10 -0000 1.116
+++ sys/rump/include/rump/rumpuser.h 20 Jun 2022 02:16:12 -0000
@@ -74,6 +74,7 @@
int rumpuser_malloc(size_t, int, void **);
void rumpuser_free(void *, size_t);
int rumpuser_anonmmap(void *, size_t, int, int, void **);
+int rumpuser_filemmap(void *, int, size_t, int, int, void **);
void rumpuser_unmap(void *, size_t);
=
/*
Index: sys/rump/librump/rumpkern/atomic_cas_generic.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvsroot/src/sys/rump/librump/rumpkern/atomic_cas_generic.c,v
retrieving revision 1.3
diff -u -r1.3 atomic_cas_generic.c
--- sys/rump/librump/rumpkern/atomic_cas_generic.c 14 Mar 2021 22:56:39 -0=
000 1.3
+++ sys/rump/librump/rumpkern/atomic_cas_generic.c 20 Jun 2022 02:16:12 -0=
000
@@ -33,6 +33,13 @@
* This is basically common/lib/libc/atomic/atomic_init_testset.c
* which always assumes MP and never uses RAS. This is used only on
* some archs. See Makefile.rumpkern for more information.
+ *
+ * We provide atomicity by using spinlocks; to avoid unnecessary contenti=
on,
+ * one of several spinlocks is chosen based on the address of the target =
word.
+ * Ordinarily, we only need to interlock between threads in a single proc=
ess,
+ * so that a plain static array of spinlocks is fine. But some tests req=
uire
+ * interlocking between threads in different processes. To support that,
+ * we can move the spinlock array into shared memory.
*/
=
#include "atomic_op_namespace.h"
@@ -40,12 +47,26 @@
#include <sys/types.h>
#include <sys/atomic.h>
#include <sys/lock.h>
+#include <sys/errno.h>
+
+#include <rump/rump.h>
+#include <rump/rumpuser.h>
+
+struct atomic_locks_shmem {
+ __cpu_simple_lock_t shared_atomic_locks[128];
+ int atomic_locks_magic;
+#define ATOMIC_LOCKS_MAGIC 0x58274727
+};
+
+static int make_one_spinlock_file(const char *path, =
+ __cpu_simple_lock_t **atomic_locks_ptr);
=
#define I2 __SIMPLELOCK_UNLOCKED, __SIMPLELOCK_UNLOCKED,
#define I16 I2 I2 I2 I2 I2 I2 I2 I2
#define I128 I16 I16 I16 I16 I16 I16 I16 I16
=
-static __cpu_simple_lock_t atomic_locks32[128] =3D { I128 };
+static __cpu_simple_lock_t static_atomic_locks32[128] =3D { I128 };
+static __cpu_simple_lock_t *atomic_locks32 =3D static_atomic_locks32;
=
uint32_t
_atomic_cas_32(volatile uint32_t *ptr, uint32_t old, uint32_t new)
@@ -53,6 +74,14 @@
__cpu_simple_lock_t *lock;
uint32_t ret;
=
+ /*
+ * Note that we use only the low-order 10 bits of the ptr to choose a
+ * spinlock. If the target word is in shared memory, it's possible that
+ * different processes would have it mapped at different addresses. But
+ * so long as all processes map the shared memory on a VM page boundary,
+ * and the page size is at least 1KB, we'll choose the same spinlock in
+ * all processes.
+ */
lock =3D &atomic_locks32[((uintptr_t)ptr >> 3) & 127];
__cpu_simple_lock(lock);
ret =3D *ptr;
@@ -70,7 +99,8 @@
__strong_alias(_atomic_cas_32_ni,_atomic_cas_32)
=
#ifdef _LP64
-static __cpu_simple_lock_t atomic_locks64[128] =3D { I128 };
+static __cpu_simple_lock_t static_atomic_locks64[128] =3D { I128 };
+static __cpu_simple_lock_t *atomic_locks64 =3D static_atomic_locks64;
=
uint64_t
_atomic_cas_64(volatile uint64_t *ptr, uint64_t old, uint64_t new)
@@ -78,7 +108,8 @@
__cpu_simple_lock_t *lock;
uint64_t ret;
=
- lock =3D &atomic_locks64[((uintptr_t)ptr >> 4) & 127];
+ /* See comment above about using only 10 bits of the ptr */
+ lock =3D &atomic_locks64[((uintptr_t)ptr >> 3) & 127];
__cpu_simple_lock(lock);
ret =3D *ptr;
if (__predict_true(ret =3D=3D old)) {
@@ -124,3 +155,106 @@
atomic_op_alias(atomic_cas_ptr_ni,_atomic_cas_32)
__strong_alias(_atomic_cas_ptr_ni,_atomic_cas_32)
#endif
+
+/*
+ * Move the atomic_cas_generic spinlocks into shared memory,
+ * so that atomicity is guaranteed across multiple processes.
+ * This is a no-op if it was already done in this process.
+ * Returns 0 if okay, else an errno code.
+ */
+extern int make_atomic_cas_globally_atomic(void); /* XXX where to put thi=
s? */
+
+int
+make_atomic_cas_globally_atomic(void)
+{
+ int error;
+
+ if (atomic_locks32 =3D=3D static_atomic_locks32) {
+ error =3D make_one_spinlock_file("netbsd-atomic-cas-32-spinlocks",
+ &atomic_locks32);
+ if (error)
+ return error;
+ }
+#ifdef _LP64
+ if (atomic_locks64 =3D=3D static_atomic_locks64) {
+ error =3D make_one_spinlock_file("netbsd-atomic-cas-64-spinlocks",
+ &atomic_locks64);
+ if (error)
+ return error;
+ }
+#endif
+ return 0;
+}
+
+static int
+make_one_spinlock_file(const char *path, =
+ __cpu_simple_lock_t **atomic_locks_ptr)
+{
+ bool created_it =3D false;
+ int memfd =3D -1; /* XXXgcc */
+ void *mem;
+ struct atomic_locks_shmem *almem;
+ int error;
+
+ /*
+ * Create or open the backing file, detecting if we created it.
+ *
+ * XXX there's no provision to remove it at shutdown. This doesn't
+ * matter in typical use under atf-run, and even when not doing that,
+ * it's not terribly harmful if the file stays around. One might
+ * need to manually remove it if a rump kernel crashes while holding
+ * one of the spinlocks, but that should be mighty rare.
+ */
+ while ((error =3D rumpuser_open(path,
+ RUMPUSER_OPEN_RDWR, &memfd)) !=3D 0)
+ {
+ if (error !=3D ENOENT)
+ return error;
+ error =3D rumpuser_open(path,
+ RUMPUSER_OPEN_RDWR | RUMPUSER_OPEN_CREATE | RUMPUSER_OPEN_EXCL,
+ &memfd);
+ if (error) {
+ if (error !=3D EEXIST)
+ return error;
+ continue; /* try again to open without creating */
+ }
+ created_it =3D true;
+ break;
+ }
+
+ /* Map it */
+ error =3D rumpuser_filemmap(NULL, memfd, sizeof(*almem), 0, 0, &mem);
+ if (error) {
+ rumpuser_close(memfd);
+ /* XXX unlink if created_it? */
+ return error;
+ }
+ almem =3D mem;
+
+ /*
+ * If we created it, initialize the shared spinlocks. Otherwise,
+ * wait if necessary for the creator to do so.
+ */
+ if (created_it) {
+ KASSERT(almem->atomic_locks_magic =3D=3D 0);
+ for (int i =3D 0; i < 128; i++)
+ __cpu_simple_lock_init(&almem->shared_atomic_locks[i]);
+ almem->atomic_locks_magic =3D ATOMIC_LOCKS_MAGIC;
+ } else {
+ while (almem->atomic_locks_magic !=3D ATOMIC_LOCKS_MAGIC)
+ (void) rumpuser_clock_sleep(RUMPUSER_CLOCK_RELWALL,
+ 0, 10000000);
+ }
+
+ /*
+ * Now replace the pointer to the static spinlock array with the
+ * address of the shared spinlocks array. This is a bit shaky
+ * because in principle some other threads in our own process could
+ * be contending for a lock, and this could transiently let them both
+ * think they got it. In practice this function should only be
+ * called during fairly early initialization, so the risk seems low.
+ */
+ *atomic_locks_ptr =3D almem->shared_atomic_locks;
+
+ return 0;
+}
Index: sys/rump/net/lib/libshmif/if_shmem.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvsroot/src/sys/rump/net/lib/libshmif/if_shmem.c,v
retrieving revision 1.84
diff -u -r1.84 if_shmem.c
--- sys/rump/net/lib/libshmif/if_shmem.c 9 Apr 2022 23:45:02 -0000 1.84
+++ sys/rump/net/lib/libshmif/if_shmem.c 20 Jun 2022 02:16:12 -0000
@@ -56,6 +56,8 @@
=
#include "shmif_user.h"
=
+extern int make_atomic_cas_globally_atomic(void); /* XXX where to put thi=
s? */
+
static int shmif_clone(struct if_clone *, int);
static int shmif_unclone(struct ifnet *);
=
@@ -240,6 +242,10 @@
void *mem;
int error;
=
+ error =3D make_atomic_cas_globally_atomic();
+ if (error)
+ return error;
+
error =3D rumpcomp_shmif_mmap(memfd, BUSMEM_SIZE, &mem);
if (error)
return error;
------- =_aaaaaaaaaa0--
Home |
Main Index |
Thread Index |
Old Index