Date: Mon, 17 Dec 2012 14:38:05 -0500 From: John Baldwin <jhb@freebsd.org> To: Alfred Perlstein <bright@mu.org> Cc: Adrian Chadd <adrian@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, Alfred Perlstein <alfred@freebsd.org>, Andriy Gapon <avg@freebsd.org>, Gleb Smirnoff <glebius@freebsd.org>, Navdeep Parhar <np@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r244112 - head/sys/kern Message-ID: <201212171438.05577.jhb@freebsd.org> In-Reply-To: <50CB8B0C.2060908@mu.org> References: <201212110708.qBB78EWx025288@svn.freebsd.org> <201212141149.07671.jhb@freebsd.org> <50CB8B0C.2060908@mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, December 14, 2012 3:24:44 pm Alfred Perlstein wrote: > On 12/14/12 8:49 AM, John Baldwin wrote: > > On Thursday, December 13, 2012 4:02:15 am Gleb Smirnoff wrote: > >> On Wed, Dec 12, 2012 at 04:53:48PM -0800, Alfred Perlstein wrote: > >> A> The problem again is that not all the KASSERTS are inviolable, if you > >> A> want to do a project to split them, then please do, it would really be > >> A> helpful, as for now, they are a mis-mash of death/warnings and there are > >> A> at least three vendors who approve of this as well as 3 long term > >> A> committers that approved my change (not including Adrian). > >> > >> Can you show examples of not inviolable KASSERTs? > > There are none. > > They are all assertions for a reason. However, in my > > experience at several large consumers of FreeBSD, no one wants to run with > > INVARIANTS in production. Not because we don't want panics (believe me, > > Yahoo! gets plenty of panics even with INVARIANTS disabled), but because the > > performance overhead, and redefining INVARIANTS into printf doesn't address > > that at all. > > > > Also, in regards to "if you think an a condition should be inviolable, make it > > a panic". I _did_ do that in WITNESS and you just reverted them! In all the > > cases of things like mismatching slock -> xlock you are about to corrupt > > WITNESS' internal state (leading to false positives or missed warnings) as > > well as the state of the locks themselves resulting in either hangs or random > > data corruption. > > > > Also, if you don't have a console wired up on all your machines (which not > > everyone does these days) a hang is _far_ worse than a crashdump, as when the > > machine hangs, you have to power cycle it, and you won't find the messages in > > /var/log/messages. With a straight-up panic if someone wants to run with > > INVARIANTS enabled they would instead have a nice crashdump that could be > > examined after the machine comes back up that points to the offending > > location. > > > > So in general, I will never use this and find it doesn't add any benefit > > whatsoever. OTOH, if it is not invasive (e.g. your original commit), then I > > think it might be ok if there are some folks who might actually find it > > useful. That said, I think direct use of kassert_panic() such as your changes > > to WITNESS is wrong. If you are going to change explicit panics, make them > > into a KASSERT() instead of changing a panic into a kassert_panic(). > > > Would that not break WITNESS without INVARIANTS. > > I can get a kernel to compile with a functional WITNESS without > INVARIANTS, however with this proposed change, then WITNESS wouldn't do > anything (not even log) without INVARIANTS. > > We can probably cobble up a WITNESS_ASSERT for witness if you'd like > that does the right thing based on ifdef INVARIANTS. > > Let me know what you think of that. Ugh, that isn't really any better than directly using kassert_panic(). Ideally the kasserts-that-aren't stuff would be transparent, but if that isn't possible directly calling kassert_panic() is probably less gross than the other alternatives. > I've also given you my personal phone number so we can hash this out > over the phone, but you have not called nor responded to that email. E-mail is functional, nor do I think that it is appropriate to exclude other folks on this thread from the conversation. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201212171438.05577.jhb>