Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement sigtimedwait for use by suckless init #1320

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
a219223
Add /sbin/init based on suckless sinit.
cahirwpz Jul 26, 2022
7286e68
First check tests to run then run init.
cahirwpz Jul 26, 2022
e07f7c1
Merge branch 'master' into suckless-sinit
cahirwpz Oct 14, 2022
6f5ad93
wip
korbiniak Nov 9, 2022
3d15ce3
[WIP] do_sigtimedwait
korbiniak Nov 10, 2022
1e3744d
Merge branch 'master' into suckless-sinit
cahirwpz Nov 10, 2022
1ce829f
[WIP] Sigtimedwait finished, first test
korbiniak Dec 18, 2022
000d730
Add /sbin/init based on suckless sinit.
cahirwpz Jul 26, 2022
2d7ca28
First check tests to run then run init.
cahirwpz Jul 26, 2022
b386909
wip
korbiniak Nov 9, 2022
fb3eaa3
[WIP] do_sigtimedwait
korbiniak Nov 10, 2022
e41929e
[WIP] Sigtimedwait finished, first test
korbiniak Dec 18, 2022
7abe9f9
Merge branch 'master' into suckless-sinit
cahirwpz Dec 18, 2022
35a1ecc
Fixes after merge.
cahirwpz Dec 18, 2022
c486015
Revert changes to the build & test infrastructure.
cahirwpz Dec 18, 2022
bc5288f
Merge branch 'suckless-sinit' of github.com:cahirwpz/mimiker into suc…
korbiniak Dec 30, 2022
7b0199c
Sigtimedwait finished
korbiniak Dec 30, 2022
239ca0e
format
korbiniak Dec 30, 2022
da05dd4
Fixed llvm version
korbiniak Dec 30, 2022
54b38ef
format
korbiniak Dec 30, 2022
1994eb2
Fixed launch changes
korbiniak Dec 30, 2022
b301a1c
Fixed race in timeout test
korbiniak Jan 1, 2023
ac3908b
Format.
korbiniak Jan 1, 2023
51bbe9f
Lowered the timeout in signal_sigtimedwait_timeout test.
korbiniak Jan 1, 2023
cbf8c25
Lower the timeout even more.
korbiniak Jan 1, 2023
29c302b
Merge branch 'master' into suckless-sinit
cahirwpz Jan 3, 2023
e597ef4
Remove sbin/init as it's a part of contrib directory
cahirwpz Jan 3, 2023
eff8b00
Merge branch 'master' into suckless-sinit
korbiniak Jan 12, 2023
76ca8f1
Use new utest interface for sigtimedwait tests.
korbiniak Jan 12, 2023
c576c73
Use new utest util macros.
korbiniak Jan 12, 2023
db00050
Check only for waited signals in the pending mask.
korbiniak Jan 12, 2023
4088dd6
Handle bad timeout format.
korbiniak Jan 13, 2023
cdfc493
Remove printf's from test, assert on all syscalls.
korbiniak Jan 13, 2023
f7977d5
Use utest_spawn and utest_child_exited
korbiniak Jan 14, 2023
97d0591
Clean up do_sigtimedwait
korbiniak Jan 19, 2023
f9014c9
Refactor tests
korbiniak Jan 19, 2023
a5fb873
Tidy up
korbiniak Jan 20, 2023
0c30ce6
Merge branch 'master' into suckless-sinit
cahirwpz Jun 8, 2023
86c4f14
Merge branch 'master' into suckless-sinit
cahirwpz Jun 14, 2023
3c7670d
Address some long standing remarks (pt. 1)
cahirwpz Jun 14, 2023
4b8edb8
Address some long lasting remarks (pt. 2)
cahirwpz Jun 14, 2023
65da74e
Address some long lasting remarks (pt. 3)
cahirwpz Jun 15, 2023
b4caf4b
Merge branch 'master' into suckless-sinit
cahirwpz Jul 26, 2023
d1df936
Fix tests.
cahirwpz Jul 28, 2023
59d3c58
Merge branch 'master' into suckless-sinit
cahirwpz Jul 28, 2023
c3c4abd
Merge branch 'master' into suckless-sinit
cahirwpz Jul 28, 2023
cfab58a
Merge branch 'master' into suckless-sinit
cahirwpz Aug 1, 2023
78e955b
Merge branch 'master' into suckless-sinit
cahirwpz Aug 1, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions bin/utest/signal.c
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,114 @@ TEST_ADD(signal_sigsuspend, 0) {
return 0;
}

