svn commit: r184293 - in head/sys: amd64/amd64 i386/i386

[Bruce Evans]

On Mon, 27 Oct 2008, John Baldwin wrote:

> On Sunday 26 October 2008 02:58:04 pm Maxim Sobolev wrote:
>> Author: sobomax
>> Date: Sun Oct 26 18:58:04 2008
>> New Revision: 184293
>> URL: http://svn.freebsd.org/changeset/base/184293
>>
>> Log:
>>   Fix division by zero panic if kern.hz less than 32.
>>
>>   MFC after:	1 day
>
> This is wrong.  In the case you are worried about here, lapic_timer_hz is less
> than 128.  There is no way you are going to fire stathz 128 times per second
> from a timer running at < 128 hz.  You are effectively running stathz at
> lapic_timer_hz, so I would just set stathz = lapic_timer_hz in this case.

stathz needs to be about 128 to work as intended, at least for SCHED_4BSD.

> Also, I would drop the extra {}'s to match style(9) as well as the existing
> style of the file.

I noticed this bug in the main commit too.

Also, hz = 10 cannot work on i386 without lapic_timer, since the i8254 timer
has a maximum interrupt period of 55 ms and thus a minimum frequency of
18.2 Hz.  Attempts to set it to 10 Hz cause similar bugs to the ones here
-- the best approximation of 18.2 is (supposed to be) used, but the system
is not informed about the enormous error in this approximation and still
thinks that 10 Hz is used.

I use hz = stathz = 100 = lapic_timer_hz on all systems with lapic
timer, mainly to avoid the large changes to scale all the timers
perfectly.  (This makes profhz = lapic_timer_hz = 100 more broken than
before.  Perfect scaling rarely matters, and asynchronicity of the
timers is broken anyway.)  hz != 100 is still supported in a simple way
that makes stathz = 100 (and lapic_timer_hz unnecessarily large --
1000) if hz < 100:

 	lapic_timer_hz = howmany(imax(hz, 100), hz) * hz;
 	stathz = lapic_timer_hz /
 	    (lapic_timer_hz < 100 ? 1 : lapic_timer_hz / 100);

>> --- head/sys/amd64/amd64/local_apic.c	Sun Oct 26 17:20:37 2008	(r184292)
>> +++ head/sys/amd64/amd64/local_apic.c	Sun Oct 26 18:58:04 2008	(r184293)
>> @@ -401,7 +401,11 @@ lapic_setup_clock(void)
>>  		lapic_timer_hz = hz * 2;
>>  	else
>>  		lapic_timer_hz = hz * 4;
>> -	stathz = lapic_timer_hz / (lapic_timer_hz / 128);
>> +	if (lapic_timer_hz < 128) {
>> +		stathz = 128;
>> +	} else {
>> +		stathz = lapic_timer_hz / (lapic_timer_hz / 128);
>> +	}

Use of if/else instead a conditional expression as in the above may be
another style bug here.

Bruce