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
Miscellanea:
o Neaten macros around the __mlog_printk uses.
o Use __func__ for __PRETTY_FUNCTION__
o Use ##__VA_ARGS__ for args...
Signed-off-by: Joe Perches <[email protected]>
---
Compiled/untested
fs/ocfs2/cluster/masklog.c | 17 +++++++++++++++++
fs/ocfs2/cluster/masklog.h | 28 +++++++++++++++++-----------
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/fs/ocfs2/cluster/masklog.c b/fs/ocfs2/cluster/masklog.c
index af7598b..5b7a351 100644
--- 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);
+}
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 \
+({ \
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) \
- __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)
@@ -205,10 +210,11 @@ extern struct mlog_bits mlog_and_bits, mlog_not_bits;
_st; \
})
-#define mlog_bug_on_msg(cond, fmt, args...) do { \
+#define mlog_bug_on_msg(cond, fmt, ...) \
+do { \
if (cond) { \
mlog(ML_ERROR, "bug expression: " #cond "\n"); \
- mlog(ML_ERROR, fmt, ##args); \
+ mlog(ML_ERROR, fmt, ##__VA_ARGS__); \
BUG(); \
} \
} while (0)
On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <[email protected]> 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 ;)
On Wed, 2015-04-22 at 15:46 -0700, Andrew Morton wrote:
> On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <[email protected]> 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...
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 <[email protected]>
---
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) ({ \
On Wed, Apr 22, 2015 at 03:46:04PM -0700, Andrew Morton wrote:
> On Fri, 17 Apr 2015 00:17:50 -0700 Joe Perches <[email protected]> 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
On Thu, 23 Apr 2015 16:04:18 -0700 Mark Fasheh <[email protected]> wrote:
> > 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.
Is there a simpleton's guide to testing ocfs2 on a local disk? One
which assumes a starting point of "knows how to type".
A few paragraphs in Documentation/filesystems/ocfs2.txt would be great
- then we can point non-ocfs2 people at it when they muck with stuff.
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(-)
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;
On Fri, Apr 24, 2015 at 1:19 AM, Andrew Morton
<[email protected]> wrote:
> On Thu, 23 Apr 2015 16:04:18 -0700 Mark Fasheh <[email protected]> wrote:
>
>> > 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.
>
> Is there a simpleton's guide to testing ocfs2 on a local disk? One
> which assumes a starting point of "knows how to type".
See http://docs.oracle.com/cd/E37670_01/E41138/html/ol_crlcl_ocfs2.html
--
Thanks,
//richard
On Fri, Apr 24, 2015 at 01:37:34AM +0200, Richard Weinberger wrote:
> On Fri, Apr 24, 2015 at 1:19 AM, Andrew Morton
> <[email protected]> wrote:
> > On Thu, 23 Apr 2015 16:04:18 -0700 Mark Fasheh <[email protected]> wrote:
> >
> >> > 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.
> >
> > Is there a simpleton's guide to testing ocfs2 on a local disk? One
> > which assumes a starting point of "knows how to type".
>
> See http://docs.oracle.com/cd/E37670_01/E41138/html/ol_crlcl_ocfs2.html
As Richard points out, it's pretty straightforward, just use the '-M local'
switch to mkfs.ocfs2. From there mount.ocfs2 will do the right thing when
you ask it to load the file system.
--Mark
--
Mark Fasheh
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
> [email protected]
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
--
Mark Fasheh
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