Re: [PATCH] qed: Add and use specific logging functions to reduce object size
On Wed, 2016-07-27 at 07:24 +, Yuval Mintz wrote: > > > > Current DP_ macros generate a lot of code. > > Using functions with vsprintf extension %pV helps reduce that size. > > > > drivers/net/ethernet/qlogic/qed/Makefile | 2 +- > > drivers/net/ethernet/qlogic/qed/qed_util.c | 82 > > ++ > > include/linux/qed/qed_if.h | 60 +- > > 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 > > drivers/net/ethernet/qlogic/qed/qed_util.c > This won't compile when CONFIG_QED*=m, as qede can't link to > the new qed_{err, verbose, info, notice} functions. > That was the original reason for putting the macros in the interface > header. > > Other alternatives: > - We can EXPORT_SYMBOL() the functions, although we've taken a > strain not adding such to the interface. > - Code duplication between qed/qede [ugly]. > - Implementing this in qede via the interface functions with qed; > But the notion of defining an interface between 2 modules passing > va_args seems [to me] like a bad one. > > If you have cleaner solutions, I'd be happy to hear those. Hello Yuval. EXPORT_SYMBOL is probably the simplest solution. It's pretty commonly used for private logging functions in drivers/net/, e.g.: drivers/net/wireless/ath/main.c I'll submit that in awhile. cheers, Joe
Re: [PATCH] qed: Add and use specific logging functions to reduce object size
On Wed, 2016-07-27 at 07:24 +, Yuval Mintz wrote: > > > > Current DP_ macros generate a lot of code. > > Using functions with vsprintf extension %pV helps reduce that size. > > > > drivers/net/ethernet/qlogic/qed/Makefile | 2 +- > > drivers/net/ethernet/qlogic/qed/qed_util.c | 82 > > ++ > > include/linux/qed/qed_if.h | 60 +- > > 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 > > drivers/net/ethernet/qlogic/qed/qed_util.c > This won't compile when CONFIG_QED*=m, as qede can't link to > the new qed_{err, verbose, info, notice} functions. > That was the original reason for putting the macros in the interface > header. > > Other alternatives: > - We can EXPORT_SYMBOL() the functions, although we've taken a > strain not adding such to the interface. > - Code duplication between qed/qede [ugly]. > - Implementing this in qede via the interface functions with qed; > But the notion of defining an interface between 2 modules passing > va_args seems [to me] like a bad one. > > If you have cleaner solutions, I'd be happy to hear those. Hello Yuval. EXPORT_SYMBOL is probably the simplest solution. It's pretty commonly used for private logging functions in drivers/net/, e.g.: drivers/net/wireless/ath/main.c I'll submit that in awhile. cheers, Joe
Re: [PATCH] qed: Add and use specific logging functions to reduce object size
Hi, [auto build test ERROR on net-next/master] [also build test ERROR on v4.7 next-20160729] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Joe-Perches/qed-Add-and-use-specific-logging-functions-to-reduce-object-size/20160727-063300 config: i386-randconfig-sb0-07310616 (attached as .config) compiler: gcc-5 (Debian 5.4.0-6) 5.4.0 20160609 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): >> ERROR: "qed_err" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_verbose" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_info" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_notice" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH] qed: Add and use specific logging functions to reduce object size
Hi, [auto build test ERROR on net-next/master] [also build test ERROR on v4.7 next-20160729] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Joe-Perches/qed-Add-and-use-specific-logging-functions-to-reduce-object-size/20160727-063300 config: i386-randconfig-sb0-07310616 (attached as .config) compiler: gcc-5 (Debian 5.4.0-6) 5.4.0 20160609 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): >> ERROR: "qed_err" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_verbose" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_info" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! >> ERROR: "qed_notice" [drivers/net/ethernet/qlogic/qede/qede.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
RE: [PATCH] qed: Add and use specific logging functions to reduce object size
> > Current DP_ macros generate a lot of code. > > Using functions with vsprintf extension %pV helps reduce that size. > > Yuval, I used the same KERN_ output types, but it is unusual > that DP_INFO outputs at KERN_NOTICE. > > Was that a copy/paste defect or should it be emitted at KERN_INFO and > DP_VERBOSE be emitted at KERN_DEBUG? I agree it's a bit odd, but it's actually by design - as none of these prints would ever be reached unless user explicitly enable them [ethtool/module param], the assumption is that NOTICE is good enough, i.e., it will prevent the need for doing additional configuration of logging levels by the user.
RE: [PATCH] qed: Add and use specific logging functions to reduce object size
> > Current DP_ macros generate a lot of code. > > Using functions with vsprintf extension %pV helps reduce that size. > > Yuval, I used the same KERN_ output types, but it is unusual > that DP_INFO outputs at KERN_NOTICE. > > Was that a copy/paste defect or should it be emitted at KERN_INFO and > DP_VERBOSE be emitted at KERN_DEBUG? I agree it's a bit odd, but it's actually by design - as none of these prints would ever be reached unless user explicitly enable them [ethtool/module param], the assumption is that NOTICE is good enough, i.e., it will prevent the need for doing additional configuration of logging levels by the user.
RE: [PATCH] qed: Add and use specific logging functions to reduce object size
> Current DP_ macros generate a lot of code. > Using functions with vsprintf extension %pV helps reduce that size. > > drivers/net/ethernet/qlogic/qed/Makefile | 2 +- > drivers/net/ethernet/qlogic/qed/qed_util.c | 82 > ++ > include/linux/qed/qed_if.h | 60 +- > 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 > drivers/net/ethernet/qlogic/qed/qed_util.c This won't compile when CONFIG_QED*=m, as qede can't link to the new qed_{err, verbose, info, notice} functions. That was the original reason for putting the macros in the interface header. Other alternatives: - We can EXPORT_SYMBOL() the functions, although we've taken a strain not adding such to the interface. - Code duplication between qed/qede [ugly]. - Implementing this in qede via the interface functions with qed; But the notion of defining an interface between 2 modules passing va_args seems [to me] like a bad one. If you have cleaner solutions, I'd be happy to hear those.
RE: [PATCH] qed: Add and use specific logging functions to reduce object size
> Current DP_ macros generate a lot of code. > Using functions with vsprintf extension %pV helps reduce that size. > > drivers/net/ethernet/qlogic/qed/Makefile | 2 +- > drivers/net/ethernet/qlogic/qed/qed_util.c | 82 > ++ > include/linux/qed/qed_if.h | 60 +- > 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 > drivers/net/ethernet/qlogic/qed/qed_util.c This won't compile when CONFIG_QED*=m, as qede can't link to the new qed_{err, verbose, info, notice} functions. That was the original reason for putting the macros in the interface header. Other alternatives: - We can EXPORT_SYMBOL() the functions, although we've taken a strain not adding such to the interface. - Code duplication between qed/qede [ugly]. - Implementing this in qede via the interface functions with qed; But the notion of defining an interface between 2 modules passing va_args seems [to me] like a bad one. If you have cleaner solutions, I'd be happy to hear those.
Re: [PATCH] qed: Add and use specific logging functions to reduce object size
On Tue, 2016-07-26 at 14:25 -0700, Joe Perches wrote: > Current DP_ macros generate a lot of code. > Using functions with vsprintf extension %pV helps reduce that size. Yuval, I used the same KERN_ output types, but it is unusual that DP_INFO outputs at KERN_NOTICE. Was that a copy/paste defect or should it be emitted at KERN_INFO and DP_VERBOSE be emitted at KERN_DEBUG? > define DP_INFO(cdev, fmt, ...) \ > - do { \ > - if (unlikely((cdev)->dp_level <= QED_LEVEL_INFO)) { \ > - pr_notice("[%s:%d(%s)]" fmt, \ > - __func__, __LINE__, \ > - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ > - ## __VA_ARGS__); \ > - } \ > - } while (0) [] > -#define DP_VERBOSE(cdev, module, fmt, ...) \ > - do {\ > - if (unlikely(((cdev)->dp_level <= QED_LEVEL_VERBOSE) && \ > - ((cdev)->dp_module & module))) { \ > - pr_notice("[%s:%d(%s)]" fmt,\ > - __func__, __LINE__, \ > - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ > - ## __VA_ARGS__); \ > - } \ > - } while (0)
Re: [PATCH] qed: Add and use specific logging functions to reduce object size
On Tue, 2016-07-26 at 14:25 -0700, Joe Perches wrote: > Current DP_ macros generate a lot of code. > Using functions with vsprintf extension %pV helps reduce that size. Yuval, I used the same KERN_ output types, but it is unusual that DP_INFO outputs at KERN_NOTICE. Was that a copy/paste defect or should it be emitted at KERN_INFO and DP_VERBOSE be emitted at KERN_DEBUG? > define DP_INFO(cdev, fmt, ...) \ > - do { \ > - if (unlikely((cdev)->dp_level <= QED_LEVEL_INFO)) { \ > - pr_notice("[%s:%d(%s)]" fmt, \ > - __func__, __LINE__, \ > - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ > - ## __VA_ARGS__); \ > - } \ > - } while (0) [] > -#define DP_VERBOSE(cdev, module, fmt, ...) \ > - do {\ > - if (unlikely(((cdev)->dp_level <= QED_LEVEL_VERBOSE) && \ > - ((cdev)->dp_module & module))) { \ > - pr_notice("[%s:%d(%s)]" fmt,\ > - __func__, __LINE__, \ > - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ > - ## __VA_ARGS__); \ > - } \ > - } while (0)
[PATCH] qed: Add and use specific logging functions to reduce object size
Current DP_ macros generate a lot of code. Using functions with vsprintf extension %pV helps reduce that size. $ size drivers/net/ethernet/qlogic/built-in.o* (x86-64) textdata bss dec hex filename 165161 28470 32812 226443 3748b drivers/net/ethernet/qlogic/built-in.o.defconfig.new 190473 28470 32812 251755 3d76b drivers/net/ethernet/qlogic/built-in.o.defconfig.old 1215984 257822 39712 1513518 17182e drivers/net/ethernet/qlogic/built-in.o.allyesconfig.new 1262402 284334 39712 1586448 183510 drivers/net/ethernet/qlogic/built-in.o.allyesconfig.old Signed-off-by: Joe Perches--- drivers/net/ethernet/qlogic/qed/Makefile | 2 +- drivers/net/ethernet/qlogic/qed/qed_util.c | 82 ++ include/linux/qed/qed_if.h | 60 +- 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 drivers/net/ethernet/qlogic/qed/qed_util.c diff --git a/drivers/net/ethernet/qlogic/qed/Makefile b/drivers/net/ethernet/qlogic/qed/Makefile index d1f157e..9d7e55f 100644 --- a/drivers/net/ethernet/qlogic/qed/Makefile +++ b/drivers/net/ethernet/qlogic/qed/Makefile @@ -2,5 +2,5 @@ obj-$(CONFIG_QED) := qed.o qed-y := qed_cxt.o qed_dev.o qed_hw.o qed_init_fw_funcs.o qed_init_ops.o \ qed_int.o qed_main.o qed_mcp.o qed_sp_commands.o qed_spq.o qed_l2.o \ -qed_selftest.o qed_dcbx.o +qed_selftest.o qed_dcbx.o qed_util.o qed-$(CONFIG_QED_SRIOV) += qed_sriov.o qed_vf.o diff --git a/drivers/net/ethernet/qlogic/qed/qed_util.c b/drivers/net/ethernet/qlogic/qed/qed_util.c new file mode 100644 index 000..2795e63 --- /dev/null +++ b/drivers/net/ethernet/qlogic/qed/qed_util.c @@ -0,0 +1,82 @@ +#include +#include + +#include "qed.h" + +void qed_err(const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_err("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_notice(u32 level, const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_NOTICE)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_info(u32 level, const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_INFO)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_verbose(u32 level, const char *name, int line, +const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_VERBOSE)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h index b1e3c57..fa3d8c0 100644 --- a/include/linux/qed/qed_if.h +++ b/include/linux/qed/qed_if.h @@ -539,44 +539,30 @@ struct qed_common_ops { #define GET_FIELD(value, name) \ (((value) >> (name ## _SHIFT)) & name ## _MASK) -/* Debug print definitions */ -#define DP_ERR(cdev, fmt, ...) \ - pr_err("[%s:%d(%s)]" fmt,\ - __func__, __LINE__, \ - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ - ## __VA_ARGS__) \ - -#define DP_NOTICE(cdev, fmt, ...)\ - do { \ - if (unlikely((cdev)->dp_level <= QED_LEVEL_NOTICE)) { \ - pr_notice("[%s:%d(%s)]" fmt, \ - __func__, __LINE__, \ - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ - ## __VA_ARGS__);\ - \ - } \ - } while (0) - -#define DP_INFO(cdev, fmt, ...) \ - do {
[PATCH] qed: Add and use specific logging functions to reduce object size
Current DP_ macros generate a lot of code. Using functions with vsprintf extension %pV helps reduce that size. $ size drivers/net/ethernet/qlogic/built-in.o* (x86-64) textdata bss dec hex filename 165161 28470 32812 226443 3748b drivers/net/ethernet/qlogic/built-in.o.defconfig.new 190473 28470 32812 251755 3d76b drivers/net/ethernet/qlogic/built-in.o.defconfig.old 1215984 257822 39712 1513518 17182e drivers/net/ethernet/qlogic/built-in.o.allyesconfig.new 1262402 284334 39712 1586448 183510 drivers/net/ethernet/qlogic/built-in.o.allyesconfig.old Signed-off-by: Joe Perches --- drivers/net/ethernet/qlogic/qed/Makefile | 2 +- drivers/net/ethernet/qlogic/qed/qed_util.c | 82 ++ include/linux/qed/qed_if.h | 60 +- 3 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 drivers/net/ethernet/qlogic/qed/qed_util.c diff --git a/drivers/net/ethernet/qlogic/qed/Makefile b/drivers/net/ethernet/qlogic/qed/Makefile index d1f157e..9d7e55f 100644 --- a/drivers/net/ethernet/qlogic/qed/Makefile +++ b/drivers/net/ethernet/qlogic/qed/Makefile @@ -2,5 +2,5 @@ obj-$(CONFIG_QED) := qed.o qed-y := qed_cxt.o qed_dev.o qed_hw.o qed_init_fw_funcs.o qed_init_ops.o \ qed_int.o qed_main.o qed_mcp.o qed_sp_commands.o qed_spq.o qed_l2.o \ -qed_selftest.o qed_dcbx.o +qed_selftest.o qed_dcbx.o qed_util.o qed-$(CONFIG_QED_SRIOV) += qed_sriov.o qed_vf.o diff --git a/drivers/net/ethernet/qlogic/qed/qed_util.c b/drivers/net/ethernet/qlogic/qed/qed_util.c new file mode 100644 index 000..2795e63 --- /dev/null +++ b/drivers/net/ethernet/qlogic/qed/qed_util.c @@ -0,0 +1,82 @@ +#include +#include + +#include "qed.h" + +void qed_err(const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_err("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_notice(u32 level, const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_NOTICE)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_info(u32 level, const char *name, int line, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_INFO)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} + +void qed_verbose(u32 level, const char *name, int line, +const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + if (likely(level > QED_LEVEL_VERBOSE)) + return; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = + + pr_notice("[%ps:%d(%s)] %pV", + __builtin_return_address(0), line, name ? name : "", + ); + + va_end(args); +} diff --git a/include/linux/qed/qed_if.h b/include/linux/qed/qed_if.h index b1e3c57..fa3d8c0 100644 --- a/include/linux/qed/qed_if.h +++ b/include/linux/qed/qed_if.h @@ -539,44 +539,30 @@ struct qed_common_ops { #define GET_FIELD(value, name) \ (((value) >> (name ## _SHIFT)) & name ## _MASK) -/* Debug print definitions */ -#define DP_ERR(cdev, fmt, ...) \ - pr_err("[%s:%d(%s)]" fmt,\ - __func__, __LINE__, \ - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ - ## __VA_ARGS__) \ - -#define DP_NOTICE(cdev, fmt, ...)\ - do { \ - if (unlikely((cdev)->dp_level <= QED_LEVEL_NOTICE)) { \ - pr_notice("[%s:%d(%s)]" fmt, \ - __func__, __LINE__, \ - DP_NAME(cdev) ? DP_NAME(cdev) : "", \ - ## __VA_ARGS__);\ - \ - } \ - } while (0) - -#define DP_INFO(cdev, fmt, ...) \ - do {