KASSERT: always assert; KWARN

[Conrad Meyer]

I'd like to logically revert r243980 and r244105, such that KASSERT
uses the __dead2-annotated panic(9).

Going back to the old behavior enables Coverity and other static
analyzers to reason about KASSERT invariants via the __dead2 panic(9)
path.

This proposal is in https://reviews.freebsd.org/D6117 .

As a follow-up, to match the assumed intent of the r243980 changes, I
propose a KWARN facility which may be muted, rate limited, or even
cause panic. Generally, KASSERTs should not be KWARNs. That proposal
is here: https://reviews.freebsd.org/D6134

Finally, I am looking for suggestions of things it *does* make sense
to KWARN about. One suggestion was witness_warn; however, it doesn't
seem like a great fit (without adding allocating sbufs in, anyway). A
sketch of that is in https://reviews.freebsd.org/D6306 .

Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?

Best,
Conrad

[Adrian Chadd]

i found it very useful to get asserts to print, rather than panic.



-a

Post by Conrad Meyer
I'd like to logically revert r243980 and r244105, such that KASSERT
uses the __dead2-annotated panic(9).
Going back to the old behavior enables Coverity and other static
analyzers to reason about KASSERT invariants via the __dead2 panic(9)
path.
This proposal is in https://reviews.freebsd.org/D6117 .
As a follow-up, to match the assumed intent of the r243980 changes, I
propose a KWARN facility which may be muted, rate limited, or even
cause panic. Generally, KASSERTs should not be KWARNs. That proposal
is here: https://reviews.freebsd.org/D6134
Finally, I am looking for suggestions of things it *does* make sense
to KWARN about. One suggestion was witness_warn; however, it doesn't
seem like a great fit (without adding allocating sbufs in, anyway). A
sketch of that is in https://reviews.freebsd.org/D6306 .
Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?
Best,
Conrad
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch

[K. Macy]

Ugh. Et tu Brutus. Then they either shouldn't be asserts or you need a new
mode for Adrian and friends. Either way, if people don't want to hit panics
from asserts they should not run with invariants enabled. No one other than
manic depressive developers look at logs in the common case.

Post by Adrian Chadd
i found it very useful to get asserts to print, rather than panic.
-a

Post by Conrad Meyer
I'd like to logically revert r243980 and r244105, such that KASSERT
uses the __dead2-annotated panic(9).
Going back to the old behavior enables Coverity and other static
analyzers to reason about KASSERT invariants via the __dead2 panic(9)
path.
This proposal is in https://reviews.freebsd.org/D6117 .
As a follow-up, to match the assumed intent of the r243980 changes, I
propose a KWARN facility which may be muted, rate limited, or even
cause panic. Generally, KASSERTs should not be KWARNs. That proposal
is here: https://reviews.freebsd.org/D6134
Finally, I am looking for suggestions of things it *does* make sense
to KWARN about. One suggestion was witness_warn; however, it doesn't
seem like a great fit (without adding allocating sbufs in, anyway). A
sketch of that is in https://reviews.freebsd.org/D6306 .
Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?
Best,
Conrad
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch

<javascript:;>"
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
<javascript:;>"

[Alfred Perlstein]

Post by Conrad Meyer
Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?

Yes.

During my time at iXsystems we used this facility several times to get a
log from a customer site with a number of "kasserts".

The reason we did this was multiple reasons:
1) We needed to ship a kernel with asserts enabled.
2) When we did, the first assert hit was:
a) In an unrelated module and not relevant.
b) Not enough information came back from just the first assert.
3) We found it more useful to get multiple errors back from a customer
in one trip rather than one fix at a time. Unfortunately one fix at a
time would have had us lose the customer.

The KASSERT/assert system is very, very, very useful. However if you
are at a last resort sending a debug kernel (with Kassert enabled) and
do not get enough information back then you will lose that customer.

I understand that a few vocal folks are upset, like seriously, seriously
upset, however at the time this was the only way we could effectively
debug a customer problem and my hope was that others could make use of
it as well.

Linux has had a similar functionality for many years. In Linux there is
the kernel "oops()" which does nearly the same thing.

Initially I mocked Linux's "oops" for being silly and "wrong", using the
exact same reasons that many have used to dislike "kassert_warn".
However once I was responsible for an extremely pissed off customer who
was paying us quite a sum of money AND I was not getting enough
information back, I changed my mind.

https://en.wikipedia.org/wiki/Linux_kernel_oops


-Alfred

[Conrad Meyer]

Post by Conrad Meyer
Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?

Yes.
During my time at iXsystems we used this facility several times to get a log
from a customer site with a number of "kasserts".
1) We needed to ship a kernel with asserts enabled.
a) In an unrelated module and not relevant.
b) Not enough information came back from just the first assert.
3) We found it more useful to get multiple errors back from a customer in
one trip rather than one fix at a time. Unfortunately one fix at a time
would have had us lose the customer.
The KASSERT/assert system is very, very, very useful. However if you are at
a last resort sending a debug kernel (with Kassert enabled) and do not get
enough information back then you will lose that customer.
I understand that a few vocal folks are upset, like seriously, seriously
upset, however at the time this was the only way we could effectively debug
a customer problem and my hope was that others could make use of it as well.
Linux has had a similar functionality for many years. In Linux there is the
kernel "oops()" which does nearly the same thing.
Initially I mocked Linux's "oops" for being silly and "wrong", using the
exact same reasons that many have used to dislike "kassert_warn". However
once I was responsible for an extremely pissed off customer who was paying
us quite a sum of money AND I was not getting enough information back, I
changed my mind.
https://en.wikipedia.org/wiki/Linux_kernel_oops

