Date: Fri, 14 Dec 2012 11:49:07 -0500 From: John Baldwin <jhb@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, Alfred Perlstein <alfred@freebsd.org>, svn-src-all@freebsd.org, Alfred Perlstein <bright@mu.org>, Andriy Gapon <avg@freebsd.org>, src-committers@freebsd.org, Navdeep Parhar <np@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r244112 - head/sys/kern Message-ID: <201212141149.07671.jhb@freebsd.org> In-Reply-To: <20121213090215.GP97487@FreeBSD.org> References: <201212110708.qBB78EWx025288@svn.freebsd.org> <50C9271C.70803@mu.org> <20121213090215.GP97487@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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(). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201212141149.07671.jhb>