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

[John Baldwin]

On Monday, October 18, 2010 5:41:57 pm Alexander Motin wrote:
> Marius Strobl wrote:
> > 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:
> >>> 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.
> 
> Interrupt frame, required for hard-/stat-/profclock() operation is
> stored in curthread. So critical section is effectively mandatory there
> now. Correct td_intr_nesting_level value is also important for proper
> interrupt threads scheduling - one more reason to have critical section
> there. It is indeed strange that td_intr_nesting_level in
> intr_event_handle() is not covered by critical section, but probably it
> should.

I don't see why the interrupt frame logic requires bumping
td_intr_nesting_level.  I agree with bde@ that in general
td_intr_nesting_level is useless (it is always 0 for most code and always 1
for filters and the ithread code that schedules an ithread).  It does not
need any protection for interrupts since the CPU has already disabled
nesting interrupts.  The critical_enter()'s sole purpose is to avoid a kind
of priority inversion to ensure that the very high priority interrupt code
(filters, etc.) is not preempted by an ithread.

As far as the interrupt frame logic, the logic should have no need of
td_intr_nesting_level.  An interrupt should just save the current value of
td_intr_frame before setting it and restore the saved value afterwards.  No
need for dealing with td_intr_nesting_level at all.

-- 
John Baldwin