Similar to CONFIG_GENERIC_BUG_RELATIVE_POINTERS that replaces (8 byte)
const char* members by (4 byte) signed offsets from the bug_entry,
this implements the similar thing for struct _ddebug, the descriptors
underlying pr_debug() and friends in a CONFIG_DYNAMIC_DEBUG kernel.
Since struct _ddebug has four string members, we save 16 byte per
instance. The total savings seem to be comparable to what is saved by
an architecture selecting GENERIC_BUG_RELATIVE_POINTERS (see patch 8
for some numbers for a common distro config).
While refreshing these patches, which were orignally just targeted at
x86-64, it occured to me that despite the implementation relying on
inline asm, there's nothing x86 specific about it, and indeed it seems
to work out-of-the-box for ppc64 and arm64 as well, but those have
only been compile-tested.
The first 6 patches are rather pedestrian preparations. The fun stuff
is in patch 7, and the remaining three patches are just the minimal
boilerplate to hook up the config option and asm-generic header on the
three architectures.
Rasmus Villemoes (10):
linux/device.h: use unique identifier for each struct _ddebug
linux/net.h: use unique identifier for each struct _ddebug
linux/printk.h: use unique identifier for each struct _ddebug
dynamic_debug: introduce accessors for string members of struct
_ddebug
dynamic_debug: drop use of bitfields in struct _ddebug
dynamic_debug: introduce CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
dynamic_debug: add asm-generic implementation for
DYNAMIC_DEBUG_RELATIVE_POINTERS
x86-64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
arm64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
powerpc: select DYNAMIC_DEBUG_RELATIVE_POINTERS for PPC64
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/Kbuild | 1 +
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/Kbuild | 1 +
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 +
arch/x86/include/asm/Kbuild | 1 +
include/asm-generic/dynamic_debug.h | 116 ++++++++++++++++++++
include/linux/device.h | 4 +-
include/linux/dynamic_debug.h | 26 +++--
include/linux/jump_label.h | 2 +
include/linux/net.h | 4 +-
include/linux/printk.h | 4 +-
lib/Kconfig.debug | 3 +
lib/dynamic_debug.c | 111 ++++++++++++++-----
15 files changed, 237 insertions(+), 40 deletions(-)
create mode 100644 include/asm-generic/dynamic_debug.h
--
2.20.1
Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for dev_dbg_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.
No functional change.
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Jason Baron <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/device.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 4e6987e11f68..64b95a913fef 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1562,7 +1562,7 @@ do { \
dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
#if defined(CONFIG_DYNAMIC_DEBUG)
/* descriptor check is first to prevent flooding with "callbacks suppressed" */
-#define dev_dbg_ratelimited(dev, fmt, ...) \
+#define _dev_dbg_ratelimited(descriptor, dev, fmt, ...) \
do { \
static DEFINE_RATELIMIT_STATE(_rs, \
DEFAULT_RATELIMIT_INTERVAL, \
@@ -1573,6 +1573,8 @@ do { \
__dynamic_dev_dbg(&descriptor, dev, dev_fmt(fmt), \
##__VA_ARGS__); \
} while (0)
+#define dev_dbg_ratelimited(dev, fmt, ...) \
+ _dev_dbg_ratelimited(__UNIQUE_ID(ddebug), dev, fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define dev_dbg_ratelimited(dev, fmt, ...) \
do { \
--
2.20.1
Properly initializing a struct containing bitfields in assembly is
hard. Instead, merge lineno and flags to a single unsigned int, which we
mask manually. This should not cause any worse code than what gcc would
need to generate for accessing the bitfields anyway.
Actually, on 64 bit, there is a four byte hole after these fields, so we
could just give flags and lineno each their own u32. But I don't think
that's worth the ifdeffery.
Acked-by: Jason Baron <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/dynamic_debug.h | 12 ++++------
lib/dynamic_debug.c | 44 +++++++++++++++++++++++------------
2 files changed, 34 insertions(+), 22 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index c2be029b9b53..e2fa2737a485 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -20,7 +20,6 @@ struct _ddebug {
const char *function;
const char *filename;
const char *format;
- unsigned int lineno:18;
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -37,7 +36,7 @@ struct _ddebug {
#else
#define _DPRINTK_FLAGS_DEFAULT 0
#endif
- unsigned int flags:8;
+ unsigned int flags_lineno; /* flags in lower 8 bits, lineno in upper 24 */
#ifdef CONFIG_JUMP_LABEL
union {
struct static_key_true dd_key_true;
@@ -46,7 +45,7 @@ struct _ddebug {
#endif
} __attribute__((aligned(8)));
-
+#define _DPRINTK_FLAGS_LINENO_INIT (((unsigned)__LINE__ << 8) | _DPRINTK_FLAGS_DEFAULT)
#if defined(CONFIG_DYNAMIC_DEBUG)
int ddebug_add_module(struct _ddebug *tab, unsigned int n,
@@ -78,8 +77,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
.function = __func__, \
.filename = __FILE__, \
.format = (fmt), \
- .lineno = __LINE__, \
- .flags = _DPRINTK_FLAGS_DEFAULT, \
+ .flags_lineno = _DPRINTK_FLAGS_LINENO_INIT, \
_DPRINTK_KEY_INIT \
}
@@ -104,10 +102,10 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
#ifdef DEBUG
#define DYNAMIC_DEBUG_BRANCH(descriptor) \
- likely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+ likely(descriptor.flags_lineno & _DPRINTK_FLAGS_PRINT)
#else
#define DYNAMIC_DEBUG_BRANCH(descriptor) \
- unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)
+ unlikely(descriptor.flags_lineno & _DPRINTK_FLAGS_PRINT)
#endif
#endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 497b1c8ccc2a..482a35a68752 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -53,6 +53,18 @@ static inline const char *dd_format(const struct _ddebug *dd)
{
return dd->format;
}
+static inline unsigned dd_lineno(const struct _ddebug *dd)
+{
+ return dd->flags_lineno >> 8;
+}
+static inline unsigned dd_flags(const struct _ddebug *dd)
+{
+ return dd->flags_lineno & 0xff;
+}
+static inline void dd_set_flags(struct _ddebug *dd, unsigned newflags)
+{
+ dd->flags_lineno = (dd_lineno(dd) << 8) | newflags;
+}
extern struct _ddebug __start___verbose[];
extern struct _ddebug __stop___verbose[];
@@ -111,7 +123,7 @@ static char *ddebug_describe_flags(struct _ddebug *dp, char *buf,
BUG_ON(maxlen < 6);
for (i = 0; i < ARRAY_SIZE(opt_array); ++i)
- if (dp->flags & opt_array[i].flag)
+ if (dd_flags(dp) & opt_array[i].flag)
*p++ = opt_array[i].opt_char;
if (p == buf)
*p++ = '_';
@@ -157,7 +169,7 @@ static int ddebug_change(const struct ddebug_query *query,
{
int i;
struct ddebug_table *dt;
- unsigned int newflags;
+ unsigned int newflags, oldflags;
unsigned int nfound = 0;
char flagbuf[10];
@@ -194,27 +206,28 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the line number range */
if (query->first_lineno &&
- dp->lineno < query->first_lineno)
+ dd_lineno(dp) < query->first_lineno)
continue;
if (query->last_lineno &&
- dp->lineno > query->last_lineno)
+ dd_lineno(dp) > query->last_lineno)
continue;
nfound++;
- newflags = (dp->flags & mask) | flags;
- if (newflags == dp->flags)
+ oldflags = dd_flags(dp);
+ newflags = (oldflags & mask) | flags;
+ if (newflags == oldflags)
continue;
#ifdef CONFIG_JUMP_LABEL
- if (dp->flags & _DPRINTK_FLAGS_PRINT) {
+ if (oldflags & _DPRINTK_FLAGS_PRINT) {
if (!(flags & _DPRINTK_FLAGS_PRINT))
static_branch_disable(&dp->key.dd_key_true);
} else if (flags & _DPRINTK_FLAGS_PRINT)
static_branch_enable(&dp->key.dd_key_true);
#endif
- dp->flags = newflags;
+ dd_set_flags(dp, newflags);
vpr_info("changed %s:%d [%s]%s =%s\n",
- trim_prefix(dd_filename(dp)), dp->lineno,
+ trim_prefix(dd_filename(dp)), dd_lineno(dp),
dt->mod_name, dd_function(dp),
ddebug_describe_flags(dp, flagbuf,
sizeof(flagbuf)));
@@ -537,10 +550,11 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
{
int pos_after_tid;
int pos = 0;
+ unsigned flags = dd_flags(desc);
*buf = '\0';
- if (desc->flags & _DPRINTK_FLAGS_INCL_TID) {
+ if (flags & _DPRINTK_FLAGS_INCL_TID) {
if (in_interrupt())
pos += snprintf(buf + pos, remaining(pos), "<intr> ");
else
@@ -548,15 +562,15 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
task_pid_vnr(current));
}
pos_after_tid = pos;
- if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
+ if (flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
dd_modname(desc));
- if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
+ if (flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
dd_function(desc));
- if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
+ if (flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
- desc->lineno);
+ dd_lineno(desc));
if (pos - pos_after_tid)
pos += snprintf(buf + pos, remaining(pos), " ");
if (pos >= PREFIX_SIZE)
@@ -807,7 +821,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
}
seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dd_filename(dp)), dp->lineno,
+ trim_prefix(dd_filename(dp)), dd_lineno(dp),
iter->table->mod_name, dd_function(dp),
ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
seq_escape(m, dd_format(dp), "\t\r\n\"");
--
2.20.1
Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for net_dbg_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.
No functional change.
Cc: [email protected]
Acked-by: Jason Baron <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/net.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/net.h b/include/linux/net.h
index c606c72311d0..828e0db1e63a 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -266,7 +266,7 @@ do { \
#define net_info_ratelimited(fmt, ...) \
net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
#if defined(CONFIG_DYNAMIC_DEBUG)
-#define net_dbg_ratelimited(fmt, ...) \
+#define _net_dbg_ratelimited(descriptor, fmt, ...) \
do { \
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
if (DYNAMIC_DEBUG_BRANCH(descriptor) && \
@@ -274,6 +274,8 @@ do { \
__dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
##__VA_ARGS__); \
} while (0)
+#define net_dbg_ratelimited(fmt, ...) \
+ _net_dbg_ratelimited(__UNIQUE_ID(ddebug), fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define net_dbg_ratelimited(fmt, ...) \
net_ratelimited_function(pr_debug, fmt, ##__VA_ARGS__)
--
2.20.1
Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for pr_debug_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.
No functional change.
Acked-by: Petr Mladek <[email protected]>
Acked-by: Jason Baron <[email protected]>
Cc: Steven Rostedt <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/printk.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index d7c77ed1a4cb..0fcc588db649 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -454,7 +454,7 @@ extern int kptr_restrict;
/* If you are writing a driver, please use dev_dbg instead */
#if defined(CONFIG_DYNAMIC_DEBUG)
/* descriptor check is first to prevent flooding with "callbacks suppressed" */
-#define pr_debug_ratelimited(fmt, ...) \
+#define _pr_debug_ratelimited(descriptor, fmt, ...) \
do { \
static DEFINE_RATELIMIT_STATE(_rs, \
DEFAULT_RATELIMIT_INTERVAL, \
@@ -464,6 +464,8 @@ do { \
__ratelimit(&_rs)) \
__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__); \
} while (0)
+#define pr_debug_ratelimited(fmt, ...) \
+ _pr_debug_ratelimited(__UNIQUE_ID(ddebug), fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define pr_debug_ratelimited(fmt, ...) \
printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
--
2.20.1
Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
four const char* members of struct _ddebug, thus saving 16 bytes per
instance (one for each pr_debug(), dev_debug() etc. in a
CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
work out-of-the-box, though this is only compile-tested.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/Kbuild | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2d0be82c3061..6821c8ae1d62 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -155,6 +155,7 @@ config PPC
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
+ select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
select DYNAMIC_FTRACE if FUNCTION_TRACER
select EDAC_ATOMIC_SCRUB
select EDAC_SUPPORT
diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
index a0c132bedfae..f332e202192a 100644
--- a/arch/powerpc/include/asm/Kbuild
+++ b/arch/powerpc/include/asm/Kbuild
@@ -3,6 +3,7 @@ generated-y += syscall_table_64.h
generated-y += syscall_table_c32.h
generated-y += syscall_table_spu.h
generic-y += div64.h
+generic-y += dynamic_debug.h
generic-y += export.h
generic-y += irq_regs.h
generic-y += local64.h
--
2.20.1
Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
four const char* members of struct _ddebug, thus saving 16 bytes per
instance (one for each pr_debug(), dev_debug() etc. in a
CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
work out-of-the-box, though this is only compile-tested.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/Kbuild | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9eba5de..d0871d523d5d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -83,6 +83,7 @@ config ARM64
select CRC32
select DCACHE_WORD_ACCESS
select DMA_DIRECT_REMAP
+ select DYNAMIC_DEBUG_RELATIVE_POINTERS
select EDAC_SUPPORT
select FRAME_POINTER
select GENERIC_ALLOCATOR
diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
index 1e17ea5c372b..1ead0d645686 100644
--- a/arch/arm64/include/asm/Kbuild
+++ b/arch/arm64/include/asm/Kbuild
@@ -3,6 +3,7 @@ generic-y += delay.h
generic-y += div64.h
generic-y += dma.h
generic-y += dma-contiguous.h
+generic-y += dynamic_debug.h
generic-y += early_ioremap.h
generic-y += emergency-restart.h
generic-y += hw_irq.h
--
2.20.1
When we introduce compact versions of these pointers (a la
CONFIG_GENERIC_BUG_RELATIVE_POINTERS), all access to these members must
go via appropriate accessors. This just mass-converts dynamic_debug.c to
use the new accessors.
Acked-by: Jason Baron <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
lib/dynamic_debug.c | 51 ++++++++++++++++++++++++++++++---------------
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7bdf98c37e91..497b1c8ccc2a 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -37,6 +37,23 @@
#include <linux/device.h>
#include <linux/netdevice.h>
+static inline const char *dd_modname(const struct _ddebug *dd)
+{
+ return dd->modname;
+}
+static inline const char *dd_function(const struct _ddebug *dd)
+{
+ return dd->function;
+}
+static inline const char *dd_filename(const struct _ddebug *dd)
+{
+ return dd->filename;
+}
+static inline const char *dd_format(const struct _ddebug *dd)
+{
+ return dd->format;
+}
+
extern struct _ddebug __start___verbose[];
extern struct _ddebug __stop___verbose[];
@@ -158,21 +175,21 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query->filename &&
- !match_wildcard(query->filename, dp->filename) &&
+ !match_wildcard(query->filename, dd_filename(dp)) &&
!match_wildcard(query->filename,
- kbasename(dp->filename)) &&
+ kbasename(dd_filename(dp))) &&
!match_wildcard(query->filename,
- trim_prefix(dp->filename)))
+ trim_prefix(dd_filename(dp))))
continue;
/* match against the function */
if (query->function &&
- !match_wildcard(query->function, dp->function))
+ !match_wildcard(query->function, dd_function(dp)))
continue;
/* match against the format */
if (query->format &&
- !strstr(dp->format, query->format))
+ !strstr(dd_format(dp), query->format))
continue;
/* match against the line number range */
@@ -197,8 +214,8 @@ static int ddebug_change(const struct ddebug_query *query,
#endif
dp->flags = newflags;
vpr_info("changed %s:%d [%s]%s =%s\n",
- trim_prefix(dp->filename), dp->lineno,
- dt->mod_name, dp->function,
+ trim_prefix(dd_filename(dp)), dp->lineno,
+ dt->mod_name, dd_function(dp),
ddebug_describe_flags(dp, flagbuf,
sizeof(flagbuf)));
}
@@ -533,10 +550,10 @@ static char *dynamic_emit_prefix(const struct _ddebug *desc, char *buf)
pos_after_tid = pos;
if (desc->flags & _DPRINTK_FLAGS_INCL_MODNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->modname);
+ dd_modname(desc));
if (desc->flags & _DPRINTK_FLAGS_INCL_FUNCNAME)
pos += snprintf(buf + pos, remaining(pos), "%s:",
- desc->function);
+ dd_function(desc));
if (desc->flags & _DPRINTK_FLAGS_INCL_LINENO)
pos += snprintf(buf + pos, remaining(pos), "%d:",
desc->lineno);
@@ -790,10 +807,10 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
}
seq_printf(m, "%s:%u [%s]%s =%s \"",
- trim_prefix(dp->filename), dp->lineno,
- iter->table->mod_name, dp->function,
+ trim_prefix(dd_filename(dp)), dp->lineno,
+ iter->table->mod_name, dd_function(dp),
ddebug_describe_flags(dp, flagsbuf, sizeof(flagsbuf)));
- seq_escape(m, dp->format, "\t\r\n\"");
+ seq_escape(m, dd_format(dp), "\t\r\n\"");
seq_puts(m, "\"\n");
return 0;
@@ -987,20 +1004,20 @@ static int __init dynamic_debug_init(void)
return 1;
}
iter = __start___verbose;
- modname = iter->modname;
+ modname = dd_modname(iter);
iter_start = iter;
for (; iter < __stop___verbose; iter++) {
entries++;
- verbose_bytes += strlen(iter->modname) + strlen(iter->function)
- + strlen(iter->filename) + strlen(iter->format);
+ verbose_bytes += strlen(dd_modname(iter)) + strlen(dd_function(iter))
+ + strlen(dd_filename(iter)) + strlen(dd_format(iter));
- if (strcmp(modname, iter->modname)) {
+ if (strcmp(modname, dd_modname(iter))) {
modct++;
ret = ddebug_add_module(iter_start, n, modname);
if (ret)
goto out_err;
n = 0;
- modname = iter->modname;
+ modname = dd_modname(iter);
iter_start = iter;
}
n++;
--
2.20.1
This reduces the size of struct _ddebug from 56 to 40 bytes. There's
one such struct for each pr_debug(), netdev_debug() etc. in a
CONFIG_DYNAMIC_DEBUG kernel. An Ubuntu 4.15 kernel has about 2550
entries in the __verbose section of vmlinux, amounting to ~40K
saved. (Modules also become smaller, but it's harder to quantify how
much that yields at runtime.)
For comparison, the __bug_table section of that Ubuntu kernel is 75576
bytes, i.e. 6298 12-byte bug_entrys, so GENERIC_BUG_RELATIVE_POINTERS
saves ~50K.
Due to the build-time sanity checks in asm-generic/dynamic_debug.h, we
need to add another #undef to vclock_gettime.c.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 +
arch/x86/include/asm/Kbuild | 1 +
3 files changed, 3 insertions(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5ad92419be19..eb5488b4577d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -30,6 +30,7 @@ config X86_64
select SWIOTLB
select X86_DEV_DMA_OPS
select ARCH_HAS_SYSCALL_WRAPPER
+ select DYNAMIC_DEBUG_RELATIVE_POINTERS
#
# Arch settings
diff --git a/arch/x86/entry/vdso/vdso32/vclock_gettime.c b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
index 9242b28418d5..9acec4426206 100644
--- a/arch/x86/entry/vdso/vdso32/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
@@ -17,6 +17,7 @@
#undef CONFIG_ILLEGAL_POINTER_VALUE
#undef CONFIG_SPARSEMEM_VMEMMAP
#undef CONFIG_NR_CPUS
+#undef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
#define CONFIG_X86_32 1
#define CONFIG_PGTABLE_LEVELS 2
diff --git a/arch/x86/include/asm/Kbuild b/arch/x86/include/asm/Kbuild
index a0ab9ab61c75..793d2c6735b9 100644
--- a/arch/x86/include/asm/Kbuild
+++ b/arch/x86/include/asm/Kbuild
@@ -7,6 +7,7 @@ generated-y += unistd_64_x32.h
generated-y += xen-hypercalls.h
generic-y += dma-contiguous.h
+generic-y += dynamic_debug.h
generic-y += early_ioremap.h
generic-y += export.h
generic-y += mcs_spinlock.h
--
2.20.1
A 64 bit architecture can reduce the size of the kernel image by
selecting CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS, but it must provide
a proper DEFINE_DYNAMIC_DEBUG_METADATA macro for emitting the struct
_ddebug in assembly. However, since that does not involve any
instructions, this generic implementation should be usable by most if
not all.
It relies on
(1) standard assembly directives that should work on
all architectures
(2) the "i" constraint for an constant, and
(3) %cN emitting the constant operand N without punctuation
and of course the layout of _ddebug being what one expects. I've
succesfully built x86-64, arm64 and ppc64 defconfig +
CONFIG_DYNAMIC_DEBUG + patches to hook up this header, and boot-tested
the x86-64 one.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/asm-generic/dynamic_debug.h | 116 ++++++++++++++++++++++++++++
include/linux/jump_label.h | 2 +
2 files changed, 118 insertions(+)
create mode 100644 include/asm-generic/dynamic_debug.h
diff --git a/include/asm-generic/dynamic_debug.h b/include/asm-generic/dynamic_debug.h
new file mode 100644
index 000000000000..f1dd3019cd98
--- /dev/null
+++ b/include/asm-generic/dynamic_debug.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_DYNAMIC_DEBUG_H
+#define _ASM_GENERIC_DYNAMIC_DEBUG_H
+
+#ifndef _DYNAMIC_DEBUG_H
+#error "don't include asm/dynamic_debug.h directly"
+#endif
+
+#include <linux/build_bug.h>
+#ifdef CONFIG_JUMP_LABEL
+#include <linux/jump_label.h>
+#endif
+
+/*
+ * We need to know the exact layout of struct _ddebug in order to
+ * initialize it in assembly. Check that all members are at expected
+ * offsets - if any of these fail, the arch cannot use this generic
+ * dynamic_debug.h. DYNAMIC_DEBUG_RELATIVE_POINTERS is pointless for
+ * !64BIT, so we expect the static_key to be at an 8-byte boundary
+ * since it contains stuff which is long-aligned.
+ */
+
+static_assert(BITS_PER_LONG == 64);
+static_assert(offsetof(struct _ddebug, modname_disp) == 0);
+static_assert(offsetof(struct _ddebug, function_disp) == 4);
+static_assert(offsetof(struct _ddebug, filename_disp) == 8);
+static_assert(offsetof(struct _ddebug, format_disp) == 12);
+static_assert(offsetof(struct _ddebug, flags_lineno) == 16);
+
+#ifdef CONFIG_JUMP_LABEL
+static_assert(offsetof(struct _ddebug, key) == 24);
+static_assert(offsetof(struct static_key, enabled) == 0);
+static_assert(offsetof(struct static_key, type) == 8);
+
+/* <4 bytes padding>, .enabled, <4 bytes padding>, .type */
+# ifdef DEBUG
+# define _DPRINTK_ASM_KEY_INIT \
+ "\t.int 0\n" "\t.int 1\n" "\t.int 0\n" "\t.quad "__stringify(__JUMP_TYPE_TRUE)"\n"
+# else
+# define _DPRINTK_ASM_KEY_INIT \
+ "\t.int 0\n" "\t.int 0\n" "\t.int 0\n" "\t.quad "__stringify(__JUMP_TYPE_FALSE)"\n"
+# endif
+#else /* !CONFIG_JUMP_LABEL */
+#define _DPRINTK_ASM_KEY_INIT ""
+#endif
+
+/*
+ * There's a bit of magic involved here.
+ *
+ * First, unlike the bug table entries, we need to define an object in
+ * assembly which we can reference from C code (for use by the
+ * DYNAMIC_DEBUG_BRANCH macro), but we don't want 'name' to have
+ * external linkage (that would require use of globally unique
+ * identifiers, which we can't guarantee). Fortunately, the "extern"
+ * keyword just tells the compiler that _somebody_ provides that
+ * symbol - usually that somebody is the linker, but in this case it's
+ * the assembler, and since we do not do .globl name, the symbol gets
+ * internal linkage.
+ *
+ * So far so good. The next problem is that there's no scope in
+ * assembly, so the identifier 'name' has to be unique within each
+ * translation unit - otherwise all uses of that identifier end up
+ * referring to the same struct _ddebug instance. pr_debug and friends
+ * do this by use of indirection and __UNIQUE_ID(), and new users of
+ * DEFINE_DYNAMIC_DEBUG_METADATA() should do something similar. We
+ * need to catch cases where this is not done at build time.
+ *
+ * With assembly-level .ifndef we can ensure that we only define a
+ * given identifier once, preventing "symbol 'foo' already defined"
+ * errors. But we still need to detect and fail on multiple uses of
+ * the same identifer. The simplest, and wrong, solution to that is to
+ * add an .else .error branch to the .ifndef. The problem is that just
+ * because the DEFINE_DYNAMIC_DEBUG_METADATA() macro is only expanded
+ * once with a given identifier, the compiler may emit the assembly
+ * code multiple times, e.g. if the macro appears in an inline
+ * function. Now, in a normal case like
+ *
+ * static inline get_next_id(void) { static int v; return ++v; }
+ *
+ * all inlined copies of get_next_id are _supposed_ to refer to the
+ * same object 'v'. So we do need to allow this chunk of assembly to
+ * appear multiple times with the same 'name', as long as they all
+ * came from the same DEFINE_DYNAMIC_DEBUG_METADATA() instance. To do
+ * that, we pass __COUNTER__ to the asm(), and set an assembler symbol
+ * name.ddebug.once to that value when we first define 'name'. When we
+ * meet a second attempt at defining 'name', we compare
+ * name.ddebug.once to %6 and error out if they are different.
+ */
+
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
+ extern struct _ddebug name; \
+ asm volatile(".ifndef " __stringify(name) "\n" \
+ ".pushsection __verbose,\"aw\"\n" \
+ ".type "__stringify(name)", STT_OBJECT\n" \
+ ".size "__stringify(name)", %c5\n" \
+ "1:\n" \
+ __stringify(name) ":\n" \
+ "\t.int %c0 - 1b /* _ddebug::modname_disp */\n" \
+ "\t.int %c1 - 1b /* _ddebug::function_disp */\n" \
+ "\t.int %c2 - 1b /* _ddebug::filename_disp */\n" \
+ "\t.int %c3 - 1b /* _ddebug::format_disp */\n" \
+ "\t.int %c4 /* _ddebug::flags_lineno */\n" \
+ _DPRINTK_ASM_KEY_INIT \
+ "\t.org 1b+%c5\n" \
+ ".popsection\n" \
+ ".set "__stringify(name)".ddebug.once, %c6\n" \
+ ".elseif "__stringify(name)".ddebug.once - %c6\n" \
+ ".line "__stringify(__LINE__) " - 1\n" \
+ ".error \"'"__stringify(name)"' used as _ddebug identifier more than once\"\n" \
+ ".endif\n" \
+ : : "i" (KBUILD_MODNAME), "i" (__func__), \
+ "i" (__FILE__), "i" (fmt), \
+ "i" (_DPRINTK_FLAGS_LINENO_INIT), \
+ "i" (sizeof(struct _ddebug)), "i" (__COUNTER__))
+
+#endif /* _ASM_GENERIC_DYNAMIC_DEBUG_H */
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 3e113a1fa0f1..2b027d2ef3d0 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -190,6 +190,8 @@ struct module;
#ifdef CONFIG_JUMP_LABEL
+#define __JUMP_TYPE_FALSE 0
+#define __JUMP_TYPE_TRUE 1
#define JUMP_TYPE_FALSE 0UL
#define JUMP_TYPE_TRUE 1UL
#define JUMP_TYPE_LINKED 2UL
--
2.20.1
Based on the same idea for struct bug_entry, an architecture can opt-in
to use relative pointers in struct _ddebug. It only makes sense for 64
bit architectures, where one saves 16 bytes per entry (out of 40 or 56,
depending on CONFIG_JUMP_LABEL). The architecture is responsible for
providing a suitable DEFINE_DYNAMIC_DEBUG_METADATA macro in
<asm/dynamic_debug.h>.
Acked-by: Jason Baron <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/dynamic_debug.h | 14 ++++++++++++++
lib/Kconfig.debug | 3 +++
lib/dynamic_debug.c | 20 ++++++++++++++++++++
3 files changed, 37 insertions(+)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index e2fa2737a485..08175c219d60 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -16,10 +16,17 @@ struct _ddebug {
* These fields are used to drive the user interface
* for selecting and displaying debug callsites.
*/
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+ signed int modname_disp;
+ signed int function_disp;
+ signed int filename_disp;
+ signed int format_disp;
+#else
const char *modname;
const char *function;
const char *filename;
const char *format;
+#endif
/*
* The flags field controls the behaviour at the callsite.
* The bits here are changed dynamically when the user
@@ -70,6 +77,12 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
const struct net_device *dev,
const char *fmt, ...);
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+#include <asm/dynamic_debug.h>
+#ifndef DEFINE_DYNAMIC_DEBUG_METADATA
+# error "asm/dynamic_debug.h must provide definition of DEFINE_DYNAMIC_DEBUG_METADATA"
+#endif
+#else
#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
static struct _ddebug __aligned(8) \
__attribute__((section("__verbose"))) name = { \
@@ -80,6 +93,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
.flags_lineno = _DPRINTK_FLAGS_LINENO_INIT, \
_DPRINTK_KEY_INIT \
}
+#endif
#ifdef CONFIG_JUMP_LABEL
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 0d9e81779e37..7cee0895f9c8 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -163,6 +163,9 @@ config DYNAMIC_DEBUG
See Documentation/admin-guide/dynamic-debug-howto.rst for additional
information.
+config DYNAMIC_DEBUG_RELATIVE_POINTERS
+ bool
+
endmenu # "printk and dmesg options"
menu "Compile-time checks and compiler options"
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 482a35a68752..58288560cc35 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -37,6 +37,24 @@
#include <linux/device.h>
#include <linux/netdevice.h>
+#ifdef CONFIG_DYNAMIC_DEBUG_RELATIVE_POINTERS
+static inline const char *dd_modname(const struct _ddebug *dd)
+{
+ return (const char *)dd + dd->modname_disp;
+}
+static inline const char *dd_function(const struct _ddebug *dd)
+{
+ return (const char *)dd + dd->function_disp;
+}
+static inline const char *dd_filename(const struct _ddebug *dd)
+{
+ return (const char *)dd + dd->filename_disp;
+}
+static inline const char *dd_format(const struct _ddebug *dd)
+{
+ return (const char *)dd + dd->format_disp;
+}
+#else
static inline const char *dd_modname(const struct _ddebug *dd)
{
return dd->modname;
@@ -53,6 +71,8 @@ static inline const char *dd_format(const struct _ddebug *dd)
{
return dd->format;
}
+#endif
+
static inline unsigned dd_lineno(const struct _ddebug *dd)
{
return dd->flags_lineno >> 8;
--
2.20.1
* Rasmus Villemoes <[email protected]> wrote:
> This reduces the size of struct _ddebug from 56 to 40 bytes. There's
> one such struct for each pr_debug(), netdev_debug() etc. in a
> CONFIG_DYNAMIC_DEBUG kernel. An Ubuntu 4.15 kernel has about 2550
> entries in the __verbose section of vmlinux, amounting to ~40K
> saved. (Modules also become smaller, but it's harder to quantify how
> much that yields at runtime.)
>
> For comparison, the __bug_table section of that Ubuntu kernel is 75576
> bytes, i.e. 6298 12-byte bug_entrys, so GENERIC_BUG_RELATIVE_POINTERS
> saves ~50K.
>
> Due to the build-time sanity checks in asm-generic/dynamic_debug.h, we
> need to add another #undef to vclock_gettime.c.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> arch/x86/Kconfig | 1 +
> arch/x86/entry/vdso/vdso32/vclock_gettime.c | 1 +
> arch/x86/include/asm/Kbuild | 1 +
> 3 files changed, 3 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 5ad92419be19..eb5488b4577d 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -30,6 +30,7 @@ config X86_64
> select SWIOTLB
> select X86_DEV_DMA_OPS
> select ARCH_HAS_SYSCALL_WRAPPER
> + select DYNAMIC_DEBUG_RELATIVE_POINTERS
Acked-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
Le 09/04/2019 à 23:25, Rasmus Villemoes a écrit :
> Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
> four const char* members of struct _ddebug, thus saving 16 bytes per
> instance (one for each pr_debug(), dev_debug() etc. in a
> CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
> work out-of-the-box, though this is only compile-tested.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/Kbuild | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2d0be82c3061..6821c8ae1d62 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -155,6 +155,7 @@ config PPC
> select BUILDTIME_EXTABLE_SORT
> select CLONE_BACKWARDS
> select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
> + select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
Why only PPC64 ? Won't it work with PPC32 ? Or would it be
counter-performant on 32 bits ?
Christophe
> select DYNAMIC_FTRACE if FUNCTION_TRACER
> select EDAC_ATOMIC_SCRUB
> select EDAC_SUPPORT
> diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
> index a0c132bedfae..f332e202192a 100644
> --- a/arch/powerpc/include/asm/Kbuild
> +++ b/arch/powerpc/include/asm/Kbuild
> @@ -3,6 +3,7 @@ generated-y += syscall_table_64.h
> generated-y += syscall_table_c32.h
> generated-y += syscall_table_spu.h
> generic-y += div64.h
> +generic-y += dynamic_debug.h
> generic-y += export.h
> generic-y += irq_regs.h
> generic-y += local64.h
>
On Tue, 23 Apr 2019 17:37:33 +0200 Christophe Leroy <[email protected]> wrote:
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -155,6 +155,7 @@ config PPC
> > select BUILDTIME_EXTABLE_SORT
> > select CLONE_BACKWARDS
> > select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
> > + select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
>
> Why only PPC64 ? Won't it work with PPC32 ? Or would it be
> counter-performant on 32 bits ?
Ditto arm and i386?
On 23/04/2019 21.36, Andrew Morton wrote:
> On Tue, 23 Apr 2019 17:37:33 +0200 Christophe Leroy <[email protected]> wrote:
>
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -155,6 +155,7 @@ config PPC
>>> select BUILDTIME_EXTABLE_SORT
>>> select CLONE_BACKWARDS
>>> select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
>>> + select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
>>
>> Why only PPC64 ? Won't it work with PPC32 ? Or would it be
>> counter-performant on 32 bits ?
>
> Ditto arm and i386?
>
It's pointless on 32-bit platforms - I'm replacing absolute const char*
pointers with a relative s32 offset from the _ddebug descriptor, so if
sizeof(void*)==4 there's no gain.
And yes, the current implementation also wouldn't work out-of-the-box
for 32-bit platforms, since the asm needs to know how to properly
initialize a whole struct _ddebug, which (often) contains a static_key,
which in turn contains a pointer member, which both affects its size as
well as its placement inside _ddebug. The C code in dynamic_debug.c
would likely Just Work, but there's no point in complicating the asm
part for no gain, so there are static_assert()s in place to ensure
BITS_PER_LONG==64 (as well as checking the various offsetof()s etc.).
[I don't think performance matters at all, it's one extra addition to
access these fields, and that is only done in the rare cases where
someone interacts with the dynamic_debug/control sysfs file, and when
one of the activated pr_debug()s is actually hit (so a few extra
instructions drown in the printk overhead).]
I do now see that PPC64 does not select GENERIC_BUG_RELATIVE_POINTERS,
so maybe this scheme simply doesn't work on PPC64, or nobody has done
the work to reduce the sizeof(struct bug_entry) on PPC64? As I said,
I've only compile-tested arm64 and ppc64.
Rasmus
On Tue, Apr 9, 2019 at 11:26 PM Rasmus Villemoes
<[email protected]> wrote:
>
> Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
> four const char* members of struct _ddebug, thus saving 16 bytes per
> instance (one for each pr_debug(), dev_debug() etc. in a
> CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
> work out-of-the-box, though this is only compile-tested.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
This causes a build regression when compiling with clang,
see http://staging-storage.kernelci.org/next-clang/master/next-20190424/arm64/allmodconfig/clang-8/build.log
../init/initramfs.c:477:3: error: invalid operand in inline asm:
'.ifndef __UNIQUE_ID_ddebug18.pushsection __verbose,"aw".type
__UNIQUE_ID_ddebug18, STT_OBJECT.size __UNIQUE_ID_ddebug18,
${5:c}1:__UNIQUE_ID_ddebug18: .int ${0:c} - 1b /*
_ddebug::modname_disp */ .int ${1:c} - 1b /* _ddebug::function_disp */
.int ${2:c} - 1b /* _ddebug::filename_disp */ .int ${3:c} - 1b /*
_ddebug::format_disp */ .int ${4:c} /* _ddebug::flags_lineno */
.org 1b+${5:c}.popsection.set __UNIQUE_ID_ddebug18.ddebug.once,
${6:c}.elseif __UNIQUE_ID_ddebug18.ddebug.once - ${6:c}.line 477 -
1.error "'__UNIQUE_ID_ddebug18' used as _ddebug identifier more than
once".endif'
pr_debug("Detected %s compressed data\n", compress_name);
^
../include/linux/printk.h:336:2: note: expanded from macro 'pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
^
../include/linux/dynamic_debug.h:158:2: note: expanded from macro
'dynamic_pr_debug'
_dynamic_func_call(fmt, __dynamic_pr_debug, \
^
../include/linux/dynamic_debug.h:148:2: note: expanded from macro
'_dynamic_func_call'
__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
^
../include/linux/dynamic_debug.h:128:2: note: expanded from macro
'__dynamic_func_call'
DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt); \
^
../include/asm-generic/dynamic_debug.h:92:15: note: expanded from
macro 'DEFINE_DYNAMIC_DEBUG_METADATA'
asm volatile(".ifndef " __stringify(name) "\n" \
^
I assume the same thing happens on the other architectures as well.
Arnd
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/Kbuild | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9eba5de..d0871d523d5d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -83,6 +83,7 @@ config ARM64
> select CRC32
> select DCACHE_WORD_ACCESS
> select DMA_DIRECT_REMAP
> + select DYNAMIC_DEBUG_RELATIVE_POINTERS
> select EDAC_SUPPORT
> select FRAME_POINTER
> select GENERIC_ALLOCATOR
> diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
> index 1e17ea5c372b..1ead0d645686 100644
> --- a/arch/arm64/include/asm/Kbuild
> +++ b/arch/arm64/include/asm/Kbuild
> @@ -3,6 +3,7 @@ generic-y += delay.h
> generic-y += div64.h
> generic-y += dma.h
> generic-y += dma-contiguous.h
> +generic-y += dynamic_debug.h
> generic-y += early_ioremap.h
> generic-y += emergency-restart.h
> generic-y += hw_irq.h
> --
> 2.20.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 26/04/2019 11.39, Arnd Bergmann wrote:
> On Tue, Apr 9, 2019 at 11:26 PM Rasmus Villemoes
> <[email protected]> wrote:
>>
>> Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
>> four const char* members of struct _ddebug, thus saving 16 bytes per
>> instance (one for each pr_debug(), dev_debug() etc. in a
>> CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
>> work out-of-the-box, though this is only compile-tested.
>>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>
> This causes a build regression when compiling with clang,
> see http://staging-storage.kernelci.org/next-clang/master/next-20190424/arm64/allmodconfig/clang-8/build.log
Yes, see also https://github.com/ClangBuiltLinux/linux/issues/456 .
The quickest short-term fix is to append "if CC_IS_GCC" to the select
statements. Then when a fix lands in clang one can change that to "if
CC_IS_GCC || CLANG_VERSION >= something". It's probably best if we fix
-next builds ASAP instead of waiting for knowing the proper value of
"something". Nathan, Nick, WDYT?
I had 0day verify my patches before sending them out officially, and
thought it also did clang builds. But apparently not, or not with enough
arch/.config combinations?
Rasmus
On Fri, Apr 26, 2019 at 12:05:57PM +0200, Rasmus Villemoes wrote:
> On 26/04/2019 11.39, Arnd Bergmann wrote:
> > On Tue, Apr 9, 2019 at 11:26 PM Rasmus Villemoes
> > <[email protected]> wrote:
> >>
> >> Similar to GENERIC_BUG_RELATIVE_POINTERS, one can now relativize the
> >> four const char* members of struct _ddebug, thus saving 16 bytes per
> >> instance (one for each pr_debug(), dev_debug() etc. in a
> >> CONFIG_DYNAMIC_DEBUG kernel). The asm-generic implementation seems to
> >> work out-of-the-box, though this is only compile-tested.
> >>
> >> Signed-off-by: Rasmus Villemoes <[email protected]>
> >
> > This causes a build regression when compiling with clang,
> > see http://staging-storage.kernelci.org/next-clang/master/next-20190424/arm64/allmodconfig/clang-8/build.log
>
> Yes, see also https://github.com/ClangBuiltLinux/linux/issues/456 .
>
> The quickest short-term fix is to append "if CC_IS_GCC" to the select
> statements. Then when a fix lands in clang one can change that to "if
> CC_IS_GCC || CLANG_VERSION >= something". It's probably best if we fix
> -next builds ASAP instead of waiting for knowing the proper value of
> "something". Nathan, Nick, WDYT?
Those select statements have to be added regardless, we might as well do
it now. It should unbreak the auto builders because they use the latest
clang stable version, which is 8.0.0.
For the record, the fix in Clang is https://reviews.llvm.org/D60887 and
should land shortly unless there are any further objections, meaning
this will be fixed in the 9.0.0 release.
'if CC_IS_GCC || CLANG_VERSION >= 90000' should do it on the arm64 and
powerpc select statements (x86 works fine because %c support has always
been present).
>
> I had 0day verify my patches before sending them out officially, and
> thought it also did clang builds. But apparently not, or not with enough
> arch/.config combinations?
They do not do clang builds unfortunately. Nick was in contact with them
before forced asm-goto on x86 happened and derailed that.
We try to do our own tests and let people know when stuff breaks but
that is usually after it hits -next, rather than the mailing list.
>
> Rasmus
Cheers,
Nathan
Current versions of clang does not like the %c modifier in inline
assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
fails on ppc64. A fix is likely to land in 9.0 (see
https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
build for older versions.
Fixes: powerpc: select DYNAMIC_DEBUG_RELATIVE_POINTERS for PPC64
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
Andrew, please apply and/or fold into 10/10.
arch/powerpc/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6821c8ae1d62..8511137ab963 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -155,7 +155,7 @@ config PPC
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
- select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
+ select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64 && (CC_IS_GCC || CLANG_VERSION >= 90000)
select DYNAMIC_FTRACE if FUNCTION_TRACER
select EDAC_ATOMIC_SCRUB
select EDAC_SUPPORT
--
2.20.1
Current versions of clang does not like the %c modifier in inline
assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
fails on arm64. A fix is likely to land in 9.0 (see
https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
build for older versions.
Fixes: arm64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
Reported-by: Nathan Chancellor <[email protected]>
Reported-by: Arnd Bergmann <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
Andrew, please apply and/or fold into 9/10.
arch/arm64/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index d0871d523d5d..315992e33b17 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -83,7 +83,7 @@ config ARM64
select CRC32
select DCACHE_WORD_ACCESS
select DMA_DIRECT_REMAP
- select DYNAMIC_DEBUG_RELATIVE_POINTERS
+ select DYNAMIC_DEBUG_RELATIVE_POINTERS if CC_IS_GCC || CLANG_VERSION >= 90000
select EDAC_SUPPORT
select FRAME_POINTER
select GENERIC_ALLOCATOR
--
2.20.1
On 26/04/2019 21.06, Rasmus Villemoes wrote:
> Andrew, please apply and/or fold into 9/10.
Hm, I'm getting bounces for Andrew's email address:
The mail system
<[email protected]>: host 172.17.192.39[172.17.192.39] said: 550
5.1.1 <[email protected]>: Recipient address rejected: User
unknown in local recipient table (in reply to RCPT TO command)
I think these fixups need to go through his tree as well. Any ideas
what's going on?
Rasmus
On Fri, Apr 26, 2019 at 09:27:46PM +0200, Rasmus Villemoes wrote:
>On 26/04/2019 21.06, Rasmus Villemoes wrote:
>
>> Andrew, please apply and/or fold into 9/10.
>
>Hm, I'm getting bounces for Andrew's email address:
>
> The mail system
>
><[email protected]>: host 172.17.192.39[172.17.192.39] said: 550
> 5.1.1 <[email protected]>: Recipient address rejected: User
> unknown in local recipient table (in reply to RCPT TO command)
>
>I think these fixups need to go through his tree as well. Any ideas
>what's going on?
When did you get this bounce? Can you give me more headers, especially
with timestamps? Andrew's mailbox was recently switched to a different
platform, so it's possible you're getting some unintended backscatter
from when we briefly delivered mail to both.
-K
On Fri, Apr 26, 2019 at 09:27:46PM +0200, Rasmus Villemoes wrote:
>On 26/04/2019 21.06, Rasmus Villemoes wrote:
>
>> Andrew, please apply and/or fold into 9/10.
>
>Hm, I'm getting bounces for Andrew's email address:
>
> The mail system
>
><[email protected]>: host 172.17.192.39[172.17.192.39] said: 550
> 5.1.1 <[email protected]>: Recipient address rejected: User
> unknown in local recipient table (in reply to RCPT TO command)
>
>I think these fixups need to go through his tree as well. Any ideas
>what's going on?
OK, I see these, too. We'll fix it up, but the important bit here is
that Andrew *is* getting all mail you send to his address. The bounces
are from a secondary system that's supposed to have been removed from
the forwarding destinations, but somehow is still getting copies of his
mail.
-K
On Fri, Apr 26, 2019 at 12:06 PM Rasmus Villemoes
<[email protected]> wrote:
>
> Current versions of clang does not like the %c modifier in inline
> assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
> fails on arm64. A fix is likely to land in 9.0 (see
> https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
> build for older versions.
>
> Fixes: arm64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> Andrew, please apply and/or fold into 9/10.
>
> arch/arm64/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index d0871d523d5d..315992e33b17 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -83,7 +83,7 @@ config ARM64
> select CRC32
> select DCACHE_WORD_ACCESS
> select DMA_DIRECT_REMAP
> - select DYNAMIC_DEBUG_RELATIVE_POINTERS
> + select DYNAMIC_DEBUG_RELATIVE_POINTERS if CC_IS_GCC || CLANG_VERSION >= 90000
I just landed the fix for this in Clang, I think around the time you
sent the patch. Should ship in Clang 9. Thanks for unbreaking our
build.
Reviewed-by: Nick Desaulniers <[email protected]>
> select EDAC_SUPPORT
> select FRAME_POINTER
> select GENERIC_ALLOCATOR
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
On Fri, Apr 26, 2019 at 12:06 PM Rasmus Villemoes
<[email protected]> wrote:
>
> Current versions of clang does not like the %c modifier in inline
> assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
> fails on ppc64. A fix is likely to land in 9.0 (see
> https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
> build for older versions.
>
> Fixes: powerpc: select DYNAMIC_DEBUG_RELATIVE_POINTERS for PPC64
> Reported-by: Nathan Chancellor <[email protected]>
> Reported-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Thanks for fixing the build.
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> Andrew, please apply and/or fold into 10/10.
>
> arch/powerpc/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 6821c8ae1d62..8511137ab963 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -155,7 +155,7 @@ config PPC
> select BUILDTIME_EXTABLE_SORT
> select CLONE_BACKWARDS
> select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN
> - select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64
> + select DYNAMIC_DEBUG_RELATIVE_POINTERS if PPC64 && (CC_IS_GCC || CLANG_VERSION >= 90000)
> select DYNAMIC_FTRACE if FUNCTION_TRACER
> select EDAC_ATOMIC_SCRUB
> select EDAC_SUPPORT
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
On Mon, Apr 29, 2019 at 10:32 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Fri, Apr 26, 2019 at 12:06 PM Rasmus Villemoes
> <[email protected]> wrote:
> >
> > Current versions of clang does not like the %c modifier in inline
> > assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
> > fails on arm64. A fix is likely to land in 9.0 (see
> > https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
> > build for older versions.
> >
> > Fixes: arm64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
> > Reported-by: Nathan Chancellor <[email protected]>
> > Reported-by: Arnd Bergmann <[email protected]>
> > Signed-off-by: Rasmus Villemoes <[email protected]>
> > ---
> > Andrew, please apply and/or fold into 9/10.
> >
> > arch/arm64/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index d0871d523d5d..315992e33b17 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -83,7 +83,7 @@ config ARM64
> > select CRC32
> > select DCACHE_WORD_ACCESS
> > select DMA_DIRECT_REMAP
> > - select DYNAMIC_DEBUG_RELATIVE_POINTERS
> > + select DYNAMIC_DEBUG_RELATIVE_POINTERS if CC_IS_GCC || CLANG_VERSION >= 90000
>
> I just landed the fix for this in Clang, I think around the time you
> sent the patch. Should ship in Clang 9. Thanks for unbreaking our
> build.
> Reviewed-by: Nick Desaulniers <[email protected]>
+ Dan
who's looking for this to get picked up to unbreak KernelCI arm64+clang builds
https://staging.kernelci.org/build/id/5cc6e080cf3a0f9d66257f6d/
--
Thanks,
~Nick Desaulniers
On 30/04/2019 20.22, Nick Desaulniers wrote:
> On Mon, Apr 29, 2019 at 10:32 AM Nick Desaulniers
> <[email protected]> wrote:
>>
>> On Fri, Apr 26, 2019 at 12:06 PM Rasmus Villemoes
>> <[email protected]> wrote:
>>>
>>> Current versions of clang does not like the %c modifier in inline
>>> assembly for targets other than x86, so any DYNAMIC_DEBUG=y build
>>> fails on arm64. A fix is likely to land in 9.0 (see
>>> https://github.com/ClangBuiltLinux/linux/issues/456), but unbreak the
>>> build for older versions.
>>>
>>> Fixes: arm64: select DYNAMIC_DEBUG_RELATIVE_POINTERS
>>> Reported-by: Nathan Chancellor <[email protected]>
>>> Reported-by: Arnd Bergmann <[email protected]>
>>> Signed-off-by: Rasmus Villemoes <[email protected]>
>>> ---
>>> Andrew, please apply and/or fold into 9/10.
Andrew, friendly ping.
>>> arch/arm64/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index d0871d523d5d..315992e33b17 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -83,7 +83,7 @@ config ARM64
>>> select CRC32
>>> select DCACHE_WORD_ACCESS
>>> select DMA_DIRECT_REMAP
>>> - select DYNAMIC_DEBUG_RELATIVE_POINTERS
>>> + select DYNAMIC_DEBUG_RELATIVE_POINTERS if CC_IS_GCC || CLANG_VERSION >= 90000
>>
>> I just landed the fix for this in Clang, I think around the time you
>> sent the patch. Should ship in Clang 9. Thanks for unbreaking our
>> build.
Sorry for breaking it in the first place :-/
Rasmus
On 09/04/2019 23.25, Rasmus Villemoes wrote:
> While refreshing these patches, which were orignally just targeted at
> x86-64, it occured to me that despite the implementation relying on
> inline asm, there's nothing x86 specific about it, and indeed it seems
> to work out-of-the-box for ppc64 and arm64 as well, but those have
> only been compile-tested.
So, apart from the Clang build failures for non-x86, I now also got a
report that gcc 4.8 miscompiles this stuff in some cases [1], even for
x86 - gcc 4.9 does not seem to have the problem. So, given that the 5.2
merge window just opened, I suppose this is the point where I should
pull the plug on this experiment :(
Rasmus
[1] Specifically, the problem manifested in net/ipv4/tcp_input.c: Both
uses of the static inline inet_csk_clear_xmit_timer() pass a
compile-time constant 'what', so the ifs get folded away and both uses
are completely inlined. Yet, gcc still decides to emit a copy of the
final 'else' branch of inet_csk_clear_xmit_timer() as its own
inet_csk_reset_xmit_timer.part.55 function, which is of course unused.
And despite the asm() that defines the ddebug descriptor being an "asm
volatile", gcc thinks it's fine to elide that (the code path is
unreachable, after all....), so the entire asm for that function is
.section .text.unlikely
.type inet_csk_reset_xmit_timer.part.55, @function
inet_csk_reset_xmit_timer.part.55:
movq $.LC1, %rsi #,
movq $__UNIQUE_ID_ddebug160, %rdi #,
xorl %eax, %eax #
jmp __dynamic_pr_debug #
.size inet_csk_reset_xmit_timer.part.55,
.-inet_csk_reset_xmit_timer.part.55
which of course fails to link since the symbol __UNIQUE_ID_ddebug160 is
nowhere defined.
* Rasmus Villemoes <[email protected]> wrote:
> On 09/04/2019 23.25, Rasmus Villemoes wrote:
>
> > While refreshing these patches, which were orignally just targeted at
> > x86-64, it occured to me that despite the implementation relying on
> > inline asm, there's nothing x86 specific about it, and indeed it seems
> > to work out-of-the-box for ppc64 and arm64 as well, but those have
> > only been compile-tested.
>
> So, apart from the Clang build failures for non-x86, I now also got a
> report that gcc 4.8 miscompiles this stuff in some cases [1], even for
> x86 - gcc 4.9 does not seem to have the problem. So, given that the 5.2
> merge window just opened, I suppose this is the point where I should
> pull the plug on this experiment :(
>
> Rasmus
>
> [1] Specifically, the problem manifested in net/ipv4/tcp_input.c: Both
> uses of the static inline inet_csk_clear_xmit_timer() pass a
> compile-time constant 'what', so the ifs get folded away and both uses
> are completely inlined. Yet, gcc still decides to emit a copy of the
> final 'else' branch of inet_csk_clear_xmit_timer() as its own
> inet_csk_reset_xmit_timer.part.55 function, which is of course unused.
> And despite the asm() that defines the ddebug descriptor being an "asm
> volatile", gcc thinks it's fine to elide that (the code path is
> unreachable, after all....), so the entire asm for that function is
>
> .section .text.unlikely
> .type inet_csk_reset_xmit_timer.part.55, @function
> inet_csk_reset_xmit_timer.part.55:
> movq $.LC1, %rsi #,
> movq $__UNIQUE_ID_ddebug160, %rdi #,
> xorl %eax, %eax #
> jmp __dynamic_pr_debug #
> .size inet_csk_reset_xmit_timer.part.55,
> .-inet_csk_reset_xmit_timer.part.55
>
> which of course fails to link since the symbol __UNIQUE_ID_ddebug160 is
> nowhere defined.
It's sad to see such nice data footprint savings go the way of the dodo
just because GCC 4.8 is buggy.
The current compatibility cut-off is GCC 4.6:
GNU C 4.6 gcc --version
Do we know where the GCC bug was fixed, was it in GCC 4.9?
According to the GCC release dates:
https://www.gnu.org/software/gcc/releases.html
4.8.0 was released in early-2013, while 4.9.0 was released in early-2014.
So the tooling compatibility window for latest upstream would narrow from
~6 years to ~5 years.
Thanks,
Ingo
On 06/05/2019 09.05, Ingo Molnar wrote:
>
>
> It's sad to see such nice data footprint savings go the way of the dodo
> just because GCC 4.8 is buggy.
>
> The current compatibility cut-off is GCC 4.6:
>
> GNU C 4.6 gcc --version
>
> Do we know where the GCC bug was fixed, was it in GCC 4.9?
Not sure. The report was from a build on CentOS with gcc 4.8.5, so I
tried installing the gcc-4.8 package on my Ubuntu machine and could
reproduce. Then I tried installed gcc-4.9, and after disabling
CONFIG_RETPOLINE (both CentOS and Ubuntu carry backported retpoline
support in their 4.8, but apparently not 4.9), I could see that the
problem was gone. But whether it's gone because it no longer elides an
asm volatile() on a code path it otherwise emits code for, or because it
simply doesn't emit the unused static inline() at all I don't know.
I thought 0day also tested a range of supported compiler versions, so I
was rather surprised at getting this report at all. But I suppose the
arch/config matrix is already pretty huge. Anyway, the bug certainly
doesn't exist in any of the gcc versions 0day does test.
I _am_ bending the C rules a bit with the "extern some_var; asm
volatile(".section some_section\nsome_var: blabla");". I should probably
ask on the gcc list whether this way of defining a local symbol in
inline assembly and referring to it from C is supposed to work, or it
just happens to work by chance.
Rasmus
* Rasmus Villemoes <[email protected]> wrote:
> I _am_ bending the C rules a bit with the "extern some_var; asm
> volatile(".section some_section\nsome_var: blabla");". I should
> probably ask on the gcc list whether this way of defining a local
> symbol in inline assembly and referring to it from C is supposed to
> work, or it just happens to work by chance.
Doing that would be rather useful I think.
Thanks,
Ingo
On Mon, May 06, 2019 at 09:34:55AM +0200, Rasmus Villemoes wrote:
> I _am_ bending the C rules a bit with the "extern some_var; asm
> volatile(".section some_section\nsome_var: blabla");". I should probably
> ask on the gcc list whether this way of defining a local symbol in
> inline assembly and referring to it from C is supposed to work, or it
> just happens to work by chance.
It only works by chance. There is no way GCC can know the asm needs
that variable. If you make it (or its address) an input of the asm it
should work as far as I can see? (Need exact code to analyse it exactly).
Segher