On Thu, Nov 18, 2021 at 07:50:01PM -0600, Scott Cheloha wrote:
> On Thu, Nov 18, 2021 at 12:30:30PM +0100, Martin Pieuchot wrote:
> > On 17/11/21(Wed) 09:51, Scott Cheloha wrote:
> > > > On Nov 17, 2021, at 03:22, Martin Pieuchot <m...@openbsd.org> wrote:
> > > >
> > > > ???On 16/11/21(Tue) 13:55, Visa Hankala wrote:
> > > >> Currently, dopselect() and doppoll() call tsleep_nsec() without retry.
> > > >> cheloha@ asked if the functions should handle spurious wakeups. I guess
> > > >> such wakeups are unlikely with the nowake wait channel, but I am not
> > > >> sure if that is a safe guess.
> > > >
> > > > I'm not sure to understand, are we afraid a thread sleeping on `nowake'
> > > > can be awaken? Is it the assumption here?
> > >
> > > Yes, but I don't know how.
> >
> > Then I'd suggest we start with understanding how this can happen otherwise
> > I fear we are adding more complexity for reasons we don't understands.
> >
> > > kettenis@ said spurious wakeups were
> > > possible on a similar loop in sigsuspend(2)
> > > so I mentioned this to visa@ off-list.
> >
> > I don't understand how this can happen.
> >
> > > If we added an assert to panic in wakeup(9)
> > > if the channel is &nowake, would that be
> > > sufficient?
> >
> > I guess so.
>
> So, something like the attached patch? All variants of wakeup(9) end
> up in wakeup_proc(), right?
>
> Wondering if it'd be better (and cheaper) to do the assert at the top
> of wakeup_n(9)...
>
> kettenis: Can you explain how a spurious wakeup would actually happen
> here or in sigsuspend(2)?
>
> Index: kern_synch.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 kern_synch.c
> --- kern_synch.c 7 Oct 2021 08:51:00 -0000 1.180
> +++ kern_synch.c 19 Nov 2021 01:41:21 -0000
> @@ -493,6 +493,8 @@ wakeup_proc(struct proc *p, const volati
> {
> int s, awakened = 0;
>
> + KASSERT(chan != &nowake);
> +
> SCHED_LOCK(s);
> if (p->p_wchan != NULL &&
> ((chan == NULL) || (p->p_wchan == chan))) {
>
To cover all wakeup point, KASSERT() should be done at:
- unsleep()
- wakeup_n()
note: wakeup_proc() will ends in calling unsleep() (setrunnable() will
call unsleep() too)
Thanks.
--
Sebastien Marie
diff a5dfec8fdaf0b4c0fdc18ec79317f74db550548a /home/semarie/repos/openbsd/src
blob - f0190261fe60789beb42bd76509517c604a487a3
file + sys/kern/kern_synch.c
--- sys/kern/kern_synch.c
+++ sys/kern/kern_synch.c
@@ -534,6 +534,8 @@ unsleep(struct proc *p)
{
SCHED_ASSERT_LOCKED();
+ KASSERT(p->p_wchan != &nowake);
+
if (p->p_wchan != NULL) {
TAILQ_REMOVE(&slpque[LOOKUP(p->p_wchan)], p, p_runq);
p->p_wchan = NULL;
@@ -553,6 +555,8 @@ wakeup_n(const volatile void *ident, int n)
struct proc *pnext;
int s;
+ KASSERT(ident != &nowake);
+
SCHED_LOCK(s);
qp = &slpque[LOOKUP(ident)];
for (p = TAILQ_FIRST(qp); p != NULL && n != 0; p = pnext) {
