[PATCH] Statclock aliasing by LAPIC

[Attilio Rao]

2009/11/30 John Baldwin <jhb at freebsd.org>:
> On Friday 27 November 2009 6:42:50 pm Attilio Rao wrote:
>> Handling all the three clocks (hardclock, softclock, profclock) within
>> the LAPIC can lead to aliasing for the softclock and profclock because
>> hz is sized to fit mainly hardclock. The fashion to handle all of them
>> within the LAPIC was introduced in 2005 and before than the softclock
>> and profclock were supposed to be handled in the rtc. Right now, too,
>> there is the necessary support to handle profclock and statclock in
>> atrtc which gets enabled if the LAPIC signals it can't take in charge
>> the three clocks.
>> The proposed patch reverts the situation preferring the atrtc to
>> handle the statclock and profclock (then a different source from the
>> LAPIC) and then avoid the aliasing problem:
>>
> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/statclock_aliasing/statclock_aliasing3.diff
>>
>> In this patch, lapic_setup_clock() has been changed in order to return
>> a three-states variable which identified if the LAPIC got in charge
>> all the three clocks, just the hardclock or none of them (the current
>> situation is just none/all) and the rtc handling runs subsequently.
>> A tunable as been added to force LAPI to get in charge all the three
>> clocks if needed.
>> In ia32 atrtc compiling is linked to atpic compiling, so a compile
>> time flag has been added to check if atpic is not present and in case
>> force LAPIC to take in charge all the three clocks (which is still
>> better than the 'safe belt values' still present in the rtc code).
>>
>> Please note that statclock and profclock are widely used in our kernel
>> (rusage is, for example, statclock driven) and fixing this would
>> result in specific improvements (as a several-reported wrong CPU usage
>> statistic in top).
>> This bug has been found by Sandvine Incorporated.
>>
>> Reviews, comments and testing are welcome.
>
> Presumably in the RTC case lapic_timer_hz should always be hz and not some
> multiple of hz.  Also, did you check to make sure all the lock is present?  I
> think at one point I changed the locking for the RTC and/or ISA timer to just
> use critical_enter/exit so that UP machines running an SMP kernel wouldn't pay
> the locking overhead since the code was only used on UP machines.  It may very
> well be that I only changed that in a development branch though and not in
> HEAD.

I still see clock_lock in place (and no particular critical section
code in that paths) or you meant to say that the clock_lock doesn't
still provide enough protection alone?
BTW, you were right about the lapic_timer_hz (I forgot to revert to
hz). There is an updated patch:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/statclock_aliasing/statclock_aliasing4.diff

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein