diff mbox

[next] ocfs2: Reduce object size of mlog uses

Message ID 1429832121.32612.43.camel@perches.com
State New, archived
Headers show

Commit Message

Joe Perches April 23, 2015, 11:35 p.m. UTC
On Thu, 2015-04-23 at 16:04 -0700, Mark Fasheh wrote:
> On Wed, Apr 22, 2015 at 03:46:04PM -0700, Andrew Morton wrote:
> > If you feel like undertaking such a rotorooting then go wild - that should
> > wake 'em up ;)
> 
> Ok, I've taken the bait  :)

"Here fishy, fishy...", erm, "Here Fasheh, Fasheh..."

With that out of the way:

A couple of possibilities:

o I wonder whether or not file/func/line matter at all.
  I think they don't.
  Removing them would reduce code size ~90K
o There's a small logging improvement possible in tcp.c.

Both below:

----------------------------------------------------------

 fs/ocfs2/cluster/masklog.c | 7 +++----
 fs/ocfs2/cluster/masklog.h | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

Comments

Mark Fasheh April 28, 2015, 6:30 p.m. UTC | #1
On Thu, Apr 23, 2015 at 04:35:21PM -0700, Joe Perches wrote:
> On Thu, 2015-04-23 at 16:04 -0700, Mark Fasheh wrote:
> > On Wed, Apr 22, 2015 at 03:46:04PM -0700, Andrew Morton wrote:
> > > If you feel like undertaking such a rotorooting then go wild - that should
> > > wake 'em up ;)
> > 
> > Ok, I've taken the bait  :)
> 
> "Here fishy, fishy...", erm, "Here Fasheh, Fasheh..."
> 
> With that out of the way:

:)


> 
> A couple of possibilities:
> 
> o I wonder whether or not file/func/line matter at all.
>   I think they don't.
>   Removing them would reduce code size ~90K

They do to those that are debugging live Ocfs2 modules. I would like at
least one pair kept please.


> o There's a small logging improvement possible in tcp.c.

This part looks good to me though, thanks for it.
	--Mark

