Re: [PATCH 1/3] drivers-core: make structured logging play nicewith dynamic-debug
From: Joe Perches
Date: Mon Jul 23 2012 - 13:09:43 EST
On Mon, 2012-07-23 at 09:07 -0400, Jason Baron wrote:
> On Thu, Jul 19, 2012 at 01:46:20PM -0600, Jim Cromie wrote:
> > commit c4e00daaa96d3a0786f1f4fe6456281c60ef9a16 changed __dev_printk
> > in a way that broke dynamic-debug's ability to control the dynamic
> > prefix of dev_dbg(dev,..), but not dev_dbg(NULL,..) or pr_debug(..),
> > which is why it wasnt noticed sooner.
> >
> > When dev==NULL, __dev_printk() just calls printk(), which just works.
> > But otherwise, it assumed that level was always a string like "<L>"
> > and just plucked out the 'L', ignoring the rest. However,
> > dynamic_emit_prefix() adds "[tid] module:func:line:" to the string,
> > those additions all got lost.
> >
I think this is not a really good approach.
3 depths of %pV can consume quite a lot of stack
and avoiding this is useful.
I'd much rather improve/centralize the mechanism
in dynamic_debug.c so that the extra recursion
depth is avoided.
Something like this:
o Remove KERN_DEBUG from dynamic_emit_prefix
o Make a function of create_syslog_header to format
the header for printk_emit.
o Call printk_emit in dynamic_dev_dbg and dynamic_netdev_dbg
o Call printk_emit in __dev_printk
o Remove now unused EXPORT_SYMBOL(__netdev_printk)
o Neatening
Thoughts?
---
drivers/base/core.c | 55 +++++++++++++++++++++++++----------------
include/linux/device.h | 6 +++-
lib/dynamic_debug.c | 63 ++++++++++++++++++++++++++++++++++++++---------
net/core/dev.c | 5 ++-
4 files changed, 91 insertions(+), 38 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 346be8b..94f3803 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1844,25 +1844,19 @@ void device_shutdown(void)
*/
#ifdef CONFIG_PRINTK
-int __dev_printk(const char *level, const struct device *dev,
- struct va_format *vaf)
+int create_syslog_header(const struct device *dev, char *hdr, size_t hdrlen)
{
- char dict[128];
- size_t dictlen = 0;
const char *subsys;
-
- if (!dev)
- return printk("%s(NULL device *): %pV", level, vaf);
+ size_t pos = 0;
if (dev->class)
subsys = dev->class->name;
else if (dev->bus)
subsys = dev->bus->name;
else
- goto skip;
+ return 0;
- dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen,
- "SUBSYSTEM=%s", subsys);
+ pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys);
/*
* Add device identifier DEVICE=:
@@ -1878,24 +1872,39 @@ int __dev_printk(const char *level, const struct device *dev,
c = 'b';
else
c = 'c';
- dictlen++;
- dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen,
- "DEVICE=%c%u:%u",
- c, MAJOR(dev->devt), MINOR(dev->devt));
+ pos++;
+ pos += snprintf(hdr + pos, hdrlen - pos,
+ "DEVICE=%c%u:%u",
+ c, MAJOR(dev->devt), MINOR(dev->devt));
} else if (strcmp(subsys, "net") == 0) {
struct net_device *net = to_net_dev(dev);
- dictlen++;
- dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen,
- "DEVICE=n%u", net->ifindex);
+ pos++;
+ pos += snprintf(hdr + pos, hdrlen - pos,
+ "DEVICE=n%u", net->ifindex);
} else {
- dictlen++;
- dictlen += snprintf(dict + dictlen, sizeof(dict) - dictlen,
- "DEVICE=+%s:%s", subsys, dev_name(dev));
+ pos++;
+ pos += snprintf(hdr + pos, hdrlen - pos,
+ "DEVICE=+%s:%s", subsys, dev_name(dev));
}
-skip:
+
+ return pos;
+}
+EXPORT_SYMBOL(create_syslog_header);
+
+int __dev_printk(const char *level, const struct device *dev,
+ struct va_format *vaf)
+{
+ char hdr[128];
+ size_t hdrlen;
+
+ if (!dev)
+ return printk("%s(NULL device *): %pV", level, vaf);
+
+ hdrlen = create_syslog_header(dev, hdr, sizeof(hdr));
+
return printk_emit(0, level[1] - '0',
- dictlen ? dict : NULL, dictlen,
+ hdrlen ? hdr : NULL, hdrlen,
"%s %s: %pV",
dev_driver_string(dev), dev_name(dev), vaf);
}
@@ -1914,6 +1923,7 @@ int dev_printk(const char *level, const struct device *dev,
vaf.va = &args;
r = __dev_printk(level, dev, &vaf);
+
va_end(args);
return r;
@@ -1933,6 +1943,7 @@ int func(const struct device *dev, const char *fmt, ...) \
vaf.va = &args; \
\
r = __dev_printk(kern_level, dev, &vaf); \
+ \
va_end(args); \
\
return r; \
diff --git a/include/linux/device.h b/include/linux/device.h
index 6de9415..74863f0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -884,12 +884,14 @@ extern const char *dev_driver_string(const struct device *dev);
#ifdef CONFIG_PRINTK
+extern int create_syslog_header(const struct device *dev,
+ char *hdr, size_t hdrlen);
+
extern int __dev_printk(const char *level, const struct device *dev,
struct va_format *vaf);
extern __printf(3, 4)
int dev_printk(const char *level, const struct device *dev,
- const char *fmt, ...)
- ;
+ const char *fmt, ...);
extern __printf(2, 3)
int dev_emerg(const struct device *dev, const char *fmt, ...);
extern __printf(2, 3)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7ca29a0..08442e7 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -521,25 +521,23 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
int pos_after_tid;
int pos = 0;
- pos += snprintf(buf + pos, remaining(pos), "%s", KERN_DEBUG);
if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
if (in_interrupt())
- pos += snprintf(buf + pos, remaining(pos), "%s ",
- "<intr>");
+ pos += snprintf(buf + pos, remaining(pos), "<intr> ");
else
pos += snprintf(buf + pos, remaining(pos), "[%d] ",
- task_pid_vnr(current));
+ task_pid_vnr(current));
}
pos_after_tid = pos;
if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->modname);
+ desc->modname);
if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->function);
+ desc->function);
if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
- desc->lineno);
+ desc->lineno);
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
@@ -559,9 +557,13 @@ int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
BUG_ON(!fmt);
va_start(args, fmt);
+
vaf.fmt = fmt;
vaf.va = &args;
- res = printk("%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
+
+ res = printk(KERN_DEBUG "%s%pV",
+ dynamic_emit_prefix(descriptor, buf), &vaf);
+
va_end(args);
return res;
@@ -580,9 +582,25 @@ int __dynamic_dev_dbg(struct _ddebug *descriptor,
BUG_ON(!fmt);
va_start(args, fmt);
+
vaf.fmt = fmt;
vaf.va = &args;
- res = __dev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf);
+
+ if (!dev) {
+ res = printk(KERN_DEBUG "(NULL device *): %pV", &vaf);
+ } else {
+ char dict[128];
+ size_t dictlen;
+
+ dictlen = create_syslog_header(dev, dict, sizeof(dict));
+
+ res = printk_emit(0, 7,
+ dictlen ? dict : NULL, dictlen,
+ "%s%s %s: %pV",
+ dynamic_emit_prefix(descriptor, buf),
+ dev_driver_string(dev), dev_name(dev), &vaf);
+ }
+
va_end(args);
return res;
@@ -592,20 +610,41 @@ EXPORT_SYMBOL(__dynamic_dev_dbg);
#ifdef CONFIG_NET
int __dynamic_netdev_dbg(struct _ddebug *descriptor,
- const struct net_device *dev, const char *fmt, ...)
+ const struct net_device *dev, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
int res;
- char buf[PREFIX_SIZE];
BUG_ON(!descriptor);
BUG_ON(!fmt);
va_start(args, fmt);
+
vaf.fmt = fmt;
vaf.va = &args;
- res = __netdev_printk(dynamic_emit_prefix(descriptor, buf), dev, &vaf);
+
+ if (dev && dev->dev.parent) {
+ char buf[PREFIX_SIZE];
+ char dict[128];
+ size_t dictlen;
+
+ dictlen = create_syslog_header(dev->dev.parent,
+ dict, sizeof(dict));
+
+ res = printk_emit(0, 7,
+ dictlen ? dict : NULL, dictlen,
+ "%s%s %s: %pV",
+ dynamic_emit_prefix(descriptor, buf),
+ dev_driver_string(dev->dev.parent),
+ netdev_name(dev), &vaf);
+
+ } else if (dev) {
+ res = printk(KERN_DEBUG "%s: %pV", netdev_name(dev), &vaf);
+ } else {
+ res = printk(KERN_DEBUG "(NULL net_device): %pV", &vaf);
+ }
+
va_end(args);
return res;
diff --git a/net/core/dev.c b/net/core/dev.c
index 1cb0d8a..1651065 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6324,7 +6324,7 @@ const char *netdev_drivername(const struct net_device *dev)
return empty;
}
-int __netdev_printk(const char *level, const struct net_device *dev,
+static int __netdev_printk(const char *level, const struct net_device *dev,
struct va_format *vaf)
{
int r;
@@ -6339,7 +6339,6 @@ int __netdev_printk(const char *level, const struct net_device *dev,
return r;
}
-EXPORT_SYMBOL(__netdev_printk);
int netdev_printk(const char *level, const struct net_device *dev,
const char *format, ...)
@@ -6354,6 +6353,7 @@ int netdev_printk(const char *level, const struct net_device *dev,
vaf.va = &args;
r = __netdev_printk(level, dev, &vaf);
+
va_end(args);
return r;
@@ -6373,6 +6373,7 @@ int func(const struct net_device *dev, const char *fmt, ...) \
vaf.va = &args; \
\
r = __netdev_printk(level, dev, &vaf); \
+ \
va_end(args); \
\
return r; \
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/