2004-06-23 18:52:15

by Dean Nelson

[permalink] [raw]
Subject: [RFC] replace assorted ASSERT()s by something officially sanctioned

It doesn't appear that an officially 'sanctioned' version of ASSERT() or
an ASSERT()-like macro exists.

And by the proliferation of its use in the linux 2.6 kernel (I saw over
3000 references to it), it would seem that BUG_ON() does not satisfy all
of the requirements of the community.

One problem with BUG_ON() is that it is always enabled. And even though
the compiler does a good job of minimizing the impact of the conditional
expression, there are times when the conditional check requires the
accessing of a cacheline that would not get accessed had the BUG_ON() not
been enabled. And if that cacheline were one that is hotly contended for,
one's performance can be adversely affected.

For debugging purposes it would be nice to have a version of BUG_ON() that
was only enabled if DEBUG was set. This is what appears to be behind the use
of the ASSERT()-like macros.

As an example of what I have in mind, I've included the following quilt
patch.

Thanks,
Dean


Index: linux/include/asm-i386/bug.h
===================================================================
--- linux.orig/include/asm-i386/bug.h
+++ linux/include/asm-i386/bug.h
@@ -21,6 +21,12 @@

#define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)

+#ifdef DEBUG
+#define DBUG_ON(condition) BUG_ON(condition)
+#else
+#define DBUG_ON(condition)
+#endif
+
#define PAGE_BUG(page) do { \
BUG(); \
} while (0)


2004-06-23 19:03:46

by Michael Buesch

[permalink] [raw]
Subject: Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> +#ifdef DEBUG
> +#define DBUG_ON(condition) BUG_ON(condition)
> +#else
> +#define DBUG_ON(condition)
> +#endif

As condition is lost when DEBUG is not defined, what about that:

#else
# define DBUG_ON(condition) do { if (condition) { /* nothing */ } } while (0)
#endif

letting the compiler optimize away all the stuff and removing
the risk of loosing an expression in ( ).

- --
Regards Michael Buesch [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFA2dPAFGK1OIvVOP4RAuPrAJ9juu+dZLSt1QjsMQeko82n9OgLqgCeN0TQ
Fec6PgrBWBRwLxl6U65QVmw=
=l7+o
-----END PGP SIGNATURE-----

2004-06-23 19:47:21

by Jeff Garzik

[permalink] [raw]
Subject: Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

Dean Nelson wrote:
> It doesn't appear that an officially 'sanctioned' version of ASSERT() or
> an ASSERT()-like macro exists.
>
> And by the proliferation of its use in the linux 2.6 kernel (I saw over
> 3000 references to it), it would seem that BUG_ON() does not satisfy all
> of the requirements of the community.
>
> One problem with BUG_ON() is that it is always enabled. And even though
> the compiler does a good job of minimizing the impact of the conditional
> expression, there are times when the conditional check requires the
> accessing of a cacheline that would not get accessed had the BUG_ON() not
> been enabled. And if that cacheline were one that is hotly contended for,
> one's performance can be adversely affected.
>
> For debugging purposes it would be nice to have a version of BUG_ON() that
> was only enabled if DEBUG was set. This is what appears to be behind the use
> of the ASSERT()-like macros.
>
> As an example of what I have in mind, I've included the following quilt
> patch.
>
> Thanks,
> Dean
>
>
> Index: linux/include/asm-i386/bug.h
> ===================================================================
> --- linux.orig/include/asm-i386/bug.h
> +++ linux/include/asm-i386/bug.h
> @@ -21,6 +21,12 @@
>
> #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); } while(0)
>
> +#ifdef DEBUG
> +#define DBUG_ON(condition) BUG_ON(condition)
> +#else
> +#define DBUG_ON(condition)
> +#endif

This won't work as it often needs to be driver-granular. Also,
WARN_ON() is often the closer implementation of assert(), than BUG_ON()

Jeff



2004-06-28 17:27:50

by Dean Nelson

[permalink] [raw]
Subject: Re: [RFC] replace assorted ASSERT()s by something officially sanctioned

On Wed, Jun 23, 2004 at 03:46:36PM -0400, Jeff Garzik wrote:
> Dean Nelson wrote:
> >--- linux.orig/include/asm-i386/bug.h
> >+++ linux/include/asm-i386/bug.h
> >@@ -21,6 +21,12 @@
> >
> > #define BUG_ON(condition) do { if (unlikely((condition)!=0)) BUG(); }
> > while(0)
> >
> >+#ifdef DEBUG
> >+#define DBUG_ON(condition) BUG_ON(condition)
> >+#else
> >+#define DBUG_ON(condition)
> >+#endif
>
> This won't work as it often needs to be driver-granular. Also,

I agree that most often the granularity of debugging needs to be at the driver
level. But I was just taking my lead from dev_dbg(), which uses '#ifdef DEBUG'
in the same way I've proposed. I would argue that the enabling/disabling of
dev_dbg() is also something one would want to control at the driver level. I'm
assuming people do this by adding a '#define DEBUG' prior to the driver's
#include of <linux/kernel.h> which has a #include of <asm/bug.h>.

The dev_printk() family of macros is a good example of what I'm interested in.
They have a set of macros (dev_err(), dev_warn(), dev_info()) that are always
enabled. Then there is dev_dbg() which is only enabled if DEBUG is defined.
(I'm not defending the choice of DEBUG as the enabling switch. I'm merely
interested in having the ability to enable/disable BUG_ON() when the driver
is built.)


> WARN_ON() is often the closer implementation of assert(), than BUG_ON()

I would agree that there are two basic flavors of ASSERT()-like macros; one
that printks some info and then Oops the system, and the other that simply
printks some info. So I would suggest providing both options, something like:

#ifdef DEBUG
#define DBUG_ON(condition) BUG_ON(condition)
#define DWARN_ON(condition) WARN_ON(condition)
#else
#define DBUG_ON(condition) do { if (condition) { /* nothing */ } } while (0)
#define DWARN_ON(condition) do { if (condition) { /* nothing */ } } while (0)
#endif

(I really don't care what the names of these macros end up being. Again, I'm
just interested in a BUG_ON()/WARN_ON()-like mechanism that is conditionally
compiled in (enabled) by some type of #ifdef switch (like DEBUG) and is
'sanctioned' by the community for use by drivers.)

Thanks,
Dean