/* ======= signal_sigtimedwait ======= */
static int sigtimedwait_child(__unused void *arg) {
pid_t ppid = getppid();
xkill(ppid, SIGUSR1);
return 0;
}

TEST_ADD(signal_sigtimedwait, 0) {
xsignal(SIGCONT, sigcont_handler);
sigset_t set, current, waitset;
sigemptyset(&set);
sigaddset(&set, SIGUSR1);
sigaddset(&set, SIGCONT);
xsigprocmask(SIG_SETMASK, &set, NULL);

spawn(sigtimedwait_child, NULL);

siginfo_t info;
sigemptyset(&waitset);
sigaddset(&waitset, SIGUSR1);
assert(sigtimedwait(&waitset, &info, NULL) == SIGUSR1);
cahirwpz marked this conversation as resolved.
Show resolved Hide resolved
assert(info.si_signo == SIGUSR1);

xsigprocmask(SIG_BLOCK, NULL, &current);
assert(sigsetequal(&set, &current));

wait_child_finished(0);
return 0;
}

/* ======= signal_sigtimedwait_timeout ======= */
static int sigtimedwait_timeout_child(__unused void *arg) {
ppid = getppid();
xkill(ppid, SIGUSR1);
xkill(ppid, SIGCONT);
return 0;
}

TEST_ADD(signal_sigtimedwait_timeout, 0) {
xsignal(SIGCONT, sigcont_handler);
sigset_t set, waitset;
sigemptyset(&set);
sigaddset(&set, SIGUSR1);
xsigprocmask(SIG_SETMASK, &set, NULL);

siginfo_t info;
sigemptyset(&waitset);
sigaddset(&waitset, SIGUSR1);
timespec_t timeout = {
.tv_nsec = -1,
.tv_sec = -1,
};

/* tv_nsec is invalid. */
syscall_fail(sigtimedwait(&waitset, &info, &timeout), EINVAL);

/* tv_nsec is valid, but tv_sec < 0. */
timeout.tv_nsec = 10000000;
syscall_fail(sigtimedwait(&waitset, &info, &timeout), EAGAIN);

timeout.tv_sec = 0;
/* Should timeout. */
syscall_fail(sigtimedwait(&waitset, &info, &timeout), EAGAIN);

spawn(sigtimedwait_timeout_child, NULL);

/* If we handled sigcont, then SIGUSR1 must be pending. */
while (!sigcont_handled)
sched_yield();

/* Should not block, but receive the signal as it is in the pending mask. */
timeout.tv_nsec = 0;
assert(sigtimedwait(&waitset, &info, &timeout) == SIGUSR1);
assert(info.si_signo == SIGUSR1);

wait_child_finished(0);
return 0;
}

/* ======= signal_sigtimedwait_intr ====== */
int sigtimedwait_intr_child(void *arg) {
pid_t ppid = getppid();
while (!sigcont_handled) {
xkill(ppid, SIGCONT);
}
return 0;
}

TEST_ADD(signal_sigtimedwait_intr, 0) {
xsignal(SIGCONT, sigcont_handler);
sigset_t set, waitset;
sigemptyset(&set);
sigaddset(&set, SIGUSR1);
xsigprocmask(SIG_SETMASK, &set, NULL);

siginfo_t info;
sigemptyset(&waitset);
sigaddset(&waitset, SIGUSR1);

pid_t cpid = spawn(sigtimedwait_intr_child, NULL);

syscall_fail(sigtimedwait(&waitset, &info, NULL), EINTR);

xkill(cpid, SIGCONT);
wait_child_finished(0);
return 0;
}

