[9fans] double wakeup disallowed

[9fans] double wakeup disallowed

[erik quanstrom]

i read russ' sleep history of the discussion of
sleep/wakeup on 9fans and didn't see any
references to this.

i've been wondering why wakeup requires that
some process be in state Wakeme and that
it be waiting for the particular Rendez passed
to wakeup.

i don't think this is required for correctness
(the model works with this commented out)
and only disallows waking up a process that's not
sleeping or sleeping on another Rendez.

it would seem to me useful to be able to not
make fancy provisions for avoiding double
wakeups.  is there any reason to avoid this
that i do not see?

- erik

Re: [9fans] double wakeup disallowed

[Charles Forsyth]

it isn't protecting against double wakeups, but
instead detects a bug in the code. there's an invariant
that the rendez and the process point to each other
while the process is asleep.
wakeup checks that invariant.
there are three primitives (sleep, wakeup and note)
and at different times in the past at least one of them
has been wrong; it's wise to check the invariant.

>and only disallows waking up a process that's not
>sleeping or sleeping on another Rendez.

but wakeup doesn't wakeup a particular process, it
wakes up exactly the process sleeping on the given Rendez,
so the situation can't arise.

Re: [9fans] double wakeup disallowed

[erik quanstrom]

On Wed Oct 28 05:42:25 EDT 2009, forsyth@terzarima.net wrote:
> it isn't protecting against double wakeups, but
> instead detects a bug in the code. there's an invariant
> that the rendez and the process point to each other
> while the process is asleep.
> wakeup checks that invariant.
> there are three primitives (sleep, wakeup and note)
> and at different times in the past at least one of them
> has been wrong; it's wise to check the invariant.
>
> >and only disallows waking up a process that's not
> >sleeping or sleeping on another Rendez.
>
> but wakeup doesn't wakeup a particular process, it
> wakes up exactly the process sleeping on the given Rendez,
> so the situation can't arise.

i agree that it's wise to check invariants.  however
the invarient that there is exactly one wakeup for
every sleep requires some careful accounting that
can be equally error prone.

the case i'm worried about is when two or more kprocs
think (perhaps for independent reasons) they need to
wakeup a Rendez.  it would seem to me less error prone
(and faster) for wakeup to allow this race and do nothing.

e.g., the interrupt routines of, e.g. 82563 and igbe take particular
care not to double-interrupt.  this requires some extra rts
across the pci/pcie bus.

- erik

Re: [9fans] double wakeup disallowed

[Russ Cox]

> i agree that it's wise to check invariants.  however
> the invarient that there is exactly one wakeup for
> every sleep requires some careful accounting that
> can be equally error prone.

There is no such requirement on callers.

> the case i'm worried about is when two or more kprocs
> think (perhaps for independent reasons) they need to
> wakeup a Rendez.  it would seem to me less error prone
> (and faster) for wakeup to allow this race and do nothing.

It is always legal to call wakeup on a Rendez:

    Wakeup is called by either a process or an interrupt handler
    to wake any process sleeping at r, signifying that the
    corresponding condition is true (the event has occurred).
    It has no effect if there is no sleeping process.

    [http://www.vitanuova.com/inferno/man/10/sleep.html]

Moving from documentation to code, if two different procs
call into /sys/src/9/port/proc.c:/^wakeup on the same r,
one will get the lock first.  That one will find (p = r->p) != nil,
ready(p), and set r->p = nil..  The next will find (p = r->p) == nil
and do nothing.

> e.g., the interrupt routines of, e.g. 82563 and igbe take particular
> care not to double-interrupt.  this requires some extra rts
> across the pci/pcie bus.

If you are referring to the dance with ctlr->im, I suspect that
has much more to do with the peculiarities of the card than
the calling convention for wakeup.

Russ

Re: [9fans] double wakeup disallowed

[Charles Forsyth]

>however the invarient that there is exactly one wakeup for
>every sleep requires some careful accounting that
>can be equally error prone.care not to double-interrupt.

if a process p sleeps on r for condition f, and there are two wakeup(r), only
the first wakeup does anything because by the time of the second,
r doesn't refer to p any more. were you wanting r to retain memory of p so
the second wakeup would ... presumably still not do anything? (because
p wouldn't be in the right state.) if so, i don't see what you've gained.
i must be missing something.

Re: [9fans] double wakeup disallowed

[erik quanstrom]

> if a process p sleeps on r for condition f, and there are two wakeup(r), only
> the first wakeup does anything because by the time of the second,
> r doesn't refer to p any more. were you wanting r to retain memory of p so
> the second wakeup would ... presumably still not do anything? (because
> p wouldn't be in the right state.) if so, i don't see what you've gained.
> i must be missing something.

i agree with you.  i'm the one who is missing something.
the case i thought i was seeing — double wakeup leading
to a panic — can't be happening.  there's something else
going on.

thanks for the thoughtful responses to a dumb question.

btw, isn't the lockstats.locks++ in taslock:/^lock
broken since >1 loads can happen simultaneously
leading to undercounting?

- erik

Re: [9fans] double wakeup disallowed

[Russ Cox]

> btw, isn't the lockstats.locks++ in taslock:/^lock
> broken since >1 loads can happen simultaneously
> leading to undercounting?

sure but does it need to be 100% accurate?

russ

Re: [9fans] double wakeup disallowed

[erik quanstrom]

On Fri Oct 30 01:47:06 EDT 2009, rsc@swtch.com wrote:
> > btw, isn't the lockstats.locks++ in taslock:/^lock
> > broken since >1 loads can happen simultaneously
> > leading to undercounting?
>
> sure but does it need to be 100% accurate?

probablly not.  but it will be most inaccurate
and cause the most performance impact when
there's a lot of locking going on.  i would think
that's exactly when it would be most interesting.
adding cores will make this worse, as does the
false sharing of the lockstats.

(i've seen some really wild results in my aoe
testing.)

so wouldn't xinc provide more consistent performance?
there would be at most 1 wait for the lockstats cacheline
per stat updated instead of 2.

- erik