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 _______________________________________________ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
