svn commit: r244112 - head/sys/kern

[Alfred Perlstein]

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.

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.  
Don't call now, I'm taking a few hours off to rest, but please let's set 
up a time to discuss this contentious issue.

-Alfred