thr3ads.net - Ocfs2 devel - [Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses [Apr 2015]

If this information is useful, please help other people find it:
Share via:

Andrew Morton

2015-Apr-22 22:46 UTC

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses

On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <joe at perches.com> wrote:
> Using a function for __mlog_printk instead of a macro
> reduces the object size of built-in.o more than 120KB, or
> ~10% overall (x86-64 defconfig with all ocfs2 options)
> 
> $ size fs/ocfs2/built-in.o*
>    text	   data	    bss	    dec	    hex	filename
>  936255	 118071	 134408	1188734	 12237e	fs/ocfs2/built-in.o.new
> 1064081	 118071	 134408	1316560	 1416d0	fs/ocfs2/built-in.o.old
It's a start.
> --- a/fs/ocfs2/cluster/masklog.c
> +++ b/fs/ocfs2/cluster/masklog.c
> @@ -64,6 +64,23 @@ static ssize_t mlog_mask_store(u64 mask, const char
*buf, size_t count)
>  	return count;
>  }
>  
> +void __mlog_printk(const char *level, const char *func, int line,
> +		   const char *fmt, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, fmt);
> +
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	printk("%s(%s,%u,%lu):%s:%d %pV",
> +	       level, current->comm, task_pid_nr(current), __mlog_cpu_guess,
> +	       func, line, &vaf);
> +
> +	va_end(args);
> +}
Logging function-name and line-number was a bit weird.  I wonder if
anyone will mind if this is converted to file-n-line, as God intended. 
That will shrink rodata a bit, because number-of-files is a lot less
than number-of-functions.
>  struct mlog_attribute {
>  	struct attribute attr;
>  	u64 mask;
> diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
> index 7fdc25a..6036e6a 100644
> --- a/fs/ocfs2/cluster/masklog.h
> +++ b/fs/ocfs2/cluster/masklog.h
> @@ -168,7 +168,8 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
>   * scream.  just do this instead of trying to guess which we're
building
>   * against.. *sigh*.
>   */
> -#define __mlog_cpu_guess ({		\
> +#define __mlog_cpu_guess		\
> +({					\
While we're in there we should turn this into __mlog_cpu_guess().

Or, preferably, just zap the sorry thing and use
raw_smp_processor_id().
>  	unsigned long _cpu = get_cpu();	\
>  	put_cpu();			\
>  	_cpu;				\
> @@ -178,21 +179,25 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
>   * before ##args is intentional. Otherwise, gcc 2.95 will eat the
>   * previous token if args expands to nothing.
>   */
> -#define __mlog_printk(level, fmt, args...)				\
> -	printk(level "(%s,%u,%lu):%s:%d " fmt, current->comm,		\
> -	       task_pid_nr(current), __mlog_cpu_guess,			\
> -	       __PRETTY_FUNCTION__, __LINE__ , ##args)
> +__printf(4, 5)
> +void __mlog_printk(const char *level, const char *func, int line,
> +		   const char *fmt, ...);
>  
> -#define mlog(mask, fmt, args...) do {					\
> +#define mlog(mask, fmt, ...)						\
> +do {									\
>  	u64 __m = MLOG_MASK_PREFIX | (mask);				\
>  	if ((__m & ML_ALLOWED_BITS) &&					\
>  	    __mlog_test_u64(__m, mlog_and_bits) &&			\
>  	    !__mlog_test_u64(__m, mlog_not_bits)) {			\
>  		if (__m & ML_ERROR)					\
All this goop can also be uninlined?
> -			__mlog_printk(KERN_ERR, "ERROR: "fmt , ##args);	\
> +			__mlog_printk(KERN_ERR, __func__, __LINE__,	\
> +				      "ERROR: " fmt, ##__VA_ARGS__);	\
>  		else if (__m & ML_NOTICE)				\
> -			__mlog_printk(KERN_NOTICE, fmt , ##args);	\
> -		else __mlog_printk(KERN_INFO, fmt , ##args);		\
> +			__mlog_printk(KERN_NOTICE, __func__, __LINE__,	\
> +				      fmt, ##__VA_ARGS__);		\
> +		else							\
> +			__mlog_printk(KERN_INFO, __func__, __LINE__,	\
> +				      fmt, ##__VA_ARGS__);		\
>  	}								\
>  } while (0)
>  
I guess this patch is a step on the way - a 10% shrink is decent.  But
I believe that with full uninlining of the ocfs2 logging code we can
shrink the filesystem's footprint by 50%.

This code needs some pretty serious rework and rethink, perhaps
involving a change to the emitted info.  I was hoping one of the ocfs2
developers would take the bait, but they're all in hiding.

If you feel like undertaking such a rotorooting then go wild - that should
wake 'em up ;)

Joe Perches

2015-Apr-23 02:34 UTC

head link

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses

On Wed, 2015-04-22 at 15:46 -0700, Andrew Morton wrote:> On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <joe at perches.com>
wrote:
> 
> > Using a function for __mlog_printk instead of a macro
> > reduces the object size of built-in.o more than 120KB, or
> > ~10% overall (x86-64 defconfig with all ocfs2 options)
> > 
> > $ size fs/ocfs2/built-in.o*
> >    text	   data	    bss	    dec	    hex	filename
> >  936255	 118071	 134408	1188734	 12237e	fs/ocfs2/built-in.o.new
> > 1064081	 118071	 134408	1316560	 1416d0	fs/ocfs2/built-in.o.old
> 
> It's a start.
> 
> > --- a/fs/ocfs2/cluster/masklog.c
> > +++ b/fs/ocfs2/cluster/masklog.c
> > @@ -64,6 +64,23 @@ static ssize_t mlog_mask_store(u64 mask, const char
*buf, size_t count)
> >  	return count;
> >  }
> >  
> > +void __mlog_printk(const char *level, const char *func, int line,
> > +		   const char *fmt, ...)
> > +{
> > +	struct va_format vaf;
> > +	va_list args;
> > +
> > +	va_start(args, fmt);
> > +
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +
> > +	printk("%s(%s,%u,%lu):%s:%d %pV",
> > +	       level, current->comm, task_pid_nr(current),
__mlog_cpu_guess,
> > +	       func, line, &vaf);
> > +
> > +	va_end(args);
> > +}
> 
> Logging function-name and line-number was a bit weird.  I wonder if
> anyone will mind if this is converted to file-n-line, as God intended. 
> That will shrink rodata a bit, because number-of-files is a lot less
> than number-of-functions.
I don't care one way or another.

Using __FILE__ vs __func__ reduces built-in.o
by about 25K.

I didn't bother to determine the actual total
reduction in a vmlinux.o
> > -#define mlog(mask, fmt, args...) do {					\
> > +#define mlog(mask, fmt, ...)						\
> > +do {									\
> >  	u64 __m = MLOG_MASK_PREFIX | (mask);				\
> >  	if ((__m & ML_ALLOWED_BITS) &&					\
> >  	    __mlog_test_u64(__m, mlog_and_bits) &&			\
> >  	    !__mlog_test_u64(__m, mlog_not_bits)) {			\
> >  		if (__m & ML_ERROR)					\
> All this goop can also be uninlined?
You have to convert the level pointer to a u64 pointer
passing &__m, but yeah, it's becomes about 65K smaller.

The macro becomes simpler too as the (__m & ML_<LEVEL>)
tests go into the function.

So that's another 7 or 8 % or more total shrinking.
> I guess this patch is a step on the way - a 10% shrink is decent.  But
> I believe that with full uninlining of the ocfs2 logging code we can
> shrink the filesystem's footprint by 50%.
Nope.

Even if CONFIG_PRINTK is not set and the mlog #define
is a no-op, it's not quite that big a reduction.

You have to turn the function tracing code off too for
that 50%.
> If you feel like undertaking such a rotorooting then go wild - that should
> wake 'em up ;)
One step at a time...

Joe Perches

2015-Apr-23 07:25 UTC

head link

[Ocfs2-devel] [PATCH V2 -next] ocfs2: Reduce object size of mlog uses

Using a function for __mlog_printk instead of a macro
reduces the object size of built-in.o by about 190KB, or
~18% overall (x86-64 defconfig with all ocfs2 options)

$ size fs/ocfs2/built-in.o*
   text    data     bss     dec     hex filename
 870954	 118471	 134408	1123833	 1125f9	fs/ocfs2/built-in.o,new
1064081  118071  134408 1316560  1416d0 fs/ocfs2/built-in.o.old

Miscellanea:

o Move the used-once __mlog_cpu_guess statement expression macro
  to the masklog.c file above the use in __mlog_printk function
o Simplify the mlog macro moving the and/or logic and level code
  into __mlog_printk

Signed-off-by: Joe Perches <joe at perches.com>
---

V2: Moving and/or logic to the __mlog_printk function reduces
    the object size an additional ~65K

 fs/ocfs2/cluster/masklog.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/cluster/masklog.h | 42 ++++++++++++------------------------------
 2 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index af7598b..fc5e522 100644
--- a/fs/ocfs2/cluster/masklog.c
+++ b/fs/ocfs2/cluster/masklog.c
@@ -64,6 +64,52 @@ static ssize_t mlog_mask_store(u64 mask, const char *buf,
size_t count)
 	return count;
 }
 
+/*
+ * smp_processor_id() "helpfully" screams when called outside
preemptible
+ * regions in current kernels.  sles doesn't have the variants that
don't
+ * scream.  just do this instead of trying to guess which we're building
+ * against.. *sigh*.
+ */
+#define __mlog_cpu_guess						\
+({									\
+	unsigned long _cpu = get_cpu();					\
+	put_cpu();							\
+	_cpu;								\
+})
+
+void __mlog_printk(const u64 *mask, const char *func, int line,
+		   const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	const char *level;
+	const char *prefix = "";
+
+	if (!__mlog_test_u64(*mask, mlog_and_bits) ||
+	    __mlog_test_u64(*mask, mlog_not_bits))
+		return;
+
+	if (*mask & ML_ERROR) {
+		level = KERN_ERR;
+		prefix = "ERROR: ";
+	} else if (*mask & ML_NOTICE) {
+		level = KERN_NOTICE;
+	} else {
+		level = KERN_INFO;
+	}
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	printk("%s(%s,%u,%lu):%s:%d %s%pV",
+	       level, current->comm, task_pid_nr(current), __mlog_cpu_guess,
+	       func, line, prefix, &vaf);
+
+	va_end(args);
+}
+
 struct mlog_attribute {
 	struct attribute attr;
 	u64 mask;
diff --git a/fs/ocfs2/cluster/masklog.h b/fs/ocfs2/cluster/masklog.h
index 7fdc25a..308ea0e 100644
--- a/fs/ocfs2/cluster/masklog.h
+++ b/fs/ocfs2/cluster/masklog.h
@@ -162,38 +162,20 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
 
 #endif
 
-/*
- * smp_processor_id() "helpfully" screams when called outside
preemptible
- * regions in current kernels.  sles doesn't have the variants that
don't
- * scream.  just do this instead of trying to guess which we're building
- * against.. *sigh*.
- */
-#define __mlog_cpu_guess ({		\
-	unsigned long _cpu = get_cpu();	\
-	put_cpu();			\
-	_cpu;				\
-})
+__printf(4, 5)
+void __mlog_printk(const u64 *m, const char *func, int line,
+		   const char *fmt, ...);
 
-/* In the following two macros, the whitespace after the ',' just
- * before ##args is intentional. Otherwise, gcc 2.95 will eat the
- * previous token if args expands to nothing.
+/*
+ * Testing before the __mlog_printk call lets the compiler eliminate the
+ * call completely when (m & ML_ALLOWED_BITS) is 0.
  */
-#define __mlog_printk(level, fmt, args...)				\
-	printk(level "(%s,%u,%lu):%s:%d " fmt, current->comm,		\
-	       task_pid_nr(current), __mlog_cpu_guess,			\
-	       __PRETTY_FUNCTION__, __LINE__ , ##args)
-
-#define mlog(mask, fmt, args...) do {					\
-	u64 __m = MLOG_MASK_PREFIX | (mask);				\
-	if ((__m & ML_ALLOWED_BITS) &&					\
-	    __mlog_test_u64(__m, mlog_and_bits) &&			\
-	    !__mlog_test_u64(__m, mlog_not_bits)) {			\
-		if (__m & ML_ERROR)					\
-			__mlog_printk(KERN_ERR, "ERROR: "fmt , ##args);	\
-		else if (__m & ML_NOTICE)				\
-			__mlog_printk(KERN_NOTICE, fmt , ##args);	\
-		else __mlog_printk(KERN_INFO, fmt , ##args);		\
-	}								\
+#define mlog(mask, fmt, ...)						\
+do {									\
+	u64 _m = MLOG_MASK_PREFIX | (mask);				\
+	if (_m & ML_ALLOWED_BITS)					\
+		__mlog_printk(&_m, __func__, __LINE__, fmt,		\
+			      ##__VA_ARGS__);				\
 } while (0)
 
 #define mlog_errno(st) ({						\

Mark Fasheh

2015-Apr-23 23:04 UTC

head link

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses

On Wed, Apr 22, 2015 at 03:46:04PM -0700, Andrew Morton
wrote:> On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <joe at perches.com>
wrote:
> 
> > Using a function for __mlog_printk instead of a macro
> > reduces the object size of built-in.o more than 120KB, or
> > ~10% overall (x86-64 defconfig with all ocfs2 options)
> > 
> > $ size fs/ocfs2/built-in.o*
> >    text	   data	    bss	    dec	    hex	filename
> >  936255	 118071	 134408	1188734	 12237e	fs/ocfs2/built-in.o.new
> > 1064081	 118071	 134408	1316560	 1416d0	fs/ocfs2/built-in.o.old
> 
> It's a start.
> 
> > --- a/fs/ocfs2/cluster/masklog.c
> > +++ b/fs/ocfs2/cluster/masklog.c
> > @@ -64,6 +64,23 @@ static ssize_t mlog_mask_store(u64 mask, const char
*buf, size_t count)
> >  	return count;
> >  }
> >  
> > +void __mlog_printk(const char *level, const char *func, int line,
> > +		   const char *fmt, ...)
> > +{
> > +	struct va_format vaf;
> > +	va_list args;
> > +
> > +	va_start(args, fmt);
> > +
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +
> > +	printk("%s(%s,%u,%lu):%s:%d %pV",
> > +	       level, current->comm, task_pid_nr(current),
__mlog_cpu_guess,
> > +	       func, line, &vaf);
> > +
> > +	va_end(args);
> > +}
> 
> Logging function-name and line-number was a bit weird.  I wonder if
> anyone will mind if this is converted to file-n-line, as God intended. 
> That will shrink rodata a bit, because number-of-files is a lot less
> than number-of-functions.
We can live with file-n-line.

> > -			__mlog_printk(KERN_ERR, "ERROR: "fmt , ##args);	\
> > +			__mlog_printk(KERN_ERR, __func__, __LINE__,	\
> > +				      "ERROR: " fmt, ##__VA_ARGS__);	\
> >  		else if (__m & ML_NOTICE)				\
> > -			__mlog_printk(KERN_NOTICE, fmt , ##args);	\
> > -		else __mlog_printk(KERN_INFO, fmt , ##args);		\
> > +			__mlog_printk(KERN_NOTICE, __func__, __LINE__,	\
> > +				      fmt, ##__VA_ARGS__);		\
> > +		else							\
> > +			__mlog_printk(KERN_INFO, __func__, __LINE__,	\
> > +				      fmt, ##__VA_ARGS__);		\
> >  	}								\
> >  } while (0)
> >  
> 
> I guess this patch is a step on the way - a 10% shrink is decent.  But
> I believe that with full uninlining of the ocfs2 logging code we can
> shrink the filesystem's footprint by 50%.
> 
> This code needs some pretty serious rework and rethink, perhaps
> involving a change to the emitted info.  I was hoping one of the ocfs2
> developers would take the bait, but they're all in hiding.
If it functions the same and doesn't have a major performance change,
I'm
pretty sure it'll be fine. We sometimes ask customers to enable some of the
debugging if they are having an issue. I would ask that it be tested
on a live system - a local fs, no cluster or cluster config required.

> If you feel like undertaking such a rotorooting then go wild - that should
> wake 'em up ;)
Ok, I've taken the bait  :)
	--Mark

--
Mark Fasheh

Ocfs2 devel - Apr 2015 - [PATCH next] ocfs2: Reduce object size of mlog uses

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses

[Ocfs2-devel] [PATCH V2 -next] ocfs2: Reduce object size of mlog uses

[Ocfs2-devel] [PATCH next] ocfs2: Reduce object size of mlog uses