> On Nov 18, 2021, at 23:59, Sebastien Marie <sema...@online.fr> wrote:
>
> 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)
This doesn't look right, this will panic.
We only want to block outside callers from
waking threads sleeping on &nowake via
wakeup(9), wakeup_one(9), wakeup_n(9),
and wakeup_proc(9).
We still need to be able to unsleep() those
threads when they catch a signal or the
sleep timeout fires or their process exits.
