Date: Mon, 25 Feb 2002 18:01:10 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: Terry Lambert <tlambert2@mindspring.com>, Alfred Perlstein <alfred@FreeBSD.ORG>, Bosko Milekic <bmilekic@unixdaemons.com>, Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp>, <current@FreeBSD.ORG>, John Baldwin <jhb@FreeBSD.ORG> Subject: Re: Patch for critical_enter()/critical_exit() & interrupt assembly revamp, please review! Message-ID: <20020225164356.Q39518-100000@gamplex.bde.org> In-Reply-To: <200202250035.g1P0ZFT25722@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 24 Feb 2002, Matthew Dillon wrote:
> :On Sun, 24 Feb 2002, Matthew Dillon wrote:
> :> * Additional cpu_critical_enter/exit() calls around icu_lock. Since
> :> critical_enter() no longer disables interrupts, special care must
> :> be taken when dealing with the icu_lock spin mutex because it is
> :> the one thing the interrupt code needs to be able to defer the
> :> interrupt.
> :
> :clock_lock needs these too, at least in my version where the soft
> :critical_enter() doesn't mask fast interrupts.
>
> I wasn't sure whether it was due to that, or whether you just wanted
> the two I8254 accesses to be right next to each other. Since I am
> msking fast interrupts I think we are safe.
Hmm, I thought I wrote more about this somewhere. We are fairly safe,
but we may be interrupted for a few microseconds, and i8254_get_timecount()
has depends on the latency being at most 20 usec (the hard-coded 20
was correctly parametrized as TIMER0_LATCH_COUNT in the old assembler
version. This has rotted to TIMER0_LATCH_COUNT being #defined,
documented but not used). Actually, we have more serious latency
problems here. i8254_get_timecount() also depends on clk0 interrupts
not being disabled for 20 usec _before_ it is called (interrupt handlers
can call it, but they must not have interrupts enabled at the time of
the call). -current probably violates this by running with spinlocks
held for more than 20 usec and calling i8254_get_timecount() from at
least the witness code on old machines which have the least to spare.
Your changes help, but clk0 interrupts will probably be delayed by 20
usec in some cases.
> :IIRC, in my version, the hard interrupt enablement in fork_exit() is
> :only needed at boot time. Something neglects to do it earlier in that
> :case, at least for some threads. I was surprised by this -- hard
> :interrupts are enabled early in the boot, and everything later should
> :disable and enable them reentrantly.
>
> I couldn't figure out who was leaving hard interrupts enabled. Did
> you ever figure out who it was? At the moment I am able to enable
No; I don't remember even trying.
> interrupts in the trampoline code just before the call to
> fork_exit(), but it has to be disabled on entry to the trampoline
> code because the task's td_critnest count is still 0 at that
> point and we can't afford to execute a real (fast) interrupt's
> service routine until we clean up the sched_lock.
I think td_critnest should be set a little earlier. Not sure why it
isn't already.
> In regards to i8254_get_timecounter() the 8254 latches the full
> count on the first access so we are theoretically safe. I'll
> change my timecounter to use the 8254 to see if it still works
> as expected.
I use pcaudio for stress testing the i8254 timecounter. It sets the
clock frequency to 16 KHz. Setting HZ to this should work almost as
well in -current. HZ can be set even faster.
> :> /*
> :> + * Critical section handling.
> :...
> :i386/machdep.c is a different wrong place for this and unpend(). I put it
> :in isa/ithread.c. It is not very MD, and is even less isa-dependent.
>
> Yah. I'm all ears as to where to put it :-) There's no good place
> to put inline versions of critical_enter() or critical_exit() either.
> machine/cpufunc.h doesn't have access to the struct thread.
I would keep it in isa/sched_ithd.c until the correct MI parts are found.
> p.s. all discussion here and below, except for the bintr/eintr cleanup, would
> be for a second work/commit round after I commit the current stuff.
I hope that leaves only 10-20K of changes :-).
> :> Index: i386/isa/apic_vector.s
> :> ===================================================================
> :> RCS file: /home/ncvs/src/sys/i386/isa/apic_vector.s,v
> :> retrieving revision 1.75
> :> diff -u -r1.75 apic_vector.s
> :> --- i386/isa/apic_vector.s 5 Jan 2002 08:47:10 -0000 1.75
> :> +++ i386/isa/apic_vector.s 24 Feb 2002 17:58:34 -0000
> :
> :Please do this without so many changes to the assembly code. Especially
> :not the reordering and style changes. I think I see how to do this
> :(see call_fast_unpend())
>
> No such luck. In order to be able to mask fast interrupts I
> had to move all the macros from between fast and slow ints to
> above fast ints.
Erm, macros don't need to be defined before macros that invoke them.
> I also decided to tab-out the backslashes in the icu_vector
> code, because I couldn't read the code easily with the
> backslashes slammed up against the instructions :-(
This uglyness has been maintained since 1992 to keep diffs somewhat
readable. I really don't want to change it now. Even the existence
of the macros is mostly a relic from 1992.
> :> +void
> :> +call_fast_unpend(int irq)
> :> +{
> :> + fastunpend[irq]();
> :> +}
> :> +
> :
> :I think this can be done using with few changes to the assembler by
> :using INTREN(irq) followed by a call to the usual Xfastintr[irq] entry
> :point, or (perhaps after a few adjustments) even to the driver's entry
> :point...
> I was going to do this but the trap frame is passed to the
> fast interrupt function. I could either do an inline 'int'
> instruction or simulate the trap frame. For the slow
> interrupt unpend code I of course just call sched_ithd()
> directly.
>
> :RELENG_4 still has complications related to this and I was pleased to
> :see them go away in -current. See vec0, vec8 and BUILD_VEC() in
> :icu_ipl.s (the vecN routines are stubs to avoid duplication of Xintr*).
> :-current made things simpler for normal interrupt handlers, and fast
> :interrupt handlers are simpler at this level except for the problem
> :with the frame.
>
> Yup!
Oops, I missed the part of the patch corresponding to vec0 and vec8
It is too many layers deep in the PUSH_DUMMY macro. PUSH_DUMMY is
just missing the %eip adjustment (it gives the too-dummy %eip of 0).
A non-dummy %eip is good for accounting, profiling and backtraces
in ddb.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020225164356.Q39518-100000>