Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450929 > unrolled thread

[PATCH] qed: Add and use specific logging functions to reduce object size

Back to article view | Back to linux.kernel

Contents

  [PATCH] qed: Add and use specific logging functions to reduce object size Joe Perches <joe@perches.com> - 2016-07-26 23:30 +0200
    Re: [PATCH] qed: Add and use specific logging functions to reduce  object size Joe Perches <joe@perches.com> - 2016-07-27 03:30 +0200
      RE: [PATCH] qed: Add and use specific logging functions to reduce  object size Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-07-27 11:40 +0200
    RE: [PATCH] qed: Add and use specific logging functions to reduce  object size Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-07-27 11:00 +0200
      Re: [PATCH] qed: Add and use specific logging functions to reduce  object size Joe Perches <joe@perches.com> - 2016-07-31 20:10 +0200
    Re: [PATCH] qed: Add and use specific logging functions to reduce  object size kbuild test robot <lkp@intel.com> - 2016-07-31 02:20 +0200

#1450929 — [PATCH] qed: Add and use specific logging functions to reduce object size

Current DP_<LEVEL> 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)
   text	   data	    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 <joe@perches.com>
---
 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 0000000..2795e63
--- /dev/null
+++ b/drivers/net/ethernet/qlogic/qed/qed_util.c
@@ -0,0 +1,82 @@
+#include <linux/kernel.h>
+#include <linux/qed/qed_if.h>
+
+#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 = &args;
+
+	pr_err("[%ps:%d(%s)] %pV",
+	       __builtin_return_address(0), line, name ? name : "",
+               &vaf);
+
+	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 = &args;
+
+	pr_notice("[%ps:%d(%s)] %pV",
+		  __builtin_return_address(0), line, name ? name : "",
+		  &vaf);
+
+	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 = &args;
+
+	pr_notice("[%ps:%d(%s)] %pV",
+		  __builtin_return_address(0), line, name ? name : "",
+		  &vaf);
+
+	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 = &args;
+
+	pr_notice("[%ps:%d(%s)] %pV",
+		  __builtin_return_address(0), line, name ? name : "",
+		  &vaf);
+
+	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 {							      \
-		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)
+__printf(3, 4)
+void qed_err(const char *name, int line, const char *fmt, ...);
+__printf(4, 5)
+void qed_notice(u32 level, const char *name, int line, const char *fmt, ...);
+__printf(4, 5)
+void qed_info(u32 level, const char *name, int line, const char *fmt, ...);
+__printf(4, 5)
+void qed_verbose(u32 level, const char *name, int line, const char *fmt, ...);
 
-#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)
+/* Debug print definitions */
+#define DP_ERR(type, fmt, ...)						\
+	qed_err(DP_NAME(type), __LINE__, fmt, ##__VA_ARGS__)
+#define DP_NOTICE(type, fmt, ...)					\
+	qed_notice((type)->dp_level, DP_NAME(type), __LINE__,		\
+		   fmt, ##__VA_ARGS__)
+#define DP_INFO(type, fmt, ...)						\
+	qed_info((type)->dp_level, DP_NAME(type), __LINE__,		\
+		 fmt, ##__VA_ARGS__)
+#define DP_VERBOSE(type, module, fmt, ...)				\
+do {									\
+	if ((type)->dp_module & module)					\
+		qed_verbose((type)->dp_level, DP_NAME(type), __LINE__,	\
+			    fmt, ##__VA_ARGS__);			\
+} while (0)
 
 enum DP_LEVEL {
 	QED_LEVEL_VERBOSE	= 0x0,
-- 
2.8.0.rc4.16.g56331f8

[toc] | [next] | [standalone]

#1450987 — 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_<LEVEL> 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)

[toc] | [prev] | [next] | [standalone]

#1451173 — 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_<LEVEL> 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.

[toc] | [prev] | [next] | [standalone]

#1451129 — RE: [PATCH] qed: Add and use specific logging functions to reduce object size

> Current DP_<LEVEL> 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.

[toc] | [prev] | [next] | [standalone]

#1452861 — Re: [PATCH] qed: Add and use specific logging functions to reduce object size

On Wed, 2016-07-27 at 07:24 +0000, 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

[toc] | [prev] | [next] | [standalone]

#1452761 — Re: [PATCH] qed: Add and use specific logging functions to reduce object size

[Multipart message — attachments visible in raw view] — view raw

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 infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web