svn commit: r244112 - head/sys/kern

[John Baldwin]

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