> 
> Both below:
> 
> ----------------------------------------------------------
> 
>  fs/ocfs2/cluster/masklog.c | 7 +++----
>  fs/ocfs2/cluster/masklog.h | 8 +++-----
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
> index fc5e522..8b9816f 100644
> --- a/fs/ocfs2/cluster/masklog.c
> +++ b/fs/ocfs2/cluster/masklog.c
> @@ -77,8 +77,7 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf, size_t count)
>  	_cpu;								\
>  })
>  
> -void __mlog_printk(const u64 *mask, const char *func, int line,
> -		   const char *fmt, ...)
> +void __mlog_printk(const u64 *mask, const char *fmt, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
> @@ -103,9 +102,9 @@ void __mlog_printk(const u64 *mask, const char *func, int line,
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  
> -	printk("%s(%s,%u,%lu):%s:%d %s%pV",
> +	printk("%s(%s,%u,%lu) %s%pV",
>  	       level, current->comm, task_pid_nr(current), __mlog_cpu_guess,
> -	       func, line, prefix, &vaf);
> +	       prefix, &vaf);
>  
>  	va_end(args);
>  }
> diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
> index 308ea0e..9e93f19 100644
> --- a/fs/ocfs2/cluster/masklog.h
> +++ b/fs/ocfs2/cluster/masklog.h
> @@ -162,9 +162,8 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
>  
>  #endif
>  
> -__printf(4, 5)
> -void __mlog_printk(const u64 *m, const char *func, int line,
> -		   const char *fmt, ...);
> +__printf(2, 3)
> +void __mlog_printk(const u64 *m, const char *fmt, ...);
>  
>  /*
>   * Testing before the __mlog_printk call lets the compiler eliminate the
> @@ -174,8 +173,7 @@ void __mlog_printk(const u64 *m, const char *func, int line,
>  do {									\
>  	u64 _m = MLOG_MASK_PREFIX | (mask);				\
>  	if (_m & ML_ALLOWED_BITS)					\
> -		__mlog_printk(&_m, __func__, __LINE__, fmt,		\
> -			      ##__VA_ARGS__);				\
> +		__mlog_printk(&_m, fmt, ##__VA_ARGS__);			\
>  } while (0)
>  
>  #define mlog_errno(st) ({						\
> 
> ----------------------------------------------------------
> 
>  fs/ocfs2/cluster/tcp.c | 60 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
> index 56c403a..2c74973 100644
> --- a/fs/ocfs2/cluster/tcp.c
> +++ b/fs/ocfs2/cluster/tcp.c
> @@ -78,29 +78,43 @@
>  			  &sc->sc_node->nd_ipv4_address,		\
>  			  ntohs(sc->sc_node->nd_ipv4_port)
>  
> -/*
> - * In the following two log macros, the whitespace after the ',' just
> - * before ##args is intentional. Otherwise, gcc 2.95 will eat the
> - * previous token if args expands to nothing.
> - */
> -#define msglog(hdr, fmt, args...) do {					\
> -	typeof(hdr) __hdr = (hdr);					\
> -	mlog(ML_MSG, "[mag %u len %u typ %u stat %d sys_stat %d "	\
> -	     "key %08x num %u] " fmt,					\
> -	     be16_to_cpu(__hdr->magic), be16_to_cpu(__hdr->data_len), 	\
> -	     be16_to_cpu(__hdr->msg_type), be32_to_cpu(__hdr->status),	\
> -	     be32_to_cpu(__hdr->sys_status), be32_to_cpu(__hdr->key),	\
> -	     be32_to_cpu(__hdr->msg_num) ,  ##args);			\
> -} while (0)
> -
> -#define sclog(sc, fmt, args...) do {					\
> -	typeof(sc) __sc = (sc);						\
> -	mlog(ML_SOCKET, "[sc %p refs %d sock %p node %u page %p "	\
> -	     "pg_off %zu] " fmt, __sc,					\
> -	     atomic_read(&__sc->sc_kref.refcount), __sc->sc_sock,	\
> -	    __sc->sc_node->nd_num, __sc->sc_page, __sc->sc_page_off ,	\
> -	    ##args);							\
> -} while (0)
> +__printf(2, 3)
> +void msglog(struct o2net_msg *hdr, const char *fmt, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	mlog(ML_MSG, "[mag %u len %u typ %u stat %d sys_stat %d key %08x num %u] %pV",
> +	     be16_to_cpu(hdr->magic), be16_to_cpu(hdr->data_len),
> +	     be16_to_cpu(hdr->msg_type), be32_to_cpu(hdr->status),
> +	     be32_to_cpu(hdr->sys_status), be32_to_cpu(hdr->key),
> +	     be32_to_cpu(hdr->msg_num), &vaf);
> +
> +	va_end(args);
> +}
> +
> +__printf(2, 3)
> +void sclog(struct o2net_sock_container *sc, const char *fmt, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	mlog(ML_SOCKET, "[sc %p refs %d sock %p node %u page %p pg_off %zu] %pV",
> +	     sc, atomic_read(&sc->sc_kref.refcount), sc->sc_sock,
> +	     sc->sc_node->nd_num, sc->sc_page, sc->sc_page_off, &vaf);
> +
> +	va_end(args);
> +}
>  
>  static DEFINE_RWLOCK(o2net_handler_lock);
>  static struct rb_root o2net_handler_tree = RB_ROOT;
> 
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
--
Mark Fasheh
Joe Perches April 30, 2015, 5:05 a.m. UTC | #2
On Tue, 2015-04-28 at 11:30 -0700, Mark Fasheh wrote:
> On Thu, Apr 23, 2015 at 04:35:21PM -0700, Joe Perches wrote:
> > On Thu, 2015-04-23 at 16:04 -0700, Mark Fasheh wrote:
> > > On Wed, Apr 22, 2015 at 03:46:04PM -0700, Andrew Morton wrote:
> > > > If you feel like undertaking such a rotorooting then go wild - that should
> > > > wake 'em up ;)
> > > Ok, I've taken the bait  :)
> > "Here fishy, fishy...", erm, "Here Fasheh, Fasheh..."
> > With that out of the way:
> :)

Yeah.  Sorry 'bout that.

My own name is vaguely aquatic too so I thought I
could get away with it.  Bad Joe...

> > A couple of possibilities:
> > 
> > o I wonder whether or not file/func/line matter at all.
> >   I think they don't.
> >   Removing them would reduce code size ~90K
> 
> They do to those that are debugging live Ocfs2 modules. I would like at
> least one pair kept please.

Your code, your choice.

> > o There's a small logging improvement possible in tcp.c.
> This part looks good to me though, thanks for it.

Please submit something like it whenever you deem
appropriate.

cheers, Joe
diff mbox

Patch

diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index fc5e522..8b9816f 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -77,8 +77,7 @@  static ssize_t mlog_mask_store(u64 mask, const char *buf, size_t count)
 	_cpu;								\
 })
 
