Patchwork [08/10] dynamic_debug: make netif_dbg() call __netdev_printk()

login
register
mail settings
Submitter Jason Baron
Date July 6, 2011, 5:25 p.m.
Message ID <889f3300a96f381aee1239ea775014fff26d93c9.1309967232.git.root@dhcp-100-18-164.bos.redhat.com>
Download mbox | patch
Permalink /patch/103548/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Jason Baron - July 6, 2011, 5:25 p.m.
From: Jason Baron <jbaron@redhat.com>

Previously, netif_dbg() was using dynamic_dev_dbg() to perform
the underlying printk. Fix it to use __netdev_printk(), instead.

Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 include/linux/dynamic_debug.h |   12 ++++++++++++
 include/linux/netdevice.h     |    6 ++----
 2 files changed, 14 insertions(+), 4 deletions(-)
Joe Perches - July 6, 2011, 9:59 p.m.
On Wed, 2011-07-06 at 13:25 -0400, Jason Baron wrote:
> From: Jason Baron <jbaron@redhat.com>
> 
> Previously, netif_dbg() was using dynamic_dev_dbg() to perform
> the underlying printk. Fix it to use __netdev_printk(), instead.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  include/linux/dynamic_debug.h |   12 ++++++++++++
>  include/linux/netdevice.h     |    6 ++----
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
[]
> +#define dynamic_netif_dbg(dev, cond, fmt, ...) do {			\
> +	static struct _ddebug descriptor				\
> +	__used								\
> +	__attribute__((section("__verbose"), aligned(8))) =		\
> +	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
> +		_DPRINTK_FLAGS_DEFAULT };				\
> +	if (unlikely(descriptor.enabled)) {				\
> +		if (cond)						\
> +			__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
> +	}								\
> +	} while (0)
> +

Just nits:

I think it'd be better to use
#define dynamic_netif_dbg(etc)						\
do {									\
	etc...
} while (0)

so that there aren't 2 consecutive close braces at the same indent level.

and maybe just use one test

	if (unlikely(descriptor.enabled) && cond)
		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);


> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9b132ef..99c358f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2731,10 +2731,8 @@ do {								\
>  #elif defined(CONFIG_DYNAMIC_DEBUG)
>  #define netif_dbg(priv, type, netdev, format, args...)		\
>  do {								\
> -	if (netif_msg_##type(priv))				\
> -		dynamic_dev_dbg((netdev)->dev.parent,		\
> -				"%s: " format,			\
> -				netdev_name(netdev), ##args);	\
> +	dynamic_netif_dbg(netdev, (netif_msg_##type(priv)),	\
> +				  format, ##args);		\

Because you've already added dynamic_netdev_dbg,
maybe this should be:

#define netif_dbg(priv, type, netdev, format, args...)		\
do {								\
	if (netif_msg_##type(priv))				\
		dynamic_netdev_dbg(netdev, format, ##args);	\
} while (0)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Baron - July 7, 2011, 2:13 p.m.
On Wed, Jul 06, 2011 at 02:59:03PM -0700, Joe Perches wrote:
> On Wed, 2011-07-06 at 13:25 -0400, Jason Baron wrote:
> > From: Jason Baron <jbaron@redhat.com>
> > 
> > Previously, netif_dbg() was using dynamic_dev_dbg() to perform
> > the underlying printk. Fix it to use __netdev_printk(), instead.
> > 
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Jason Baron <jbaron@redhat.com>
> > ---
> >  include/linux/dynamic_debug.h |   12 ++++++++++++
> >  include/linux/netdevice.h     |    6 ++----
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> []
> > +#define dynamic_netif_dbg(dev, cond, fmt, ...) do {			\
> > +	static struct _ddebug descriptor				\
> > +	__used								\
> > +	__attribute__((section("__verbose"), aligned(8))) =		\
> > +	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
> > +		_DPRINTK_FLAGS_DEFAULT };				\
> > +	if (unlikely(descriptor.enabled)) {				\
> > +		if (cond)						\
> > +			__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
> > +	}								\
> > +	} while (0)
> > +
> 
> Just nits:
> 
> I think it'd be better to use
> #define dynamic_netif_dbg(etc)						\
> do {									\
> 	etc...
> } while (0)
> 
> so that there aren't 2 consecutive close braces at the same indent level.
> 
> and maybe just use one test
> 
> 	if (unlikely(descriptor.enabled) && cond)
> 		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);
> 

If you look at the next patch, 9/10, I've combined the tests there
just as you've described. I agree, that it would be better if that were
folded into this patch. will fix.

> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 9b132ef..99c358f 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -2731,10 +2731,8 @@ do {								\
> >  #elif defined(CONFIG_DYNAMIC_DEBUG)
> >  #define netif_dbg(priv, type, netdev, format, args...)		\
> >  do {								\
> > -	if (netif_msg_##type(priv))				\
> > -		dynamic_dev_dbg((netdev)->dev.parent,		\
> > -				"%s: " format,			\
> > -				netdev_name(netdev), ##args);	\
> > +	dynamic_netif_dbg(netdev, (netif_msg_##type(priv)),	\
> > +				  format, ##args);		\
> 
> Because you've already added dynamic_netdev_dbg,
> maybe this should be:
> 
> #define netif_dbg(priv, type, netdev, format, args...)		\
> do {								\
> 	if (netif_msg_##type(priv))				\
> 		dynamic_netdev_dbg(netdev, format, ##args);	\
> } while (0)
> 
> 

The reason I didn't add it this way is b/c I plan on converting the
outer 'ifs' to the jump label infrastructure - which makes the disabled
case just a no-op and moves the printk and tests out of line.

Until that is done, i could see coding it as you've suggested, but I'd
prefer to leave it as is (and leave future churn to within the dynamic
debug code as opposed to the netdevice.h header).

Thanks,

-Jason

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - July 7, 2011, 4:29 p.m.
On Thu, 2011-07-07 at 10:13 -0400, Jason Baron wrote:
> On Wed, Jul 06, 2011 at 02:59:03PM -0700, Joe Perches wrote:
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > @@ -2731,10 +2731,8 @@ do {								\
> > >  #elif defined(CONFIG_DYNAMIC_DEBUG)
> > >  #define netif_dbg(priv, type, netdev, format, args...)		\
> > >  do {								\
> > > -	if (netif_msg_##type(priv))				\
> > > -		dynamic_dev_dbg((netdev)->dev.parent,		\
> > > -				"%s: " format,			\
> > > -				netdev_name(netdev), ##args);	\
> > > +	dynamic_netif_dbg(netdev, (netif_msg_##type(priv)),	\
> > > +				  format, ##args);		\
> > Because you've already added dynamic_netdev_dbg,
> > maybe this should be:
> > #define netif_dbg(priv, type, netdev, format, args...)		\
> > do {								\
> > 	if (netif_msg_##type(priv))				\
> > 		dynamic_netdev_dbg(netdev, format, ##args);	\
> > } while (0)
> The reason I didn't add it this way is b/c I plan on converting the
> outer 'ifs' to the jump label infrastructure - which makes the disabled
> case just a no-op and moves the printk and tests out of line.

Perhaps you needn't do that.

I think there's little to be gained to move the test
outwards and not perform the netif_msg##type(priv)

> Until that is done, i could see coding it as you've suggested, but I'd
> prefer to leave it as is (and leave future churn to within the dynamic
> debug code as opposed to the netdevice.h header).

Shrug.  I think that dynamic_debug will have continuing
impacts on various subsystems unless there's some generic
__dynamic_dbg() and _prefix() mechanism introduced into
more generic <foo>_dbg style.

Anything logging message that uses <foo>_dbg or <foo>_vdbg
is a candidate for dynamic_debug uses, but there's no
current generic mechanism to avoid subsystem specific needs.

Any of these could need some dynamic_debug consideration:

$ grep -rPoh --include=*.[ch] "[a-z_]+_[v]?dbg\(" * | sort | uniq
acpi_ut_allocate_object_desc_dbg(
acpi_ut_create_internal_object_dbg(
adc_dbg(
add_dyn_dbg(
airo_print_dbg(
ata_dev_dbg(
ata_link_dbg(
ata_port_dbg(
ath_dbg(
atm_dbg(
bat_dbg(
bit_dbg(
cafe_dev_dbg(
cam_dbg(
c_freq_dbg(
chan_dbg(
chan_reg_rule_print_dbg(
cmm_dbg(
c_pm_dbg(
ctrl_dbg(
__dbg(
desc_dbg(
dev_dbg(
dev_vdbg(
dma_request_channel_dbg(
__dump_desc_dbg(
dump_desc_dbg(
dump_pq_desc_dbg(
dynamic_dev_dbg(
e_dbg(
ehca_dbg(
ehca_gen_dbg(
ehci_dbg(
ehci_vdbg(
en_dbg(
ep_dbg(
ep_vdbg(
fhci_dbg(
fhci_vdbg(
fit_dbg(
gig_dbg(
gpio_dbg(
gru_dbg(
hgpk_dbg(
hid_dbg(
hw_dbg(
ibmvfc_dbg(
ipath_dbg(
ipoib_dbg(
ipr_dbg(
iser_dbg(
isp_isr_dbg(
itd_dbg(
ite_dbg(
l_dbg(
lg_dbg(
mce_dbg(
memblock_dbg(
mhwmp_dbg(
mpeg_dbg(
mpl_dbg(
msg_dbg(
mthca_dbg(
netdev_dbg(
netdev_vdbg(
netif_dbg(
netif_vdbg(
nfc_dbg(
nfc_dev_dbg(
nsp_dbg(
nvt_dbg(
ohci_dbg(
ohci_vdbg(
oxu_dbg(
oxu_vdbg(
pch_dbg(
pch_pci_dbg(
pm_dev_dbg(
pnp_dbg(
pop_dbg(
prep_dma_pq_dbg(
prep_dma_pqzero_sum_dbg(
prep_dma_xor_dbg(
_print_dbg(
print_dbg(
pwm_dbg(
rdev_dbg(
reg_dbg(
sh_keysc_map_dbg(
slice_dbg(
smsc_dbg(
start_dbg(
stop_dbg(
sysrq_handle_dbg(
tda_dbg(
tgt_dbg(
tipc_msg_dbg(
__tuner_dbg(
tuner_dbg(
tveeprom_dbg(
tx_dbg(
uea_dbg(
uea_vdbg(
ugeth_dbg(
ugeth_vdbg(
urb_dbg(
usb_dbg(
vpif_dbg(
wiphy_dbg(
wiphy_vdbg(
xhci_dbg(
x_show_dbg(


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Baron - July 7, 2011, 6:09 p.m.
On Thu, Jul 07, 2011 at 09:29:21AM -0700, Joe Perches wrote:
> On Thu, 2011-07-07 at 10:13 -0400, Jason Baron wrote:
> > On Wed, Jul 06, 2011 at 02:59:03PM -0700, Joe Perches wrote:
> > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > @@ -2731,10 +2731,8 @@ do {								\
> > > >  #elif defined(CONFIG_DYNAMIC_DEBUG)
> > > >  #define netif_dbg(priv, type, netdev, format, args...)		\
> > > >  do {								\
> > > > -	if (netif_msg_##type(priv))				\
> > > > -		dynamic_dev_dbg((netdev)->dev.parent,		\
> > > > -				"%s: " format,			\
> > > > -				netdev_name(netdev), ##args);	\
> > > > +	dynamic_netif_dbg(netdev, (netif_msg_##type(priv)),	\
> > > > +				  format, ##args);		\
> > > Because you've already added dynamic_netdev_dbg,
> > > maybe this should be:
> > > #define netif_dbg(priv, type, netdev, format, args...)		\
> > > do {								\
> > > 	if (netif_msg_##type(priv))				\
> > > 		dynamic_netdev_dbg(netdev, format, ##args);	\
> > > } while (0)
> > The reason I didn't add it this way is b/c I plan on converting the
> > outer 'ifs' to the jump label infrastructure - which makes the disabled
> > case just a no-op and moves the printk and tests out of line.
> 
> Perhaps you needn't do that.
> 
> I think there's little to be gained to move the test
> outwards and not perform the netif_msg##type(priv)

In this particualr case, there might not be a large gain, but when I've
converted all of the dynamic debug infrastructure to jump labels I can
consistently see througput gains of 1% on tbench testing.

> 
> > Until that is done, i could see coding it as you've suggested, but I'd
> > prefer to leave it as is (and leave future churn to within the dynamic
> > debug code as opposed to the netdevice.h header).
> 
> Shrug.  I think that dynamic_debug will have continuing
> impacts on various subsystems unless there's some generic
> __dynamic_dbg() and _prefix() mechanism introduced into
> more generic <foo>_dbg style.
> 
> Anything logging message that uses <foo>_dbg or <foo>_vdbg
> is a candidate for dynamic_debug uses, but there's no
> current generic mechanism to avoid subsystem specific needs.
> 
> Any of these could need some dynamic_debug consideration:
> 

right. looking quickly over this list there seem to be a few different
categories:

-some just alias to dev_dbg(), so they are already picked up
-some use level logging, this could be easily added to dyanmic debug -
  we store level info in the descriptor and then check it against
  a currently set level, which can be per-debug statement
-any ones that can't fit the current model could probably be easily
 converted using a callback, That is we have some dynamic debug
 function take an optional function, which if the debugging is enabled
 is called. The format string, could optionally be blank in this case,
 I guess.

So I think it could be converted while being minimaly invasive in terms
of run-time checking. In fact, that was one of my original goals was to
try and convert all the disparate debugging calls, to a more generic
infrastructure. I know some subsystem converted to use pr_debug(), to
tie into dynamic debug, but it would take a bit of work to convert the
rest...thoughts?

thanks,

-Jason


> $ grep -rPoh --include=*.[ch] "[a-z_]+_[v]?dbg\(" * | sort | uniq
> acpi_ut_allocate_object_desc_dbg(
> acpi_ut_create_internal_object_dbg(
> adc_dbg(
> add_dyn_dbg(
> airo_print_dbg(
> ata_dev_dbg(
> ata_link_dbg(
> ata_port_dbg(
> ath_dbg(
> atm_dbg(
> bat_dbg(
> bit_dbg(
> cafe_dev_dbg(
> cam_dbg(
> c_freq_dbg(
> chan_dbg(
> chan_reg_rule_print_dbg(
> cmm_dbg(
> c_pm_dbg(
> ctrl_dbg(
> __dbg(
> desc_dbg(
> dev_dbg(
> dev_vdbg(
> dma_request_channel_dbg(
> __dump_desc_dbg(
> dump_desc_dbg(
> dump_pq_desc_dbg(
> dynamic_dev_dbg(
> e_dbg(
> ehca_dbg(
> ehca_gen_dbg(
> ehci_dbg(
> ehci_vdbg(
> en_dbg(
> ep_dbg(
> ep_vdbg(
> fhci_dbg(
> fhci_vdbg(
> fit_dbg(
> gig_dbg(
> gpio_dbg(
> gru_dbg(
> hgpk_dbg(
> hid_dbg(
> hw_dbg(
> ibmvfc_dbg(
> ipath_dbg(
> ipoib_dbg(
> ipr_dbg(
> iser_dbg(
> isp_isr_dbg(
> itd_dbg(
> ite_dbg(
> l_dbg(
> lg_dbg(
> mce_dbg(
> memblock_dbg(
> mhwmp_dbg(
> mpeg_dbg(
> mpl_dbg(
> msg_dbg(
> mthca_dbg(
> netdev_dbg(
> netdev_vdbg(
> netif_dbg(
> netif_vdbg(
> nfc_dbg(
> nfc_dev_dbg(
> nsp_dbg(
> nvt_dbg(
> ohci_dbg(
> ohci_vdbg(
> oxu_dbg(
> oxu_vdbg(
> pch_dbg(
> pch_pci_dbg(
> pm_dev_dbg(
> pnp_dbg(
> pop_dbg(
> prep_dma_pq_dbg(
> prep_dma_pqzero_sum_dbg(
> prep_dma_xor_dbg(
> _print_dbg(
> print_dbg(
> pwm_dbg(
> rdev_dbg(
> reg_dbg(
> sh_keysc_map_dbg(
> slice_dbg(
> smsc_dbg(
> start_dbg(
> stop_dbg(
> sysrq_handle_dbg(
> tda_dbg(
> tgt_dbg(
> tipc_msg_dbg(
> __tuner_dbg(
> tuner_dbg(
> tveeprom_dbg(
> tx_dbg(
> uea_dbg(
> uea_vdbg(
> ugeth_dbg(
> ugeth_vdbg(
> urb_dbg(
> usb_dbg(
> vpif_dbg(
> wiphy_dbg(
> wiphy_vdbg(
> xhci_dbg(
> x_show_dbg(
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - July 7, 2011, 9:55 p.m.
On Thu, 2011-07-07 at 14:09 -0400, Jason Baron wrote:
> On Thu, Jul 07, 2011 at 09:29:21AM -0700, Joe Perches wrote:
> > I think there's little to be gained to move the test
> > outwards and not perform the netif_msg##type(priv)
> In this particualr case, there might not be a large gain, but when I've
> converted all of the dynamic debug infrastructure to jump labels I can
> consistently see througput gains of 1% on tbench testing.

And that's not this case is it.
I don't see any value here.

[]

> I think that dynamic_debug will have continuing
> > impacts on various subsystems unless there's some generic
> > __dynamic_dbg() and _prefix() mechanism introduced into
> > more generic <foo>_dbg style.
> > Anything logging message that uses <foo>_dbg or <foo>_vdbg
> > is a candidate for dynamic_debug uses, but there's no
> > current generic mechanism to avoid subsystem specific needs.
> > Any of these could need some dynamic_debug consideration:
> right. looking quickly over this list there seem to be a few different
> categories:
> -some just alias to dev_dbg(), so they are already picked up
> -some use level logging, this could be easily added to dyanmic debug -
>   we store level info in the descriptor and then check it against
>   a currently set level, which can be per-debug statement

Fine by me.

That might also make all other netif_<type>()
and <foo>_<level>(bitmap or level test, fmt, ...)
possible to combine in this mechanism as well.

There are a lot of those.

> -any ones that can't fit the current model could probably be easily
>  converted using a callback, That is we have some dynamic debug
>  function take an optional function, which if the debugging is enabled
>  is called.

I believe that would require some registration mechanism
for modules.

> In fact, that was one of my original goals was to
> try and convert all the disparate debugging calls, to a more generic
> infrastructure. I know some subsystem converted to use pr_debug(), to
> tie into dynamic debug, but it would take a bit of work to convert the
> rest...thoughts?

Go for it.
You're the ddebug maintainer.
I'm gladly review though.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index feaac1e..7048e64 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -84,6 +84,18 @@  extern int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
 	} while (0)
 
+#define dynamic_netif_dbg(dev, cond, fmt, ...) do {			\
+	static struct _ddebug descriptor				\
+	__used								\
+	__attribute__((section("__verbose"), aligned(8))) =		\
+	{ KBUILD_MODNAME, __func__, __FILE__, fmt, __LINE__,		\
+		_DPRINTK_FLAGS_DEFAULT };				\
+	if (unlikely(descriptor.enabled)) {				\
+		if (cond)						\
+			__dynamic_netdev_dbg(&descriptor, dev, fmt, ##__VA_ARGS__);\
+	}								\
+	} while (0)
+
 #else
 
 static inline int ddebug_remove_module(const char *mod)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9b132ef..99c358f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2731,10 +2731,8 @@  do {								\
 #elif defined(CONFIG_DYNAMIC_DEBUG)
 #define netif_dbg(priv, type, netdev, format, args...)		\
 do {								\
-	if (netif_msg_##type(priv))				\
-		dynamic_dev_dbg((netdev)->dev.parent,		\
-				"%s: " format,			\
-				netdev_name(netdev), ##args);	\
+	dynamic_netif_dbg(netdev, (netif_msg_##type(priv)),	\
+				  format, ##args);		\
 } while (0)
 #else
 #define netif_dbg(priv, type, dev, format, args...)			\