NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/57705: rump doesn't have a way to interrupt rump_sys_* by a signal
The following reply was made to PR kern/57705; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/57705: rump doesn't have a way to interrupt rump_sys_* by a signal
Date: Sun, 19 Nov 2023 04:02:37 +0000
This is a multi-part message in MIME format.
--=_UO/DuVWULAgwAF6z8mwyycn6j85jhood
The attached patch implements a rump signal model,
RUMP_SIGMODEL_RECORD_WAKEUP, for use with rump_boot_setsigmodel, that
makes rump signal delivery record the signal in the process and wake
up any cv_wait_sig or cv_timedwait_sig in progress. And it adds a
function rump_lwp_kill(l, signo) to simulate signal delivery.
Combined with RUMP_SIGMODEL_RECORD_WAKEUP, rump_lwp_kill will cause
pending cv_wait_sig or cv_timedwait_sig to wake, and cause subsequent
cv_wait_sig or cv_timedwait_sig to fail with EINTR without sleeping.
This is a new signal model so it won't break any existing code
(although in principle the extra logic in cv_wait_sig and
cv_timedwait_sig could break code because it's not conditional on
RUMP_SIGMODEL_RECORD_WAKEUP).
It's a little bodgy:
1. It takes proc_lock in any cv_wait_sig or cv_timedwait_sig call --
perhaps we can relax this to use a narrower-scoped lock, like
l->l_mutex (if that works), so it doesn't become a point of
contention in all rump code.
2. It delivers the signal to all lwps in a process, not just the
requested one -- should be a simple matter of programming to make
it work for just the requested one, but this serves my testing
needs for now.
3. Nothing clears the signal, so subsequent cv_wait_sig or
cv_timedwait_sig will continue to fail with EINTR without sleeping.
Either the caller of rump_sys_* will need to do something to clear
the pending signal (i.e., pretend to `handle' it), or we'll need to
invent some way for rump_sys_* itself to clear it.
But this serves my needs for the moment, which is to test the fix for
the PR kern/57703 eventfd panic without crashing my development
laptop.
--=_UO/DuVWULAgwAF6z8mwyycn6j85jhood
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57705-rumpsignalwakeup"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57705-rumpsignalwakeup.patch"
From ec313e6d40f530da7a2fd5a823c1c71e99e36fc0 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 19 Nov 2023 01:14:21 +0000
Subject: [PATCH] rump: New mechanism to interrupt rump_sys_* as if by a
signal.
PR kern/57705
---
sys/rump/include/rump/rump.h | 5 ++-
sys/rump/librump/rumpkern/locks.c | 52 ++++++++++++++++++++----
sys/rump/librump/rumpkern/signals.c | 28 +++++++++++++
tests/rump/rumpkern/t_signals.c | 62 +++++++++++++++++++++++++++++
4 files changed, 139 insertions(+), 8 deletions(-)
diff --git a/sys/rump/include/rump/rump.h b/sys/rump/include/rump/rump.h
index a4679d225fe7..48d1b794c95e 100644
--- a/sys/rump/include/rump/rump.h
+++ b/sys/rump/include/rump/rump.h
@@ -64,7 +64,8 @@ enum rump_sigmodel {
RUMP_SIGMODEL_IGNORE,
RUMP_SIGMODEL__HOST_NOTANYMORE,
RUMP_SIGMODEL_RAISE,
- RUMP_SIGMODEL_RECORD
+ RUMP_SIGMODEL_RECORD,
+ RUMP_SIGMODEL_RECORD_WAKEUP,
};
=20
/* flags to rump_lwproc_rfork */
@@ -124,6 +125,8 @@ int rump_init_server(const char *);
int rump_daemonize_done(int);
#define RUMP_DAEMONIZE_SUCCESS 0
=20
+void rump_lwp_kill(struct lwp *, int);
+
#ifndef _KERNEL
#include <rump/rumpkern_if_pub.h>
#include <rump/rumpvfs_if_pub.h>
diff --git a/sys/rump/librump/rumpkern/locks.c b/sys/rump/librump/rumpkern/=
locks.c
index 1b60b8e70706..bb3931708102 100644
--- a/sys/rump/librump/rumpkern/locks.c
+++ b/sys/rump/librump/rumpkern/locks.c
@@ -378,7 +378,7 @@ cv_destroy(kcondvar_t *cv)
}
=20
static int
-docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts)
+docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespec *ts, int catch)
{
struct lwp *l =3D curlwp;
int rv;
@@ -395,9 +395,21 @@ docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespe=
c *ts)
=20
UNLOCKED(mtx, false);
=20
- l->l_sched.info =3D cv;
rv =3D 0;
- if (ts) {
+
+ if (catch) {
+ if (mtx !=3D &proc_lock)
+ mutex_enter(&proc_lock);
+ }
+ l->l_sched.info =3D cv;
+ if (catch) {
+ rv =3D sigispending(curlwp, 0);
+ if (mtx !=3D &proc_lock)
+ mutex_exit(&proc_lock);
+ }
+ if (rv) {
+ /* nothing */
+ } else if (ts) {
if (rumpuser_cv_timedwait(RUMPCV(cv), RUMPMTX(mtx),
ts->tv_sec, ts->tv_nsec))
rv =3D EWOULDBLOCK;
@@ -430,7 +442,15 @@ docvwait(kcondvar_t *cv, kmutex_t *mtx, struct timespe=
c *ts)
mutex_enter(mtx);
rv =3D EINTR;
}
+ if (catch) {
+ if (mtx !=3D &proc_lock)
+ mutex_enter(&proc_lock);
+ }
l->l_sched.info =3D NULL;
+ if (catch) {
+ if (mtx !=3D &proc_lock)
+ mutex_exit(&proc_lock);
+ }
=20
return rv;
}
@@ -441,7 +461,7 @@ cv_wait(kcondvar_t *cv, kmutex_t *mtx)
=20
if (__predict_false(rump_threads =3D=3D 0))
panic("cv_wait without threads");
- (void) docvwait(cv, mtx, NULL);
+ (void) docvwait(cv, mtx, NULL, /*catch*/0);
}
=20
int
@@ -450,7 +470,7 @@ cv_wait_sig(kcondvar_t *cv, kmutex_t *mtx)
=20
if (__predict_false(rump_threads =3D=3D 0))
panic("cv_wait without threads");
- return docvwait(cv, mtx, NULL);
+ return docvwait(cv, mtx, NULL, /*catch*/1);
}
=20
int
@@ -460,17 +480,35 @@ cv_timedwait(kcondvar_t *cv, kmutex_t *mtx, int ticks)
extern int hz;
int rv;
=20
+ if (ticks =3D=3D 0) {
+ cv_wait(cv, mtx);
+ rv =3D 0;
+ } else {
+ ts.tv_sec =3D ticks / hz;
+ ts.tv_nsec =3D (ticks % hz) * (1000000000/hz);
+ rv =3D docvwait(cv, mtx, &ts, /*catch*/0);
+ }
+
+ return rv;
+}
+
+int
+cv_timedwait_sig(kcondvar_t *cv, kmutex_t *mtx, int ticks)
+{
+ struct timespec ts;
+ extern int hz;
+ int rv;
+
if (ticks =3D=3D 0) {
rv =3D cv_wait_sig(cv, mtx);
} else {
ts.tv_sec =3D ticks / hz;
ts.tv_nsec =3D (ticks % hz) * (1000000000/hz);
- rv =3D docvwait(cv, mtx, &ts);
+ rv =3D docvwait(cv, mtx, &ts, /*catch*/1);
}
=20
return rv;
}
-__strong_alias(cv_timedwait_sig,cv_timedwait);
=20
void
cv_signal(kcondvar_t *cv)
diff --git a/sys/rump/librump/rumpkern/signals.c b/sys/rump/librump/rumpker=
n/signals.c
index 2eb06f188bf5..556678e08b4d 100644
--- a/sys/rump/librump/rumpkern/signals.c
+++ b/sys/rump/librump/rumpkern/signals.c
@@ -105,6 +105,22 @@ rumpsig_record(struct proc *p, int signo)
}
}
=20
+static void
+rumpsig_record_wakeup(struct proc *p, int signo)
+{
+ struct lwp *l;
+
+ if (!sigismember(&p->p_sigctx.ps_sigignore, signo)) {
+ sigaddset(&p->p_sigpend.sp_set, signo);
+ mutex_enter(p->p_lock);
+ LIST_FOREACH(l, &p->p_lwps, l_sibling) {
+ if (l->l_sched.info)
+ cv_broadcast(l->l_sched.info);
+ }
+ mutex_exit(p->p_lock);
+ }
+}
+
typedef void (*rumpsig_fn)(struct proc *, int);
=20
static rumpsig_fn rumpsig =3D rumpsig_raise;
@@ -135,6 +151,9 @@ rump_boot_setsigmodel(enum rump_sigmodel model)
case RUMP_SIGMODEL_RECORD:
rumpsig =3D rumpsig_record;
break;
+ case RUMP_SIGMODEL_RECORD_WAKEUP:
+ rumpsig =3D rumpsig_record_wakeup;
+ break;
=20
/* for compat, though I doubt anyone is using it */
case RUMP_SIGMODEL__HOST_NOTANYMORE:
@@ -143,6 +162,15 @@ rump_boot_setsigmodel(enum rump_sigmodel model)
}
}
=20
+void
+rump_lwp_kill(struct lwp *l, int signo)
+{
+
+ mutex_enter(&proc_lock);
+ psignal(l->l_proc, signo);
+ mutex_exit(&proc_lock);
+}
+
void
psignal(struct proc *p, int sig)
{
diff --git a/tests/rump/rumpkern/t_signals.c b/tests/rump/rumpkern/t_signal=
s.c
index ba0c0ea36fb2..1f7712d29eee 100644
--- a/tests/rump/rumpkern/t_signals.c
+++ b/tests/rump/rumpkern/t_signals.c
@@ -32,11 +32,13 @@
=20
#include <atf-c.h>
#include <errno.h>
+#include <pthread.h>
#include <string.h>
#include <signal.h>
#include <unistd.h>
=20
#include <rump/rump.h>
+#include <rump/rump_syscalls.h>
=20
#include "../kernspace/kernspace.h"
#include "h_macros.h"
@@ -62,6 +64,13 @@ ATF_TC_HEAD(sigpanic, tc)
atf_tc_set_md_var(tc, "descr", "RUMP_SIGMODEL_PANIC");
}
=20
+ATF_TC(sigrecordwakeup);
+ATF_TC_HEAD(sigrecordwakeup, tc)
+{
+
+ atf_tc_set_md_var(tc, "descr", "RUMP_SIGMODEL_RECORD_WAKEUP");
+}
+
static volatile sig_atomic_t sigcnt;
static void
thehand(int sig)
@@ -116,12 +125,65 @@ ATF_TC_BODY(sigpanic, tc)
}
}
=20
+struct sigrecordwakeupctx {
+ pid_t pid;
+ struct lwp *lwp;
+};
+
+static void *
+signalme(void *cookie)
+{
+ struct sigrecordwakeupctx *C =3D cookie;
+
+ RZ(rump_pub_lwproc_newlwp(C->pid));
+
+ sleep(1);
+ rump_lwp_kill(C->lwp, SIGUSR1);
+
+ return NULL;
+}
+
+ATF_TC_BODY(sigrecordwakeup, tc)
+{
+ struct sigrecordwakeupctx ctx, *C =3D &ctx;
+ pthread_t t;
+ int fd[2];
+ char c;
+ ssize_t nwrit;
+ int error;
+
+ rump_boot_setsigmodel(RUMP_SIGMODEL_RECORD_WAKEUP);
+ rump_init();
+
+ memset(C, 0, sizeof(*C));
+
+ RZ(rump_pub_lwproc_rfork(RUMP_RFCFDG));
+ RL(C->pid =3D rump_sys_getpid());
+ RZ(rump_pub_lwproc_newlwp(C->pid));
+ C->lwp =3D rump_pub_lwproc_curlwp();
+
+ RL(rump_sys_pipe(fd));
+
+ RZ(pthread_create(&t, NULL, &signalme, C));
+
+ alarm(2);
+ nwrit =3D rump_sys_read(fd[0], &c, sizeof(c));
+ ATF_REQUIRE_EQ_MSG(nwrit, -1, "rump_sys_read: %zd", nwrit);
+ error =3D errno;
+ ATF_REQUIRE_EQ_MSG(error, EINTR, "errno=3D%d (%s)",
+ error, strerror(error));
+
+ RZ(pthread_join(t, NULL));
+ alarm(0);
+}
+
ATF_TP_ADD_TCS(tp)
{
=20
ATF_TP_ADD_TC(tp, sigraise);
ATF_TP_ADD_TC(tp, sigignore);
ATF_TP_ADD_TC(tp, sigpanic);
+ ATF_TP_ADD_TC(tp, sigrecordwakeup);
=20
return atf_no_error();
}
--=_UO/DuVWULAgwAF6z8mwyycn6j85jhood--
Home |
Main Index |
Thread Index |
Old Index