linux-kernel - Re: [PATCH V2] tick-broadcast: Register for hrtimer based broadcast as the fallback broadcast mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141208104828.GA21680@leverpostej>
Date:	Mon, 8 Dec 2014 10:48:29 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
Cc:	"tglx@...utronix.de" <tglx@...utronix.de>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	"peterz@...radead.org" <peterz@...radead.org>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
	Will Deacon <Will.Deacon@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"fweisbec@...il.com" <fweisbec@...il.com>,
	"jingchang.lu@...escale.com" <jingchang.lu@...escale.com>,
	"svaidy@...ux.vnet.ibm.com" <svaidy@...ux.vnet.ibm.com>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH V2] tick-broadcast: Register for hrtimer based broadcast
 as the fallback broadcast mode

Hi Preeti,

On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote:
> Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a
> hrtimer based broadcast mode for those platforms in which local timers stop
> when CPUs enter deep idle states. The commit expected the platforms to
> register for this mode explicitly when they lacked a better external device
> to wake up CPUs in deep idle. Given that more platforms are beginning to use
> this mode, we can avoid the call to set it up on every platform that requires
> it, by registering for the hrtimer based broadcast mode in the core code if
> no better broadcast device is available.
> 
> This commit also helps detect cases where the platform fails to register for
> a broadcast device but invokes the help of one when entering deep idle states.
> Currently we do not handle this situation at all and call the broadcast clock
> device without checking for its existence. This patch will handle such buggy
> cases properly.
> 
> Signed-off-by: Preeti U Murthy <preeti@...ux.vnet.ibm.com>

I've just given this a go on an arm64 platform (Juno) without any
system-wide clock_event_devices registered, and everything works well
with CPUs entering and exiting idle states where the cpu-local timers
lose state. So:

Tested-by: Mark Rutland <mark.rutland@....com>

One minor thing I noticed when testing was that
/sys/devices/system/clockevents/broadcast/name contained "(null)",
because we never set the name field on the clock_event_device. It's
always been that way, but now might be a good time to change that to
something like "broadcast_hrtimer".

[...]

> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 2e4cb67..91754b0 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void);
>  #endif
>  
>  #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
> -extern void tick_setup_hrtimer_broadcast(void);
> +extern int __init tick_setup_hrtimer_broadcast(void);
>  extern int tick_check_broadcast_expired(void);
>  #else
>  static inline int tick_check_broadcast_expired(void) { return 0; }
> -static inline void tick_setup_hrtimer_broadcast(void) {};
> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }
>  #endif
>  
>  #ifdef CONFIG_GENERIC_CLOCKEVENTS
> @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {}
>  
>  static inline int clockevents_notify(unsigned long reason, void *arg) { return 0; }
>  static inline int tick_check_broadcast_expired(void) { return 0; }
> -static inline void tick_setup_hrtimer_broadcast(void) {};
> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }

With the initcall moved to the driver we have no external users of
tick_setup_hrtimer_broadcast, so I think we can remove the prototype
entirely from clockchips.h...

>  #endif
>  
> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> index eb682d5..5c35995 100644
> --- a/kernel/time/tick-broadcast-hrtimer.c
> +++ b/kernel/time/tick-broadcast-hrtimer.c
> @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
>  	return HRTIMER_RESTART;
>  }
>  
> -void tick_setup_hrtimer_broadcast(void)
> +int __init tick_setup_hrtimer_broadcast(void)

...and make it static here.

>  {
>  	hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>  	bctimer.function = bc_handler;
>  	clockevents_register_device(&ce_broadcast_hrtimer);
> +	return 0;
>  }
> +early_initcall(tick_setup_hrtimer_broadcast);

Otherwise this looks good to me, thanks for putting this together!

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/