Re: tsleep(9): sleep on private channel if ident is NULL
> Date: Fri, 18 Dec 2020 13:49:32 -0600
> From: Scott Cheloha
>
> Hi,
>
> This patch adds support for passing NULL as the ident when calling
> tsleep(9) etc. When this happens, sleep_setup() will use the address
> of the sleep_state struct as the value for p_wchan. This address is
> basically always a private value so the thread should never receive a
> wakeup(9) broadcast.
>
> Why do we want this? Sometimes there is no logical ident to sleep on.
> Sometimes there is legitimately no reason to receive a wakeup(9).
>
> In the past, people have handrolled private channels to work around
> this situation. The code often looks like this:
>
> void
> foo(void)
> {
> int chan;
>
> tsleep(&chan, ...);
> }
>
> Permitting the use of NULL and letting the implementation choose a
> private channel is better than handrolling a private channel for two
> reasons:
>
> 1. We save a bit of stack space. tsleep(9) etc. already have a
>sleep_state struct on the stack and it's at a private address
>so there is no space cost to use it.
>
> 2. The NULL clearly communicates the author's intent to the reader.
>It indicates the author had no wakeup channel in mind when they
>wrote the code. The reader then doesn't need to reason about
>whether or not the ident value is superfluous. Poring over
>a file (or several) to determine whether any thread ever calls
>wakeup(9) on a given ident sucks.
>
> FreeBSD/NetBSD have a dedicated interface for this "sleep without a
> wakeup channel" operation. They call it "pause". I proposed adding
> it but I got mixed feedback on the patch. Then mpi@ proposed this
> idea. I think this is simpler and better.
>
> The actual implementation requires just a few small changes to
> sleep_setup().
>
> I've added an additional KASSERT to each of tsleep(9), msleep(9), and
> rwsleep(9). You now need at least one of (a) an ident or (b) PCATCH
> or (c) a timeout, otherwise there is no way to get the thread started
> again. This would indicate a programmer error and we should panic if
> it ever happens.
>
> I've documented the new NULL ident behavior in tsleep.9.
>
> Also included here is a sample user, sys_nanosleep(). nanosleep(2)
> wakes up due to interruption by signal or timeout. It should never be
> awoken with wakeup(9). Up until now we had a private channel on the
> stack. Now we can just pass NULL. It's simpler.
>
> There are a bunch of other potential users but they can wait until a
> later patch.
>
> I'm running with this now so I'm pretty sure this is a sound change.
> Feel free to test it out. nanosleep(2) gets called all the time so if
> there was an issue I imagine it'd show up pretty quickly.
>
> Thoughts? ok?
The downside of this approach is that we no longer catch cases where
somebody inadvertedly uses a pointer that happens to be NULL as a
sleep channel.
I still think using a single unqieue global "nowait" variable is the
best thing to do.
> Index: share/man/man9/tsleep.9
> ===
> RCS file: /cvs/src/share/man/man9/tsleep.9,v
> retrieving revision 1.15
> diff -u -p -r1.15 tsleep.9
> --- share/man/man9/tsleep.9 20 Mar 2020 03:37:09 - 1.15
> +++ share/man/man9/tsleep.9 18 Dec 2020 19:40:04 -
> @@ -144,8 +144,11 @@ to the resource for which the process is
> The same identifier must be used in a call to
> .Fn wakeup
> to get the process going again.
> +If the thread does not want to receive any
> +.Fn wakeup
> +broadcasts,
> .Fa ident
> -should not be
> +should be
> .Dv NULL .
> .It Fa priority
> The process priority to be used when the process is awakened and put on
> Index: sys/kern/kern_synch.c
> ===
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.172
> diff -u -p -r1.172 kern_synch.c
> --- sys/kern/kern_synch.c 7 Dec 2020 16:55:29 - 1.172
> +++ sys/kern/kern_synch.c 18 Dec 2020 19:40:04 -
> @@ -119,6 +119,7 @@ tsleep(const volatile void *ident, int p
> #endif
>
> KASSERT((priority & ~(PRIMASK | PCATCH)) == 0);
> + KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
>
> #ifdef MULTIPROCESSOR
> KASSERT(timo || _kernel_lock_held());
> @@ -214,6 +215,7 @@ msleep(const volatile void *ident, struc
>
> KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
> KASSERT(mtx != NULL);
> + KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
>
> if (priority & PCATCH)
> KERNEL_ASSERT_LOCKED();
> @@ -301,6 +303,7 @@ rwsleep(const volatile void *ident, stru
> int error, status;
>
> KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
> + KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
> rw_assert_anylock(rwl);
> status = rw_status(rwl);
>
> @@ -351,8 +354,6 @@ sleep_setup(struct sleep_state *sls, con
>
tsleep(9): sleep on private channel if ident is NULL
Hi,
This patch adds support for passing NULL as the ident when calling
tsleep(9) etc. When this happens, sleep_setup() will use the address
of the sleep_state struct as the value for p_wchan. This address is
basically always a private value so the thread should never receive a
wakeup(9) broadcast.
Why do we want this? Sometimes there is no logical ident to sleep on.
Sometimes there is legitimately no reason to receive a wakeup(9).
In the past, people have handrolled private channels to work around
this situation. The code often looks like this:
void
foo(void)
{
int chan;
tsleep(&chan, ...);
}
Permitting the use of NULL and letting the implementation choose a
private channel is better than handrolling a private channel for two
reasons:
1. We save a bit of stack space. tsleep(9) etc. already have a
sleep_state struct on the stack and it's at a private address
so there is no space cost to use it.
2. The NULL clearly communicates the author's intent to the reader.
It indicates the author had no wakeup channel in mind when they
wrote the code. The reader then doesn't need to reason about
whether or not the ident value is superfluous. Poring over
a file (or several) to determine whether any thread ever calls
wakeup(9) on a given ident sucks.
FreeBSD/NetBSD have a dedicated interface for this "sleep without a
wakeup channel" operation. They call it "pause". I proposed adding
it but I got mixed feedback on the patch. Then mpi@ proposed this
idea. I think this is simpler and better.
The actual implementation requires just a few small changes to
sleep_setup().
I've added an additional KASSERT to each of tsleep(9), msleep(9), and
rwsleep(9). You now need at least one of (a) an ident or (b) PCATCH
or (c) a timeout, otherwise there is no way to get the thread started
again. This would indicate a programmer error and we should panic if
it ever happens.
I've documented the new NULL ident behavior in tsleep.9.
Also included here is a sample user, sys_nanosleep(). nanosleep(2)
wakes up due to interruption by signal or timeout. It should never be
awoken with wakeup(9). Up until now we had a private channel on the
stack. Now we can just pass NULL. It's simpler.
There are a bunch of other potential users but they can wait until a
later patch.
I'm running with this now so I'm pretty sure this is a sound change.
Feel free to test it out. nanosleep(2) gets called all the time so if
there was an issue I imagine it'd show up pretty quickly.
Thoughts? ok?
Index: share/man/man9/tsleep.9
===
RCS file: /cvs/src/share/man/man9/tsleep.9,v
retrieving revision 1.15
diff -u -p -r1.15 tsleep.9
--- share/man/man9/tsleep.9 20 Mar 2020 03:37:09 - 1.15
+++ share/man/man9/tsleep.9 18 Dec 2020 19:40:04 -
@@ -144,8 +144,11 @@ to the resource for which the process is
The same identifier must be used in a call to
.Fn wakeup
to get the process going again.
+If the thread does not want to receive any
+.Fn wakeup
+broadcasts,
.Fa ident
-should not be
+should be
.Dv NULL .
.It Fa priority
The process priority to be used when the process is awakened and put on
Index: sys/kern/kern_synch.c
===
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.172
diff -u -p -r1.172 kern_synch.c
--- sys/kern/kern_synch.c 7 Dec 2020 16:55:29 - 1.172
+++ sys/kern/kern_synch.c 18 Dec 2020 19:40:04 -
@@ -119,6 +119,7 @@ tsleep(const volatile void *ident, int p
#endif
KASSERT((priority & ~(PRIMASK | PCATCH)) == 0);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
#ifdef MULTIPROCESSOR
KASSERT(timo || _kernel_lock_held());
@@ -214,6 +215,7 @@ msleep(const volatile void *ident, struc
KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
KASSERT(mtx != NULL);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
if (priority & PCATCH)
KERNEL_ASSERT_LOCKED();
@@ -301,6 +303,7 @@ rwsleep(const volatile void *ident, stru
int error, status;
KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
+ KASSERT(ident != NULL || ISSET(priority, PCATCH) || timo != 0);
rw_assert_anylock(rwl);
status = rw_status(rwl);
@@ -351,8 +354,6 @@ sleep_setup(struct sleep_state *sls, con
#ifdef DIAGNOSTIC
if (p->p_flag & P_CANTSLEEP)
panic("sleep: %s failed insomnia", p->p_p->ps_comm);
- if (ident == NULL)
- panic("tsleep: no ident");
if (p->p_stat != SONPROC)
panic("tsleep: not SONPROC");
#endif
@@ -378,11 +379,23 @@ sleep_setup(struct sleep_state *sls, con
TRACEPOINT(sched, sleep, NULL);
- p->p_wchan = ident;
+ /*
+* If ident is NULL the caller does not want to receive
+*
