On Sat, Apr 28, 2007 at 11:07:06AM -0400, der Mouse wrote:

> >> 	while (! (sc->flags & SCF_INTERRUPTED))
> >> 		;
> 
> >> (Yes, it's somewhat unrealistic; top halves rarely busy-wait like
> >> that.  But that's an efficiency issue; absent volatile, there's a
> >> correctness issue as well.)
> > Yes, there's a correctness issue.  If a top half is going to look at
> > data that is shared with the bottom half, it must lock out the bottom
> > half to do so.
> 
> This is not actually necessary.  (If NetBSD wants to make it true by
> fiat for NetBSD code, that's fine, but it should not be confused with
> any underlying necessity.)
>
> Provided stores and reads are serialized in some order, with each
> accessor's accesses performed in the order they're issued, it's possible
> to interlock without additional help.
> 
> While this is not suitable for general-purpose use, it's perfectly
> reasonable for cases where the types of contention that are likely to
> arise are appropriate.

In the NetBSD kernel it's unsafe to busy wait for another thread of
execution without releasing the kernel lock (or more generally, any
locks that are held):

	KERNEL_UNLOCK_ALL(curlwp, &lockcount);
 	while (!(sc->flags & SCF_INTERRUPTED))
 		;
	KERNEL_LOCK(lockcount, curlwp);

> > That code is simply bogus; it needs to look more like this:
> >
> > s = splfoo()
> >   while (! (sc->flags & SCF_INTERRUPTED)) {
> >         tsleep();
> >   }
> > splx(s);

If you are targeting -current:

	mutex_enter(&foo_lock);
	while ((sc->flags & SCF_INTERRUPTED) == 0)
		cv_wait(&foo_cv, &foo_lock);
	mutex_exit(&foo_lock);

:-)

> Except it actually doesn't - provided sc->flags is volatile.  And
> indeed it quite likely is not a good idea for it to, if the loop does
> not loop more than some small number of times on the average (or if
> this is early enough that the scheduler behind tsleep() isn't up).

In the context of progamming device drivers, the "early enough" caveat is
a bug in the autoconfiguration system that we need to eliminate.

> > ... noting that the bottom half which can write to sc->flags is
> > blocked out while it is being examined.
> 
> Still doesn't help if the data being examined is file-static and does
> not have its address stored anywhere global and thus is not modifiable
> by tsleep() according to the C virtual machine; the compiler is then
> perfectly within its rights to keep the flags word in a register across
> the tsleep().

Unless it's a local/automatic variable the compiler must assume that it can
be modified across function calls.

Cheers,
Andrew