| Message ID | 889f3300a96f381aee1239ea775014fff26d93c9.1309967232.git.root@dhcp-100-18-164.bos.redhat.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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...) \