Make better use of noreturn. It allows optimising compilers to produce more efficient code. George: I request that this is included for 4.4 - It is no functional change, but quite a nice improvement in terms of code size. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <george.dunlap@eu.citrix.com>
Andrew Cooper
2013-Nov-25 10:25 UTC
[PATCH 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))
Make a formal define for noreturn in compiler.h, and replace opencoded
uses of __attribute__((noreturn)).
There is a stale statement in sh_skip_sync(); BUG() now contains
unreachable(), removing the justification for having "return 0".
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>
---
I have compile tested this for both arm32 and arm64
---
xen/arch/arm/early_printk.c | 2 +-
xen/arch/x86/crash.c | 2 +-
xen/arch/x86/mm/shadow/common.c | 1 -
xen/arch/x86/shutdown.c | 2 +-
xen/include/asm-arm/early_printk.h | 4 ++--
xen/include/xen/compiler.h | 2 ++
xen/include/xen/lib.h | 2 +-
xen/include/xen/sched.h | 4 ++--
8 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
index 058d044..00ec72a 100644
--- a/xen/arch/arm/early_printk.c
+++ b/xen/arch/arm/early_printk.c
@@ -52,7 +52,7 @@ void __init early_printk(const char *fmt, ...)
va_end(args);
}
-void __attribute__((noreturn)) __init
+void noreturn __init
early_panic(const char *fmt, ...)
{
va_list args;
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 01fd906..836babb 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -36,7 +36,7 @@ static unsigned int crashing_cpu;
static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
/* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. */
-void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
+void noreturn do_nmi_crash(struct cpu_user_regs *regs)
{
int cpu = smp_processor_id();
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 0bfa595..c1a416c 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -875,7 +875,6 @@ static int sh_skip_sync(struct vcpu *v, mfn_t gl1mfn)
SHADOW_ERROR("gmfn %#lx was OOS but not shadowed as an l1.\n",
mfn_x(gl1mfn));
BUG();
- return 0; /* BUG() is no longer __attribute__((noreturn)). */
}
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 6eba271..6143c40 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -85,7 +85,7 @@ static inline void kb_wait(void)
break;
}
-static void __attribute__((noreturn)) __machine_halt(void *unused)
+static void noreturn __machine_halt(void *unused)
{
local_irq_disable();
for ( ; ; )
diff --git a/xen/include/asm-arm/early_printk.h
b/xen/include/asm-arm/early_printk.h
index 707bbf7..3cb8dab 100644
--- a/xen/include/asm-arm/early_printk.h
+++ b/xen/include/asm-arm/early_printk.h
@@ -26,7 +26,7 @@
void early_printk(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
-void early_panic(const char *fmt, ...) __attribute__((noreturn))
+void early_panic(const char *fmt, ...) noreturn
__attribute__((format (printf, 1, 2)));
#else
@@ -35,7 +35,7 @@ static inline __attribute__((format (printf, 1, 2))) void
early_printk(const char *fmt, ...)
{}
-static inline void __attribute__((noreturn))
+static inline void noreturn
__attribute__((format (printf, 1, 2))) early_panic(const char *fmt, ...)
{while(1);}
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 7d6805c..c80398d 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -14,6 +14,8 @@
#define always_inline __inline__ __attribute__ ((always_inline))
#define noinline __attribute__((noinline))
+#define noreturn __attribute__((noreturn))
+
#if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__
< 5))
#define unreachable() do {} while (1)
#else
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 5b258fd..814fcb4 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -8,7 +8,7 @@
#include <xen/string.h>
#include <asm/bug.h>
-void __bug(char *file, int line) __attribute__((noreturn));
+void __bug(char *file, int line) noreturn;
void __warn(char *file, int line);
#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index cbdf377..0d2a442 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -578,7 +578,7 @@ void __domain_crash(struct domain *d);
* Mark current domain as crashed and synchronously deschedule from the local
* processor. This function never returns.
*/
-void __domain_crash_synchronous(void) __attribute__((noreturn));
+void __domain_crash_synchronous(void) noreturn;
#define domain_crash_synchronous() do { \
printk("domain_crash_sync called from %s:%d\n", __FILE__,
__LINE__); \
__domain_crash_synchronous(); \
@@ -589,7 +589,7 @@ void __domain_crash_synchronous(void)
__attribute__((noreturn));
* the crash occured. If addr is 0, look up address from last extable
* redirection.
*/
-void asm_domain_crash_synchronous(unsigned long addr)
__attribute__((noreturn));
+void asm_domain_crash_synchronous(unsigned long addr) noreturn;
#define set_current_state(_s) do { current->state = (_s); } while (0)
void scheduler_init(void);
--
1.7.10.4
Andrew Cooper
2013-Nov-25 10:25 UTC
[PATCH 2/2] xen: Identify panic and reboot/halt functions as noreturn
On an x86 build (GCC Debian 4.7.2-5), this reduces the .text size by exactly
4K, and .init.text by 1751 bytes.
Even in a non-debug build, the generated code uses `call` rather than `jmp` so
there should be no impact on any stack trace generation.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>
---
I believe I have identified each reasonable ARM function which could be tagged
as noreturn, but am not overly familiar with the codebase.
I have compile tested arm32 and arm64
---
xen/arch/arm/shutdown.c | 6 +++---
xen/arch/arm/smpboot.c | 2 +-
xen/arch/x86/cpu/mcheck/mce.c | 2 +-
xen/arch/x86/cpu/mcheck/mce.h | 2 +-
xen/arch/x86/shutdown.c | 6 +++---
xen/common/shutdown.c | 4 ++--
xen/drivers/char/console.c | 2 +-
xen/include/asm-arm/smp.h | 2 +-
xen/include/xen/lib.h | 2 +-
xen/include/xen/shutdown.h | 10 ++++++----
10 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
index 767cc12..58f1cf1 100644
--- a/xen/arch/arm/shutdown.c
+++ b/xen/arch/arm/shutdown.c
@@ -11,12 +11,12 @@ static void raw_machine_reset(void)
platform_reset();
}
-static void halt_this_cpu(void *arg)
+static void noreturn halt_this_cpu(void *arg)
{
stop_cpu();
}
-void machine_halt(void)
+void noreturn machine_halt(void)
{
watchdog_disable();
console_start_sync();
@@ -25,7 +25,7 @@ void machine_halt(void)
halt_this_cpu(NULL);
}
-void machine_restart(unsigned int delay_millisecs)
+void noreturn machine_restart(unsigned int delay_millisecs)
{
int timeout = 10;
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 6c90fa6..7242331 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -310,7 +310,7 @@ void __cpu_disable(void)
* scheduler will drop to the idle loop, which will call stop_cpu(). */
}
-void stop_cpu(void)
+void noreturn stop_cpu(void)
{
local_irq_disable();
cpu_is_dead = 1;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 93d7ae1..8a226ca 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -1534,7 +1534,7 @@ static void mc_panic_dump(void)
dprintk(XENLOG_ERR, "End dump mc_info, %x mcinfo dumped\n",
mcinfo_dumpped);
}
-void mc_panic(char *s)
+void noreturn mc_panic(char *s)
{
is_mc_panic = 1;
console_force_unlock();
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index cbd123d..db791ff 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -57,7 +57,7 @@ int mce_available(struct cpuinfo_x86 *c);
unsigned int mce_firstbank(struct cpuinfo_x86 *c);
/* Helper functions used for collecting error telemetry */
struct mc_info *x86_mcinfo_getptr(void);
-void mc_panic(char *s);
+void mc_panic(char *s) noreturn;
void x86_mc_get_cpu_info(unsigned, uint32_t *, uint16_t *, uint16_t *,
uint32_t *, uint32_t *, uint32_t *, uint32_t *);
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 6143c40..a99a599 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -92,7 +92,7 @@ static void noreturn __machine_halt(void *unused)
halt();
}
-void machine_halt(void)
+void noreturn machine_halt(void)
{
watchdog_disable();
console_start_sync();
@@ -452,12 +452,12 @@ static int __init reboot_init(void)
}
__initcall(reboot_init);
-static void __machine_restart(void *pdelay)
+static void noreturn __machine_restart(void *pdelay)
{
machine_restart(*(unsigned int *)pdelay);
}
-void machine_restart(unsigned int delay_millisecs)
+void noreturn machine_restart(unsigned int delay_millisecs)
{
unsigned int i, attempt;
enum reboot_type orig_reboot_type = reboot_type;
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 9bccd34..87424b1 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -17,7 +17,7 @@
bool_t __read_mostly opt_noreboot;
boolean_param("noreboot", opt_noreboot);
-static void maybe_reboot(void)
+static void noreturn maybe_reboot(void)
{
if ( opt_noreboot )
{
@@ -32,7 +32,7 @@ static void maybe_reboot(void)
}
}
-void dom0_shutdown(u8 reason)
+void noreturn dom0_shutdown(u8 reason)
{
switch ( reason )
{
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 508f845..7347f96 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1038,7 +1038,7 @@ __initcall(debugtrace_init);
* **************************************************************
*/
-void panic(const char *fmt, ...)
+void noreturn panic(const char *fmt, ...)
{
va_list args;
unsigned long flags;
diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
index 1485cc6..0f57c68 100644
--- a/xen/include/asm-arm/smp.h
+++ b/xen/include/asm-arm/smp.h
@@ -15,7 +15,7 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
#define raw_smp_processor_id() (get_processor_id())
-extern void stop_cpu(void);
+extern void stop_cpu(void) noreturn;
extern int arch_smp_init(void);
extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 814fcb4..c8b35f1 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -87,7 +87,7 @@ extern void printk(const char *format, ...)
__attribute__ ((format (printf, 1, 2)));
extern void guest_printk(const struct domain *d, const char *format, ...)
__attribute__ ((format (printf, 2, 3)));
-extern void panic(const char *format, ...)
+extern void panic(const char *format, ...) noreturn
__attribute__ ((format (printf, 1, 2)));
extern long vm_assist(struct domain *, unsigned int, unsigned int);
extern int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst);
diff --git a/xen/include/xen/shutdown.h b/xen/include/xen/shutdown.h
index 2bee748..dfe80d0 100644
--- a/xen/include/xen/shutdown.h
+++ b/xen/include/xen/shutdown.h
@@ -1,13 +1,15 @@
#ifndef __XEN_SHUTDOWN_H__
#define __XEN_SHUTDOWN_H__
+#include <xen/compiler.h>
+
/* opt_noreboot: If true, machine will need manual reset on error. */
extern bool_t opt_noreboot;
-void dom0_shutdown(u8 reason);
+void dom0_shutdown(u8 reason) noreturn;
-void machine_restart(unsigned int delay_millisecs);
-void machine_halt(void);
-void machine_power_off(void);
+void machine_restart(unsigned int delay_millisecs) noreturn;
+void machine_halt(void) noreturn;
+void machine_power_off(void) noreturn;
#endif /* __XEN_SHUTDOWN_H__ */
--
1.7.10.4
Ian Campbell
2013-Nov-25 10:31 UTC
Re: [PATCH 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))
On Mon, 2013-11-25 at 10:25 +0000, Andrew Cooper wrote:> Make a formal define for noreturn in compiler.h, and replace opencoded > uses of __attribute__((noreturn)). > > There is a stale statement in sh_skip_sync(); BUG() now contains > unreachable(), removing the justification for having "return 0". > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <JBeulich@suse.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> CC: Stefano Stabellini <stefano.stabellini@citrix.com> > CC: Tim Deegan <tim@xen.org> > > --- > > I have compile tested this for both arm32 and arm64 > --- > xen/arch/arm/early_printk.c | 2 +- > xen/arch/x86/crash.c | 2 +- > xen/arch/x86/mm/shadow/common.c | 1 - > xen/arch/x86/shutdown.c | 2 +- > xen/include/asm-arm/early_printk.h | 4 ++-- > xen/include/xen/compiler.h | 2 ++ > xen/include/xen/lib.h | 2 +- > xen/include/xen/sched.h | 4 ++-- > 8 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c > index 058d044..00ec72a 100644 > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -52,7 +52,7 @@ void __init early_printk(const char *fmt, ...) > va_end(args); > } > > -void __attribute__((noreturn)) __init > +void noreturn __init > early_panic(const char *fmt, ...) > { > va_list args; > diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c > index 01fd906..836babb 100644 > --- a/xen/arch/x86/crash.c > +++ b/xen/arch/x86/crash.c > @@ -36,7 +36,7 @@ static unsigned int crashing_cpu; > static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done); > > /* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. */ > -void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs) > +void noreturn do_nmi_crash(struct cpu_user_regs *regs) > { > int cpu = smp_processor_id(); > > diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c > index 0bfa595..c1a416c 100644 > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -875,7 +875,6 @@ static int sh_skip_sync(struct vcpu *v, mfn_t gl1mfn) > SHADOW_ERROR("gmfn %#lx was OOS but not shadowed as an l1.\n", > mfn_x(gl1mfn)); > BUG(); > - return 0; /* BUG() is no longer __attribute__((noreturn)). */ > } > > > diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c > index 6eba271..6143c40 100644 > --- a/xen/arch/x86/shutdown.c > +++ b/xen/arch/x86/shutdown.c > @@ -85,7 +85,7 @@ static inline void kb_wait(void) > break; > } > > -static void __attribute__((noreturn)) __machine_halt(void *unused) > +static void noreturn __machine_halt(void *unused) > { > local_irq_disable(); > for ( ; ; ) > diff --git a/xen/include/asm-arm/early_printk.h b/xen/include/asm-arm/early_printk.h > index 707bbf7..3cb8dab 100644 > --- a/xen/include/asm-arm/early_printk.h > +++ b/xen/include/asm-arm/early_printk.h > @@ -26,7 +26,7 @@ > > void early_printk(const char *fmt, ...) > __attribute__((format (printf, 1, 2))); > -void early_panic(const char *fmt, ...) __attribute__((noreturn)) > +void early_panic(const char *fmt, ...) noreturn > __attribute__((format (printf, 1, 2))); > > #else > @@ -35,7 +35,7 @@ static inline __attribute__((format (printf, 1, 2))) void > early_printk(const char *fmt, ...) > {} > > -static inline void __attribute__((noreturn)) > +static inline void noreturn > __attribute__((format (printf, 1, 2))) early_panic(const char *fmt, ...) > {while(1);} > > diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h > index 7d6805c..c80398d 100644 > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -14,6 +14,8 @@ > #define always_inline __inline__ __attribute__ ((always_inline)) > #define noinline __attribute__((noinline)) > > +#define noreturn __attribute__((noreturn)) > + > #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5)) > #define unreachable() do {} while (1) > #else > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h > index 5b258fd..814fcb4 100644 > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -8,7 +8,7 @@ > #include <xen/string.h> > #include <asm/bug.h> > > -void __bug(char *file, int line) __attribute__((noreturn)); > +void __bug(char *file, int line) noreturn; > void __warn(char *file, int line); > > #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index cbdf377..0d2a442 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -578,7 +578,7 @@ void __domain_crash(struct domain *d); > * Mark current domain as crashed and synchronously deschedule from the local > * processor. This function never returns. > */ > -void __domain_crash_synchronous(void) __attribute__((noreturn)); > +void __domain_crash_synchronous(void) noreturn; > #define domain_crash_synchronous() do { \ > printk("domain_crash_sync called from %s:%d\n", __FILE__, __LINE__); \ > __domain_crash_synchronous(); \ > @@ -589,7 +589,7 @@ void __domain_crash_synchronous(void) __attribute__((noreturn)); > * the crash occured. If addr is 0, look up address from last extable > * redirection. > */ > -void asm_domain_crash_synchronous(unsigned long addr) __attribute__((noreturn)); > +void asm_domain_crash_synchronous(unsigned long addr) noreturn; > > #define set_current_state(_s) do { current->state = (_s); } while (0) > void scheduler_init(void);
Ian Campbell
2013-Nov-25 10:34 UTC
Re: [PATCH 2/2] xen: Identify panic and reboot/halt functions as noreturn
On Mon, 2013-11-25 at 10:25 +0000, Andrew Cooper wrote:> On an x86 build (GCC Debian 4.7.2-5), this reduces the .text size by exactly > 4K, and .init.text by 1751 bytes. > > Even in a non-debug build, the generated code uses `call` rather than `jmp` so > there should be no impact on any stack trace generation. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Ian Campbell <ian.campbell@citrix.com> > CC: Stefano Stabellini <stefano.stabellini@citrix.com> > CC: Tim Deegan <tim@xen.org> > > --- > > I believe I have identified each reasonable ARM function which could be tagged > as noreturn, but am not overly familiar with the codebase. > > I have compile tested arm32 and arm64 > --- > xen/arch/arm/shutdown.c | 6 +++--- > xen/arch/arm/smpboot.c | 2 +- > xen/arch/x86/cpu/mcheck/mce.c | 2 +- > xen/arch/x86/cpu/mcheck/mce.h | 2 +- > xen/arch/x86/shutdown.c | 6 +++--- > xen/common/shutdown.c | 4 ++-- > xen/drivers/char/console.c | 2 +- > xen/include/asm-arm/smp.h | 2 +- > xen/include/xen/lib.h | 2 +- > xen/include/xen/shutdown.h | 10 ++++++---- > 10 files changed, 20 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c > index 767cc12..58f1cf1 100644 > --- a/xen/arch/arm/shutdown.c > +++ b/xen/arch/arm/shutdown.c > @@ -11,12 +11,12 @@ static void raw_machine_reset(void) > platform_reset(); > } > > -static void halt_this_cpu(void *arg) > +static void noreturn halt_this_cpu(void *arg) > { > stop_cpu();You also tag stop_cpu, are both necessary? Especially for a static function.> } > > -void machine_halt(void) > +void noreturn machine_halt(void)And you also tag halt_this_cpu which machine_halt ends with.> { > watchdog_disable(); > console_start_sync(); > @@ -25,7 +25,7 @@ void machine_halt(void) > halt_this_cpu(NULL); > } > > -void machine_restart(unsigned int delay_millisecs) > +void noreturn machine_restart(unsigned int delay_millisecs) > { > int timeout = 10; > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index 6c90fa6..7242331 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -310,7 +310,7 @@ void __cpu_disable(void) > * scheduler will drop to the idle loop, which will call stop_cpu(). */ > } > > -void stop_cpu(void) > +void noreturn stop_cpu(void) > { > local_irq_disable(); > cpu_is_dead = 1;
Andrew Cooper
2013-Nov-25 10:37 UTC
Re: [PATCH 2/2] xen: Identify panic and reboot/halt functions as noreturn
On 25/11/13 10:34, Ian Campbell wrote:> On Mon, 2013-11-25 at 10:25 +0000, Andrew Cooper wrote: >> On an x86 build (GCC Debian 4.7.2-5), this reduces the .text size by exactly >> 4K, and .init.text by 1751 bytes. >> >> Even in a non-debug build, the generated code uses `call` rather than `jmp` so >> there should be no impact on any stack trace generation. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Ian Campbell <ian.campbell@citrix.com> >> CC: Stefano Stabellini <stefano.stabellini@citrix.com> >> CC: Tim Deegan <tim@xen.org> >> >> --- >> >> I believe I have identified each reasonable ARM function which could be tagged >> as noreturn, but am not overly familiar with the codebase. >> >> I have compile tested arm32 and arm64 >> --- >> xen/arch/arm/shutdown.c | 6 +++--- >> xen/arch/arm/smpboot.c | 2 +- >> xen/arch/x86/cpu/mcheck/mce.c | 2 +- >> xen/arch/x86/cpu/mcheck/mce.h | 2 +- >> xen/arch/x86/shutdown.c | 6 +++--- >> xen/common/shutdown.c | 4 ++-- >> xen/drivers/char/console.c | 2 +- >> xen/include/asm-arm/smp.h | 2 +- >> xen/include/xen/lib.h | 2 +- >> xen/include/xen/shutdown.h | 10 ++++++---- >> 10 files changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c >> index 767cc12..58f1cf1 100644 >> --- a/xen/arch/arm/shutdown.c >> +++ b/xen/arch/arm/shutdown.c >> @@ -11,12 +11,12 @@ static void raw_machine_reset(void) >> platform_reset(); >> } >> >> -static void halt_this_cpu(void *arg) >> +static void noreturn halt_this_cpu(void *arg) >> { >> stop_cpu(); > You also tag stop_cpu, are both necessary? Especially for a static > function. > >> } >> >> -void machine_halt(void) >> +void noreturn machine_halt(void) > And you also tag halt_this_cpu which machine_halt ends with.Based on my experience getting x86 to compile with panic alone as noreturn, yes. machine_halt() certainly as I changed the common function declaration. ~Andrew> >> { >> watchdog_disable(); >> console_start_sync(); >> @@ -25,7 +25,7 @@ void machine_halt(void) >> halt_this_cpu(NULL); >> } >> >> -void machine_restart(unsigned int delay_millisecs) >> +void noreturn machine_restart(unsigned int delay_millisecs) >> { >> int timeout = 10; >> >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index 6c90fa6..7242331 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -310,7 +310,7 @@ void __cpu_disable(void) >> * scheduler will drop to the idle loop, which will call stop_cpu(). */ >> } >> >> -void stop_cpu(void) >> +void noreturn stop_cpu(void) >> { >> local_irq_disable(); >> cpu_is_dead = 1; >
Tim Deegan
2013-Nov-25 10:45 UTC
Re: [PATCH 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))
At 10:25 +0000 on 25 Nov (1385371541), Andrew Cooper wrote:> Make a formal define for noreturn in compiler.h, and replace opencoded > uses of __attribute__((noreturn)). > > There is a stale statement in sh_skip_sync(); BUG() now contains > unreachable(), removing the justification for having "return 0". > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>Acked-by: Tim Deegan <tim@xen.org>
Tim Deegan
2013-Nov-25 10:45 UTC
Re: [PATCH 2/2] xen: Identify panic and reboot/halt functions as noreturn
At 10:25 +0000 on 25 Nov (1385371542), Andrew Cooper wrote:> On an x86 build (GCC Debian 4.7.2-5), this reduces the .text size by exactly > 4K, and .init.text by 1751 bytes. > > Even in a non-debug build, the generated code uses `call` rather than `jmp` so > there should be no impact on any stack trace generation. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>Acked-by: Tim Deegan <tim@xen.org>
Jan Beulich
2013-Nov-25 10:57 UTC
Re: [PATCH 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))
>>> On 25.11.13 at 11:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -52,7 +52,7 @@ void __init early_printk(const char *fmt, ...) > va_end(args); > } > > -void __attribute__((noreturn)) __init > +void noreturn __init > early_panic(const char *fmt, ...) > { > va_list args;Here and elsewhere - these belong on the declaration of the function (and only there, or else there''s pointless redundancy), not the definition (or else the compiler can''t take proper action on the respective call sites). Only static functions not needing forward declarations would have the attribute on their definitions. Jan
On Mon, Nov 25, 2013 at 10:25 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> Make better use of noreturn. It allows optimising compilers to produce more > efficient code. > > George: > I request that this is included for 4.4 - It is no functional change, but > quite a nice improvement in terms of code size.No functional change *if compilers are correct*. If they''re not, for any reason, it will be very difficult to actually catch between now and the release. I''m not inclined to think that a reduction of 6k is a big enough improvement to take the risk at this point. -George
On 25/11/13 11:47, George Dunlap wrote:> On Mon, Nov 25, 2013 at 10:25 AM, Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> Make better use of noreturn. It allows optimising compilers to produce more >> efficient code. >> >> George: >> I request that this is included for 4.4 - It is no functional change, but >> quite a nice improvement in terms of code size. > No functional change *if compilers are correct*. If they''re not, for > any reason, it will be very difficult to actually catch between now > and the release. > > I''m not inclined to think that a reduction of 6k is a big enough > improvement to take the risk at this point. > > -GeorgePatch 1 is literally just textual replacement, cleaning up its current uses in the codebase. Patch 2 applies its use to more functions in the codebase. While I agree that it is "No functional change if compilers are correct", this kind of paranoia could be applied to any and every patch we consider taking. Anyway, Linux uses it far more than we do at the moment. As for difficulty to catch regressions, I would disagree. Failures to reboot correctly will not pass the oss test, and the other alternative is failures to crash correctly. ~Andrew
On 11/25/2013 02:02 PM, Andrew Cooper wrote:> On 25/11/13 11:47, George Dunlap wrote: >> On Mon, Nov 25, 2013 at 10:25 AM, Andrew Cooper >> <andrew.cooper3@citrix.com> wrote: >>> Make better use of noreturn. It allows optimising compilers to produce more >>> efficient code. >>> >>> George: >>> I request that this is included for 4.4 - It is no functional change, but >>> quite a nice improvement in terms of code size. >> No functional change *if compilers are correct*. If they''re not, for >> any reason, it will be very difficult to actually catch between now >> and the release. >> >> I''m not inclined to think that a reduction of 6k is a big enough >> improvement to take the risk at this point. >> >> -George > Patch 1 is literally just textual replacement, cleaning up its current > uses in the codebase. > > Patch 2 applies its use to more functions in the codebase. > > While I agree that it is "No functional change if compilers are > correct", this kind of paranoia could be applied to any and every patch > we consider taking. Anyway, Linux uses it far more than we do at the > moment.But what we''re talking about here isn''t just changing normal code flow or what-not; we''re talking about a fairly niche optimization being applied to functions were it wasn''t applied before. I would be willing to bet that "noreturn" has two or three orders of magnitude less usage and testing than anything in the "normal" C standard; that''s why I consider it to be a higher risk than a patch which changes code in a "normal" way. Now sure, it''s a fairly tiny risk, but the benefit is pretty tiny too. At the moment I don''t see a good reason this can''t wait until 4.5. -George
On 25/11/13 14:46, George Dunlap wrote:> On 11/25/2013 02:02 PM, Andrew Cooper wrote: >> On 25/11/13 11:47, George Dunlap wrote: >>> On Mon, Nov 25, 2013 at 10:25 AM, Andrew Cooper >>> <andrew.cooper3@citrix.com> wrote: >>>> Make better use of noreturn. It allows optimising compilers to >>>> produce more >>>> efficient code. >>>> >>>> George: >>>> I request that this is included for 4.4 - It is no functional >>>> change, but >>>> quite a nice improvement in terms of code size. >>> No functional change *if compilers are correct*. If they''re not, for >>> any reason, it will be very difficult to actually catch between now >>> and the release. >>> >>> I''m not inclined to think that a reduction of 6k is a big enough >>> improvement to take the risk at this point. >>> >>> -George >> Patch 1 is literally just textual replacement, cleaning up its current >> uses in the codebase. >> >> Patch 2 applies its use to more functions in the codebase. >> >> While I agree that it is "No functional change if compilers are >> correct", this kind of paranoia could be applied to any and every patch >> we consider taking. Anyway, Linux uses it far more than we do at the >> moment. > > But what we''re talking about here isn''t just changing normal code flow > or what-not; we''re talking about a fairly niche optimization being > applied to functions were it wasn''t applied before. I would be > willing to bet that "noreturn" has two or three orders of magnitude > less usage and testing than anything in the "normal" C standard; > that''s why I consider it to be a higher risk than a patch which > changes code in a "normal" way. > > Now sure, it''s a fairly tiny risk, but the benefit is pretty tiny > too. At the moment I don''t see a good reason this can''t wait until 4.5. > > -GeorgeOk - I will leave this queued up ready for 4.5 to open for general changes again.