svn commit: r213985 - head/sys/sparc64/sparc64

[Marius Strobl]

On Mon, Oct 18, 2010 at 05:05:24PM -0400, John Baldwin wrote:
> On Monday, October 18, 2010 4:52:24 pm Marius Strobl wrote:
> > On Mon, Oct 18, 2010 at 11:28:13PM +0300, Alexander Motin wrote:
> > > Marius Strobl wrote:
> > > > On Mon, Oct 18, 2010 at 10:03:12AM -0400, John Baldwin wrote:
> > > >> On Sunday, October 17, 2010 12:46:54 pm Marius Strobl wrote:
> > > >>> Author: marius
> > > >>> Date: Sun Oct 17 16:46:54 2010
> > > >>> New Revision: 213985
> > > >>> URL: http://svn.freebsd.org/changeset/base/213985
> > > >>>
> > > >>> Log:
> > > >>>   - In oneshot-mode it doesn't make sense to try to compensate the clock
> > > >>>     drift in order to achieve a more stable clock as the tick intervals may
> > > >>>     vary in the first place. In fact I haven't seen this code kick in when
> > > >>>     in oneshot-mode so just skip it in that case.
> > > >>>   - There's no need to explicitly stop the (S)TICK counter in oneshot-mode
> > > >>>     with every tick as it just won't trigger again with the (S)TICK compare
> > > >>>     register set to a value in the past (with a wrap-around once every ~195
> > > >>>     years of uptime at 1.5 GHz this isn't something we have to worry about
> > > >>>     in practice).
> > > >>>   - Given that we'll disable interrupts completely anyway there's no
> > > >>>     need to enter critical sections.
> > > >> This last is not entirely true.  The purpose of the critical section is to 
> > > >> prevent the kernel from preempting to the softclock swi thread until all of 
> > > >> the hardclock handler has finished execution.  Thus, places that actually 
> > > >> actually call hardclock() should probably still be wrapped in a critical
> > > >> section.
> > > > 
> > > > It's currently unclear to me how on architectures converted to the
> > > > event timer world order hardclock() is called eventually but in any case
> > > > shouldn't it be the responsibility of the code actually calling it (or
> > > > the equivalent code) to wrap it in a critical section instead then? After
> > > > all the MD part just enrolls in calling _something_ in one-shot and/or
> > > > periodic mode without knowing what it actually calls (and IMO it also
> > > > should no longer need to). In handleevents() of kern_clocksource.c
> > > > hardclock_anycpu() is called so i think that is what actually needs to
> > > > be wrapped in a critical section.
> > > 
> > > At this time on most (all?) platforms critical section is grabbed by MD
> > > interrupt code. It is important to be there, as soon as there touched
> > > td_intr_nesting_level and td_intr_frame fields of curthread. We can't
> > > allow thread migration until all counted interrupt handlers complete.
> > > 
> > 
> > AFAICT this is not true; intr_event_handle() in sys/kern/kern_intr.c
> > is what enters a critical section and f.e. on amd64 I don't see where
> > anywhere in the path from ISR_VEC() to intr_execute_handlers()
> > calling intr_event_handle() a critical section would be entered,
> > which also means that in intr_execute_handlers() td_intr_nesting_level
> > is incremented outside of a critical section.
> 
> Not all of the clock interrupts use intr_event_handle().  The local APIC
> timer uses its own interrupt entry point on x86 for example and uses an
> explicit critical section as a result.  I suspect the sparc64 tick interrupt
> is closer to the local APIC timer case and doesn't use intr_event_handle().

Correct; but still you can't say that the MD interrupt code enters a
critical section in general, neither is incrementing td_intr_nesting_level
in intr_execute_handlers() protected by a critical section.

> 
> The fact that some clock interrupts do use intr_event_handle() (e.g. the
> atrtc driver on x86 now) does indicate that the low-level interrupt code
> probably does not belong in the time events code but in the caller.
> 

Well, I agree that entering a critical section in the time events
code would mean entering a nested critical section unnecessarily in
case the clock driver uses a regular "fast" interrupt handler and
that should be avoided. Still I don't think the event time front-end
actually should need to worry about wrapping the callback in a
critical section.

Marius