Hi,

Here's my follow-up from the Phabricator review. (Alfred, you've
already seen it. But, for everyone else:)

Here's another proposal:

Add a mode between INVARIANTS off and INVARIANTS on. Call it
INVARIANTS_OPTIONAL.

* In !INVARIANTS mode, you don't have KASSERTs at all (like today).

* In INVARIANTS && INVARIANTS_OPTIONAL mode, you get the all the
print-and-continue KASSERT knobs you have today. (So, same default of
panicing, but optionally they can be disabled and turned into logs.)

* In INVARIANTS && !INVARIANTS_OPTIONAL mode, KASSERT always panics.

I would suggest GENERIC move from the current mode, effectively
INVARIANTS_OPTIONAL, to !INVARIANTS_OPTIONAL. But if you want to ship
a kernel with pass-through assertions enabled, you can still do that
by enabling INVARIANTS_OPTIONAL.

This gives the expected KASSERT behavior for Coverity modeling, and
still enables the KASSERT-lite use case. (It would just be kicked out
of GENERIC.)

Adrian, would that meet your needs?

Thanks,
Conrad

[John Baldwin]

Post by Conrad Meyer

Post by Conrad Meyer
Thoughts or objections? Does anyone like the ability to opt out of
invariants asserts?

Yes.
During my time at iXsystems we used this facility several times to get a log
from a customer site with a number of "kasserts".
1) We needed to ship a kernel with asserts enabled.
a) In an unrelated module and not relevant.
b) Not enough information came back from just the first assert.
3) We found it more useful to get multiple errors back from a customer in
one trip rather than one fix at a time. Unfortunately one fix at a time
would have had us lose the customer.
The KASSERT/assert system is very, very, very useful. However if you are at
a last resort sending a debug kernel (with Kassert enabled) and do not get
enough information back then you will lose that customer.
I understand that a few vocal folks are upset, like seriously, seriously
upset, however at the time this was the only way we could effectively debug
a customer problem and my hope was that others could make use of it as well.
Linux has had a similar functionality for many years. In Linux there is the
kernel "oops()" which does nearly the same thing.
Initially I mocked Linux's "oops" for being silly and "wrong", using the
exact same reasons that many have used to dislike "kassert_warn". However
once I was responsible for an extremely pissed off customer who was paying
us quite a sum of money AND I was not getting enough information back, I
changed my mind.
https://en.wikipedia.org/wiki/Linux_kernel_oops

Hi,
Here's my follow-up from the Phabricator review. (Alfred, you've
already seen it. But, for everyone else:)
Add a mode between INVARIANTS off and INVARIANTS on. Call it
INVARIANTS_OPTIONAL.
* In !INVARIANTS mode, you don't have KASSERTs at all (like today).
* In INVARIANTS && INVARIANTS_OPTIONAL mode, you get the all the
print-and-continue KASSERT knobs you have today. (So, same default of
panicing, but optionally they can be disabled and turned into logs.)
* In INVARIANTS && !INVARIANTS_OPTIONAL mode, KASSERT always panics.
I would suggest GENERIC move from the current mode, effectively
INVARIANTS_OPTIONAL, to !INVARIANTS_OPTIONAL. But if you want to ship
a kernel with pass-through assertions enabled, you can still do that
by enabling INVARIANTS_OPTIONAL.
This gives the expected KASSERT behavior for Coverity modeling, and
still enables the KASSERT-lite use case. (It would just be kicked out
of GENERIC.)
Adrian, would that meet your needs?

Eh, if you keep going past many of the assertions the original code
enabled, you will get _more_ bogus assertions as fallout. For example,
if you aren't holding the correct locks (or you try to unlock a lock
locked in a different mode), then you are going to start corrupting data
resulting in false positives. This is like getting 20 compiler errors
due to missing a semicolon earlier in the file. The first error is
real, the rest are noise. Sometimes there are actual errors in the noise,
but you have to sort through a lot of noise. However, the compiler doesn't
normally mangle the rest of your source code after your first error, but
int the case of assertions the kernel often _is_ going to start mangling
your data after your first failure.

If you have things that aren't actual errors but for which you _handle_
the unexpected case instead of just blindly corrupting data, then you
can use KWARN for those cases. However, you have to actually handle the
condition for that to be safe.

In your case of the cdrom driver, if you weren't using it, then turn it
off (i.e. take it out of the kernel) if you don't have time to debug it.

[Rui Paulo]

Post by John Baldwin
Eh, if you keep going past many of the assertions the original code
enabled, you will get _more_ bogus assertions as fallout.

I agree with John.  We really shouldn't be fiddling with this.

There's just one case where this could be useful: you just imported a
bunch of code that you don't understand and you're using the kassert's
as a way to create a mental model of the code.  However, this is a very
specific edge case.

[Alfred Perlstein]

Post by John Baldwin
Eh, if you keep going past many of the assertions the original code
enabled, you will get _more_ bogus assertions as fallout.

I agree with John. We really shouldn't be fiddling with this.
There's just one case where this could be useful: you just imported a
bunch of code that you don't understand and you're using the kassert's
as a way to create a mental model of the code. However, this is a very
specific edge case.

I'm not sure what you and John are suggesting here.

Are you suggesting to revert the explicit panics in the witness code, or
to revert the entire kassert_warn subsystem?

-Alfred