/* ======= signal_sigsuspend_stop ======= */
TEST_ADD(signal_sigsuspend_stop, 0) {
pid_t ppid = getpid();
Expand Down
Empty file modified etc/rc.init
100644 → 100755
Empty file.
Empty file modified etc/rc.shutdown
100644 → 100755
Empty file.
84 changes: 83 additions & 1 deletion sys/kern/signal.c
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,89 @@ int do_sigsuspend(proc_t *p, const sigset_t *mask) {

int do_sigtimedwait(proc_t *p, sigset_t waitset, ksiginfo_t *ksi,
timespec_t *tsp) {
return ENOTSUP;
int error = 0, timeout = 0;
sigset_t saved_mask;
signo_t sig;
thread_t *td = p->p_thread;

/*
* Calculate timeout based on tsp.
*
* NULL pointer means an infinite timeout.
* {.tv_sec = 0, .tv_nsec = 0} means to not block.
* Timeout is valid only if 0 <= tv_nsec <= 1e9. If this is not true,
* immediately return EINVAL. If tv_sec < 0 immediately return EAGAIN.
*
* We set the timeout value such that:
* - timeout == -1: do not block
* - timeout == 0: wait indefinitely
pj1031999 marked this conversation as resolved.
Show resolved Hide resolved
* - timeout > 0: wait for timeout.
*/
if (tsp != NULL) {
if (tsp->tv_nsec < 0 || tsp->tv_nsec >= 1000000000) {
return EINVAL;
}
if (tsp->tv_sec < 0) {
return EAGAIN;
}

timeout = ts2hz(tsp);
if (timeout == 0) {
if (tsp->tv_nsec == 0 && tsp->tv_sec == 0) {
timeout = -1;
korbiniak marked this conversation as resolved.
Show resolved Hide resolved
} else {
/* Shortest possible timeout */
timeout = 1;
}
}
}

bzero(ksi, sizeof(*ksi));

/* Silently ignore SIGKILL and SIGSTOP. */
__sigminusset(&cantmask, &waitset);

WITH_PROC_LOCK(p) {
/* Unblocking temporarly waited signals so we're woken up upon receiving
* such signal. */
saved_mask = td->td_sigmask;
__sigminusset(&waitset, &td->td_sigmask);

/* We need to find pending signal that we also wait for manually, as
* sig_pending may return a pending signal not from waitset. */
sigset_t pending = td->td_sigpend.sp_set;
__sigandset(&waitset, &pending);
if ((sig = __sigfindset(&pending))) {
sigpend_get(&td->td_sigpend, sig, ksi);
error = 0;
goto out;
}
if (timeout == -1) {
error = EAGAIN;
goto out;
}

error =
sleepq_wait_timed(&td->td_sigmask, "sigtimedwait()", &p->p_lock, timeout);

if (error == ETIMEDOUT) {
error = EAGAIN;
goto out;
}

pending = td->td_sigpend.sp_set;
__sigandset(&waitset, &pending);
if ((sig = __sigfindset(&pending))) {
sigpend_get(&td->td_sigpend, sig, ksi);
error = 0;
cahirwpz marked this conversation as resolved.
Show resolved Hide resolved
} else {
error = EINTR;
}
}

out:
td->td_sigmask = saved_mask;
return error;
}

int do_sigpending(proc_t *p, sigset_t *set) {
Expand Down
5 changes: 4 additions & 1 deletion sys/kern/sleepq.c
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,10 @@ int sleepq_wait_timed(void *wchan, const void *waitpt, mtx_t *mtx,
callout_schedule(&td->td_slpcallout, timeout);
}

td->td_flags |= (timeout > 0) ? TDF_SLPTIMED : TDF_SLPINTR;
td->td_flags |= TDF_SLPINTR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with sleepq, but was there a reason why we chose TDF_SLPINTR only when we hadn't got a timeout? You have changed the semantics here and I cant tell if it is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did change the semantics as I think it was a bug that TDF_SLPINTR was not set. See discussion here: https://discord.com/channels/951841212267130950/1054133625731424337

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I agree with that, but I also think that @cahirwpz should take a look at that.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to do with possible deficiencies in sleepq API. ATM we have two wait functions - with and without timeout. IMO there should be wait and wait_intr with optional timeout. That should let us express all use cases for putting a thread to sleep.

Please note that non-interruptible wait should probably be only used by the code that does not interface user-space directly - i.e. the code that does not understand the concept of a signal or need not be burdened with complexity of extra error handling (ETIMEDOUT). Examples would be waking up a thread from interrupt or "fast" device drivers code. On contrary the interruptible wait should be used in "slow" device drivers code (where read or write may be interrupted) and all syscalls that can tolerate being interrupted.

Perhaps what I wrote above should be redacted and put somewhere into sleepq.h.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer the question - the change is ok, since function is documented as follows:

/*! \brief Performs interruptible sleep with timeout.
 *
 * Timed sleep is also interruptible. [...]
 */

if (timeout > 0)
td->td_flags |= TDF_SLPTIMED;

sq_enter(td, sc, wchan, waitpt);

/* After wakeup, only one of the following flags may be set:
Expand Down