Date: Mon, 27 Oct 2008 16:25:36 -0400 From: John Baldwin <jhb@freebsd.org> To: Maxim Sobolev <sobomax@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r184293 - in head/sys: amd64/amd64 i386/i386 Message-ID: <200810271625.36538.jhb@freebsd.org> In-Reply-To: <4906185C.1010900@FreeBSD.org> References: <200810261858.m9QIw4YV091893@svn.freebsd.org> <200810271411.11813.jhb@freebsd.org> <4906185C.1010900@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Monday 27 October 2008 03:37:00 pm Maxim Sobolev wrote: > 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. > > Also, I would drop the extra {}'s to match style(9) as well as the existing > > style of the file. > > It might be wrong, but it's better than division by zero panic. And it > actually works, check this screenshot: > > http://sobomax.sippysoft.com/~sobomax/ScreenShot362.png You didn't read a word I wrote apparently. :( > [ssp-root@sippy ~]$ sysctl -a | grep hz > kern.clockrate: { hz = 10, tick = 100000, profhz = 40, stathz = 128 } Yes, so now the kernel lies to userland, congratulations. :( stathz is running at 40hz, but userland will think it runs at 128, so if any code tries to actually use stathz directly (e.g. divide some stat counter by stathz, etc.) it will compute wrong results. > As for the style(9), I am not really sure that applies, the exact quote: > > <quote> > Space after keywords (if, while, for, return, switch). No braces (`{' > and `}') are used for control statements with zero or only a single > statement unless that statement is more than a single line in which case > they are permitted. Forever loops are done with for's, not while's. > </quote> > > To me the following if: > > if () > stmt1; > else > stmt2; > > falls into the category of statements with more than single line (3 in > this case), so that {} is appropriate. So that either my change is OK, > or style(9) needs to be clarified to include if/else explicitly. 1) I disagree with your interpretation. For one, 'else' is clearly not a statement by itself. In fact, if you check the BNF in "The C Programming Language", both "if (expression) statement" and "if (expression) statement else statement" are part of the "selection-statement" definition. So rather than 'else' being a statement, the entire thing is a single 'statement'. 2) The bigger rule is to be consistent within a file. Look at the 4 lines immediately preceding your change. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200810271625.36538.jhb>