-void __mlog_printk(const u64 *mask, const char *func, int line,
-		   const char *fmt, ...)
+void __mlog_printk(const u64 *mask, const char *fmt, ...)
 {
 	struct va_format vaf;
 	va_list args;
@@ -103,9 +102,9 @@  void __mlog_printk(const u64 *mask, const char *func, int line,
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	printk("%s(%s,%u,%lu):%s:%d %s%pV",
+	printk("%s(%s,%u,%lu) %s%pV",
 	       level, current->comm, task_pid_nr(current), __mlog_cpu_guess,
-	       func, line, prefix, &vaf);
+	       prefix, &vaf);
 
 	va_end(args);
 }
diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
index 308ea0e..9e93f19 100644
--- a/fs/ocfs2/cluster/masklog.h
+++ b/fs/ocfs2/cluster/masklog.h
@@ -162,9 +162,8 @@  extern struct mlog_bits mlog_and_bits, mlog_not_bits;
 
 #endif
 
-__printf(4, 5)
-void __mlog_printk(const u64 *m, const char *func, int line,
-		   const char *fmt, ...);
+__printf(2, 3)
+void __mlog_printk(const u64 *m, const char *fmt, ...);
 
 /*
  * Testing before the __mlog_printk call lets the compiler eliminate the
@@ -174,8 +173,7 @@  void __mlog_printk(const u64 *m, const char *func, int line,
 do {									\
 	u64 _m = MLOG_MASK_PREFIX | (mask);				\
 	if (_m & ML_ALLOWED_BITS)					\
-		__mlog_printk(&_m, __func__, __LINE__, fmt,		\
-			      ##__VA_ARGS__);				\
+		__mlog_printk(&_m, fmt, ##__VA_ARGS__);			\
 } while (0)
 
 #define mlog_errno(st) ({						\

----------------------------------------------------------

 fs/ocfs2/cluster/tcp.c | 60 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index 56c403a..2c74973 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
@@ -78,29 +78,43 @@ 
 			  &sc->sc_node->nd_ipv4_address,		\
 			  ntohs(sc->sc_node->nd_ipv4_port)
 
-/*
- * In the following two log macros, the whitespace after the ',' just
- * before ##args is intentional. Otherwise, gcc 2.95 will eat the
- * previous token if args expands to nothing.
- */
-#define msglog(hdr, fmt, args...) do {					\
-	typeof(hdr) __hdr = (hdr);					\
-	mlog(ML_MSG, "[mag %u len %u typ %u stat %d sys_stat %d "	\
-	     "key %08x num %u] " fmt,					\
-	     be16_to_cpu(__hdr->magic), be16_to_cpu(__hdr->data_len), 	\
-	     be16_to_cpu(__hdr->msg_type), be32_to_cpu(__hdr->status),	\
-	     be32_to_cpu(__hdr->sys_status), be32_to_cpu(__hdr->key),	\
-	     be32_to_cpu(__hdr->msg_num) ,  ##args);			\
-} while (0)
-
-#define sclog(sc, fmt, args...) do {					\
-	typeof(sc) __sc = (sc);						\
-	mlog(ML_SOCKET, "[sc %p refs %d sock %p node %u page %p "	\
-	     "pg_off %zu] " fmt, __sc,					\
-	     atomic_read(&__sc->sc_kref.refcount), __sc->sc_sock,	\
-	    __sc->sc_node->nd_num, __sc->sc_page, __sc->sc_page_off ,	\
-	    ##args);							\
-} while (0)
+__printf(2, 3)
+void msglog(struct o2net_msg *hdr, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	mlog(ML_MSG, "[mag %u len %u typ %u stat %d sys_stat %d key %08x num %u] %pV",
+	     be16_to_cpu(hdr->magic), be16_to_cpu(hdr->data_len),
+	     be16_to_cpu(hdr->msg_type), be32_to_cpu(hdr->status),
+	     be32_to_cpu(hdr->sys_status), be32_to_cpu(hdr->key),
+	     be32_to_cpu(hdr->msg_num), &vaf);
+
+	va_end(args);
+}
+
+__printf(2, 3)
+void sclog(struct o2net_sock_container *sc, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	mlog(ML_SOCKET, "[sc %p refs %d sock %p node %u page %p pg_off %zu] %pV",
+	     sc, atomic_read(&sc->sc_kref.refcount), sc->sc_sock,
+	     sc->sc_node->nd_num, sc->sc_page, sc->sc_page_off, &vaf);
+
+	va_end(args);
+}
 
 static DEFINE_RWLOCK(o2net_handler_lock);
 static struct rb_root o2net_handler_tree = RB_ROOT;