Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1468174 > unrolled thread

[RFC v4 00/22] adapt clockevents frequencies to mono clock

Back to article view | Back to linux.kernel

Contents

  [RFC v4 00/22] adapt clockevents frequencies to mono clock Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 15/22] clockevents: clockevents_program_min_delta(): don't set ->next_event Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 02/22] clocksource: sh_tmu: compute rate before registration again Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 11/22] clockevents: always initialize ->min_delta_ns and ->max_delta_ns Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 18/22] timer_list: print_tickdevice(): calculate ->*_delta_ns dynamically Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 19/22] clockevents: purge ->min_delta_ns and ->max_delta_ns Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 13/22] clockevents: check a programmed delta's bounds in terms of cycles Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200
    [RFC v4 10/22] arch/tile/kernel/time: set ->min_delta_ticks and ->max_delta_ticks Nicolai Stange <nicstange@gmail.com> - 2016-08-23 01:40 +0200

#1468174 — [RFC v4 00/22] adapt clockevents frequencies to mono clock

Previous v3 of this series can be found here:

  http://lkml.kernel.org/r/20160713130017.8202-1-nicstange@gmail.com

First of all, apologies for sending such a huge series.
My intent is to give you a good idea of what would IMO be
necessary to fix the "known issues" I listed in v3.
In case you think that this many changes are in no way justified by
the final goal, namely to make the clockevent core NTP correction
aware and thus avoiding the too short timer interrupts with NOHZ_FULL,
please just drop me a note and I'll shut up.

The "known issues" mentioned in v3 were:

Nicolai Stange <nicstange@gmail.com> writes:
> - The patchset assumes that a clockevent device's ->mult is changed after
>   registration only through calls to clockevents_update_freq().
>   For a handful of non-x86 drivers this isn't the case.

Addressed by

[1/22] clocksource: sh_cmt: compute rate before registration again
[2/22] clocksource: sh_tmu: compute rate before registration again
[3/22] clocksource: em_sti: split clock prepare and enable steps
[4/22] clocksource: em_sti: compute rate before registration
[5/22] clocksource: h8300_timer8: don't reset rate in ->set_state_oneshot()


> - ->min_delta_ns and ->max_delta_ns vs ->mult_mono:
>   In clockevents_program_event(), we had
>    	delta = min(delta, (int64_t) dev->max_delta_ns);
> 	delta = max(delta, (int64_t) dev->min_delta_ns);
> 	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
>   The dev->mult is replaced with the dynamically adjusted dev->mult_mono
>   by this series. That's problematic since as I understand it, especially
>   ->max_delta_ns is a hard limit preventing the clockevent devices counter
>   to be programmed with values larger than its width allows for.
>   If ->mult_mono happens to be only slightly larger than ->mult, the
>   comparison of delta against the ->mult based ->max_delta_ns can pass
>   although the final clc might actually be larger than allowed.
>   I think what we really want to have at this place is a check of clc
>   against the already present ->min_delta_ticks and ->max_delta_ticks.
>
>   The problem with this approach is that many drivers (~40) initialize
>   ->min_delta_ns and ->max_delta_ns (typically with clockevent_delta2ns())
>   but not the ->*_delta_ticks members. My suggestion at this point would
>   be to convert them.

This subseries

[7/22]  many clockevent drivers: set ->min_delta_ticks and ->max_delta_ticks
[8/22]  arch/s390/kernel/time: set ->min_delta_ticks and ->max_delta_ticks
[9/22]  arch/x86/platform/uv/uv_time: set ->min_delta_ticks and
           ->max_delta_ticks
[10/22]  arch/tile/kernel/time: set ->min_delta_ticks and ->max_delta_ticks
[11/22]  clockevents: always initialize ->min_delta_ns and ->max_delta_ns
[12/22]  many clockevent drivers: don't set ->min_delta_ns and ->max_delta_ns

converts all drivers to set ->*_delta_ticks rather than ->*_delta_ns.


>   This makes ->max_delta_ns obsolete right away.
>   ->min_delta_ns is still needed in order to set the ->next_event in
>   clockevents_program_min_delta() though. My claim is that
>   ->min_delta_ns can be safely replaced with 0 in
>   clockevents_program_min_delta()

This subseries

[13/22] clockevents: check a programmed delta's bounds in terms of cycles
[14/22] clockevents: clockevents_program_event(): turn clc into unsigned long
[15/22] clockevents: clockevents_program_min_delta(): don't set ->next_event
[16/22] clockevents: use ->min_delta_ticks_adjusted to program minimum delta
[17/22] clockevents: min delta increment: calculate min_delta_ns from ticks
[18/22] timer_list: print_tickdevice(): calculate ->*_delta_ns dynamically
[19/22] clockevents: purge ->min_delta_ns and ->max_delta_ns

gets rid of the ->*_delta_ns alltogether.


The remaining patches, i.e. [20-22/22], correspond to the former
[1-3/3] from v3.


Tested on next-20160816. Applies to next-20160822.


In case that you want me to proceed with this, I'd really appreciate
some hints on how to send this in non-RFC mode, i.e. with everybody
CC'd. Split this into smaller series and send one after another?
Possibly spread out over several releases?


Changes to v3:
 [20/22] ("clockevents: initial support for mono to raw time conversion")
   - Following J. Stultz' suggestion, I renamed ->mult_mono to
     ->mult_adjusted
   - J. Stultz pointed out that locking is necessary in the
     timekeeping_get_mono_mult() helper. Added.
 [21/22] ("clockevents: make setting of ->mult and ->mult_adjusted atomic")
   - I got the locking wrong here: updates to the bc device should be
     serialized as well. Extend the lock to the bc case.
   - Adapt to the ->mult_mono => ->mult_adjusted renaming.
 [22/22] ("timekeeping: inform clockevents about freq adjustments")
   - Adapt to the ->mult_mono => ->mult_adjusted renaming.

Nicolai Stange (22):
  clocksource: sh_cmt: compute rate before registration again
  clocksource: sh_tmu: compute rate before registration again
  clocksource: em_sti: split clock prepare and enable steps
  clocksource: em_sti: compute rate before registration
  clocksource: h8300_timer8: don't reset rate in ->set_state_oneshot()
  clockevents: make clockevents_config() static
  many clockevent drivers: set ->min_delta_ticks and ->max_delta_ticks
  arch/s390/kernel/time: set ->min_delta_ticks and ->max_delta_ticks
  arch/x86/platform/uv/uv_time: set ->min_delta_ticks and
    ->max_delta_ticks
  arch/tile/kernel/time: set ->min_delta_ticks and ->max_delta_ticks
  clockevents: always initialize ->min_delta_ns and ->max_delta_ns
  many clockevent drivers: don't set ->min_delta_ns and ->max_delta_ns
  clockevents: check a programmed delta's bounds in terms of cycles
  clockevents: clockevents_program_event(): turn clc into unsigned long
  clockevents: clockevents_program_min_delta(): don't set ->next_event
  clockevents: use ->min_delta_ticks_adjusted to program minimum delta
  clockevents: min delta increment: calculate min_delta_ns from ticks
  timer_list: print_tickdevice(): calculate ->*_delta_ns dynamically
  clockevents: purge ->min_delta_ns and ->max_delta_ns
  clockevents: initial support for mono to raw time conversion
  clockevents: make setting of ->mult and ->mult_adjusted atomic
  timekeeping: inform clockevents about freq adjustments

 arch/avr32/kernel/time.c                          |   4 +-
 arch/blackfin/kernel/time-ts.c                    |   8 +-
 arch/c6x/platforms/timer64.c                      |   4 +-
 arch/hexagon/kernel/time.c                        |   4 +-
 arch/m68k/coldfire/pit.c                          |   6 +-
 arch/microblaze/kernel/timer.c                    |   6 +-
 arch/mips/alchemy/common/time.c                   |   4 +-
 arch/mips/jz4740/time.c                           |   4 +-
 arch/mips/kernel/cevt-bcm1480.c                   |   4 +-
 arch/mips/kernel/cevt-ds1287.c                    |   4 +-
 arch/mips/kernel/cevt-gt641xx.c                   |   4 +-
 arch/mips/kernel/cevt-sb1250.c                    |   4 +-
 arch/mips/kernel/cevt-txx9.c                      |   5 +-
 arch/mips/loongson32/common/time.c                |   4 +-
 arch/mips/loongson64/common/cs5536/cs5536_mfgpt.c |   4 +-
 arch/mips/loongson64/loongson-3/hpet.c            |   4 +-
 arch/mips/ralink/cevt-rt3352.c                    |   4 +-
 arch/mips/sgi-ip27/ip27-timer.c                   |   4 +-
 arch/mn10300/kernel/cevt-mn10300.c                |   4 +-
 arch/powerpc/kernel/time.c                        |   6 +-
 arch/s390/kernel/time.c                           |   4 +-
 arch/score/kernel/time.c                          |   6 +-
 arch/sparc/kernel/time_32.c                       |   4 +-
 arch/sparc/kernel/time_64.c                       |   6 +-
 arch/tile/kernel/time.c                           |   4 +-
 arch/um/kernel/time.c                             |   4 +-
 arch/unicore32/kernel/time.c                      |   6 +-
 arch/x86/kernel/apic/apic.c                       |  12 +-
 arch/x86/lguest/boot.c                            |   4 +-
 arch/x86/platform/uv/uv_time.c                    |   6 +-
 arch/x86/xen/time.c                               |   8 +-
 drivers/clocksource/dw_apb_timer.c                |   5 +-
 drivers/clocksource/em_sti.c                      |  49 ++++---
 drivers/clocksource/h8300_timer8.c                |   8 --
 drivers/clocksource/metag_generic.c               |   4 +-
 drivers/clocksource/numachip.c                    |   4 +-
 drivers/clocksource/sh_cmt.c                      |  50 +++----
 drivers/clocksource/sh_tmu.c                      |  26 ++--
 drivers/clocksource/timer-atlas7.c                |   4 +-
 include/linux/clockchips.h                        |  17 ++-
 kernel/time/clockevents.c                         | 153 ++++++++++++++++------
 kernel/time/tick-broadcast-hrtimer.c              |   2 -
 kernel/time/tick-internal.h                       |   2 +
 kernel/time/timekeeping.c                         |  17 +++
 kernel/time/timer_list.c                          |  10 +-
 45 files changed, 295 insertions(+), 211 deletions(-)

-- 
2.9.2

[toc] | [next] | [standalone]

#1468176 — [RFC v4 15/22] clockevents: clockevents_program_min_delta(): don't set ->next_event

Currently, clockevents_program_min_delta() sets a clockevent device's
->next_event to the point in time where the minimum delta would actually
expire:

  delta = dev->min_delta_ns;
  dev->next_event = ktime_add_ns(ktime_get(), delta);

For your reference, this is so since the initial advent of
clockevents_program_min_delta() with
  commit d1748302f70b ("clockevents: Make minimum delay adjustments
                        configurable").

clockevents_program_min_delta() is called from clockevents_program_event()
only. More specifically, it is called if the latter's force argument is set
and, neglecting the case of device programming failure for the moment, if
the requested expiry is in the past.

On the contrary, if the expiry requested from clockevents_program_event()
is in the future, but less than ->min_delta_ns behind, then
- ->next_event gets set to that expiry verbatim
- but the clockevent device gets silently programmed to fire after
  ->min_delta_ns only.

Thus, in the extreme cases of expires == ktime_get() and
expires == ktime_get() + 1, the respective values of ->next_event would
differ by ->min_delta_ns while the clockevent device would actually get
programmed to fire at (almost) the same times (with force being set,
of course).

While this discontinuity of ->next_event at expires == ktime_get() is not
a problem by itself, the mere use of ->min_delta_ns in the event
programming path hinders upcoming changes making the clockevent core
NTP correction aware: both, ->mult and ->min_delta_ns would need to get
updated as well as consumed atomically and we'd rather like to avoid any
locking here.

Thus, let clockevents_program_event() unconditionally set ->next_event to
the expiry time actually requested by its caller, i.e. don't set
->next_event from clockevents_program_min_delta().

A few notes on why this change is safe with the current consumers of
->next_event:
1.
Note that a clockevents_program_event() with a requested expiry in the
past and force being set basically means: "fire ASAP". Now, consider this
so programmed event getting handed once again to
clockevents_program_event(), i.e. that a

  clockevents_program_event(dev, dev->next_event, false)

as in __clockevents_update_freq() is done.
With this change applied, clockevents_program_event() would now properly
detect the expiry being in the past and, due to the force argument being
unset, wouldn't actually do anything.
Before this change OTOH, there would be the (very unlikely) possibility
that the requested event is still somewhere in the future and
clockevents_program_event() would silently delay the event expiration by
another ->min_delta_ns.

2.
The periodic tick handlers on oneshot-only devices use ->next_event
to calculate the followup expiry time.
tick_handle_periodic() spins on reprogramming the clockevent device
until some expiry in the future has been reached:

  ktime_t next = dev->next_event;
  ...
  for(;;) {
    next = ktime_add(next, tick_period);
    if (!clockevents_program_event(dev, next, false))
      return;
    ...
  }

Thus, tick_handle_periodic() isn't affected by this change.
For tick_handle_periodic_broadcast(), the situation is different since

  commit 2951d5c031a3 ("tick: broadcast: Prevent livelock from event
                        handler")

though: a loop similar to the one from tick_handle_periodic() above got
replaced by a single

  ktime_t next = ktime_add(dev->next_event, tick_period);
  clockevents_program_event(dev, next, true);

In the case that dev->next_event + tick_period happens to be less than
ktime_get() + ->min_delta_ns, without this change applied, ->next_event
would get recovered to some point in the future after a single
tick_handle_periodic_broadcast() event.
On the contrary, with this patch applied, it could potentially take some
number of tick_handle_periodic_broadcast() events, each separated by
->min_delta_ns only, until ->next_event is able to catch up with the
current ktime_get(). However, if this turns out to become a problem,
the reprogramming loop in tick_handle_periodic_broadcast() can probably
be restored easily.

3.
In kernel/time/tick-broadcast.c, the broadcast receiving clockevent
devices' ->next_event is read multiple times in order to determine who's
next or who must be pinged. These uses all continue to work. Moreover,
clockevent devices getting programmed to something less than
ktime_get() + ->min_delta_ns
might not be the best candidates for a transition into C3 anyway.

4.
Finally, a "sleep length" is calculated at the very end of
tick_nohz_stop_sched_tick() as follows:

  ts->sleep_length = ktime_sub(dev->next_event, now);

AFAICS, this can happen to be negative w/o this change applied already: in
NOHZ_MODE_HIGHRES mode there can be some overdue hrtimers whose removal is
blocked because tick_nohz_stop_sched_tick() gets called with interrupts
disabled. Unfortunately, the only user, the menu cpuidle governor,
can't cope with negative sleep lengths as it casts the return value
of the tick_nohz_get_sleep_length() getter to an unsigned int.
This change can very well make things worse here. A followup patch
will force this ->sleep_length to >= 0.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 kernel/time/clockevents.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 8fddb67..f0a80fc 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -251,7 +251,6 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 
 	for (i = 0;;) {
 		delta = dev->min_delta_ns;
-		dev->next_event = ktime_add_ns(ktime_get(), delta);
 
 		if (clockevent_state_shutdown(dev))
 			return 0;
@@ -288,7 +287,6 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 	int64_t delta;
 
 	delta = dev->min_delta_ns;
-	dev->next_event = ktime_add_ns(ktime_get(), delta);
 
 	if (clockevent_state_shutdown(dev))
 		return 0;
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468178 — [RFC v4 02/22] clocksource: sh_tmu: compute rate before registration again

With the upcoming NTP correction related rate adjustments to be implemented
in the clockevents core, the latter needs to get informed about every rate
change of a clockevent device made after its registration.

Currently, sh_tmu violates this requirement in that it registers its
clockevent device with a dummy rate and sets its final rate through
clockevents_config() called from its ->set_state_oneshot() and
->set_state_periodic() functions respectively.

This patch moves the setting of the clockevent device's rate to its
registration.

Note that there has been some back and forth regarding this question with
respect to the clocksource also provided by this driver:
  commit 66f49121ffa4 ("clocksource: sh_tmu: compute mult and shift before
                        registration")
moves the rate determination from the clocksource's ->enable() function to
before its registration. OTOH, the later
  commit 0aeac458d9eb ("clocksource: sh_tmu: __clocksource_updatefreq_hz()
                        update")
basically reverts this, saying
  "Without this patch the old code uses clocksource_register() together
   with a hack that assumes a never changing clock rate."

However, I checked all current sh_tmu users in arch/sh as well as in
arch/arm/mach-shmobile carefully and right now, none of them changes any
rate in any clock tree relevant to sh_tmu after their respective
time_init(). Since all sh_tmu instances are created after time_init(), none
of them should ever observe any clock rate changes.

What's more, both, a clocksource as well as a clockevent device, can
immediately get selected for use at their registration and thus, enabled
at this point already. So it's probably safer to assume a "never changing
clock rate" here.

- Move the struct sh_tmu_channel's ->rate member to struct sh_tmu_device:
  it's a property of the underlying clock which is in turn specific to
  the sh_tmu_device.
- Determine the ->rate value in sh_tmu_setup() at device probing rather
  than at first usage.
- Set the clockevent device's rate at its registration.
- Although not strictly necessary for the upcoming clockevent core changes,
  set the clocksource's rate at its registration for consistency.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---

Notes:
    For a detailed analysis of the current sh_tmu users, please see
    https://nicst.de/ced-clk-rate-change-analysis/sh_tmu-cgitted.html
    
    Compile-only tested on ARCH=sh and ARCH=arm.

 drivers/clocksource/sh_tmu.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index 469e776..8200042 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -46,7 +46,6 @@ struct sh_tmu_channel {
 	void __iomem *base;
 	int irq;
 
-	unsigned long rate;
 	unsigned long periodic;
 	struct clock_event_device ced;
 	struct clocksource cs;
@@ -59,6 +58,7 @@ struct sh_tmu_device {
 
 	void __iomem *mapbase;
 	struct clk *clk;
+	unsigned long rate;
 
 	enum sh_tmu_model model;
 
@@ -165,7 +165,6 @@ static int __sh_tmu_enable(struct sh_tmu_channel *ch)
 	sh_tmu_write(ch, TCNT, 0xffffffff);
 
 	/* configure channel to parent clock / 4, irq off */
-	ch->rate = clk_get_rate(ch->tmu->clk) / 4;
 	sh_tmu_write(ch, TCR, TCR_TPSC_CLK4);
 
 	/* enable channel */
@@ -271,10 +270,8 @@ static int sh_tmu_clocksource_enable(struct clocksource *cs)
 		return 0;
 
 	ret = sh_tmu_enable(ch);
-	if (!ret) {
-		__clocksource_update_freq_hz(cs, ch->rate);
+	if (!ret)
 		ch->cs_enabled = true;
-	}
 
 	return ret;
 }
@@ -334,8 +331,7 @@ static int sh_tmu_register_clocksource(struct sh_tmu_channel *ch,
 	dev_info(&ch->tmu->pdev->dev, "ch%u: used as clock source\n",
 		 ch->index);
 
-	/* Register with dummy 1 Hz value, gets updated in ->enable() */
-	clocksource_register_hz(cs, 1);
+	clocksource_register_hz(cs, ch->tmu->rate);
 	return 0;
 }
 
@@ -346,14 +342,10 @@ static struct sh_tmu_channel *ced_to_sh_tmu(struct clock_event_device *ced)
 
 static void sh_tmu_clock_event_start(struct sh_tmu_channel *ch, int periodic)
 {
-	struct clock_event_device *ced = &ch->ced;
-
 	sh_tmu_enable(ch);
 
-	clockevents_config(ced, ch->rate);
-
 	if (periodic) {
-		ch->periodic = (ch->rate + HZ/2) / HZ;
+		ch->periodic = (ch->tmu->rate + HZ/2) / HZ;
 		sh_tmu_set_next(ch, ch->periodic, 1);
 	}
 }
@@ -435,7 +427,7 @@ static void sh_tmu_register_clockevent(struct sh_tmu_channel *ch,
 	dev_info(&ch->tmu->pdev->dev, "ch%u: used for clock events\n",
 		 ch->index);
 
-	clockevents_config_and_register(ced, 1, 0x300, 0xffffffff);
+	clockevents_config_and_register(ced, ch->tmu->rate, 0x300, 0xffffffff);
 
 	ret = request_irq(ch->irq, sh_tmu_interrupt,
 			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
@@ -561,6 +553,14 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
 	if (ret < 0)
 		goto err_clk_put;
 
+	/* Determine clock rate. */
+	ret = clk_enable(tmu->clk);
+	if (ret < 0)
+		goto err_clk_unprepare;
+
+	tmu->rate = clk_get_rate(tmu->clk) / 4;
+	clk_disable(tmu->clk);
+
 	/* Map the memory resource. */
 	ret = sh_tmu_map_memory(tmu);
 	if (ret < 0) {
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468181 — [RFC v4 11/22] clockevents: always initialize ->min_delta_ns and ->max_delta_ns

Now that all clockevent drivers set ->min_delta_ticks and ->max_delta_ticks
independently of whether they use clockevents_config*() or not, the
clockevent core can calculate ->min_delta_ns and ->max_delta_ns from these
unconditionally.

The goal is to prepare the clockevent core for introducing NTP rate
correction awareness: as the clockevent devices' rates will get adjusted,
the ->*_delta_ns won't stay fixed but need to get changed as well.

Thus, make the clockevent core calculate the ->*_delta_ns values from
the invariant ->_delta_ticks ones at device registration.

In order to facilitate this, move the corresponding code from
clockevents_config() into its own helper function,
__clockevents_update_bounds() and invoke this where needed, in
particular from clockevents_register_device().

Note that there is a side effect affecting those drivers
- not using clockevents_config*()
- and manually initializing their ->min_delta_ns to values less than 1us.
In order to avoid "pointless noise", the clockevent core always sets
->min_delta_ns to values >= 1us, and thus, those drivers' ->min_delta_ns
is now increased. I think that this side effect is desired though.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 kernel/time/clockevents.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index e73ac7f..f352f54 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -442,6 +442,19 @@ int clockevents_unbind_device(struct clock_event_device *ced, int cpu)
 }
 EXPORT_SYMBOL_GPL(clockevents_unbind_device);
 
+static void __clockevents_update_bounds(struct clock_event_device *dev)
+{
+	if (!(dev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return;
+
+	/*
+	 * cev_delta2ns() never returns values less than 1us and thus,
+	 * we'll never program any ced with anything less.
+	 */
+	dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
+	dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
+}
+
 /**
  * clockevents_register_device - register a clock event device
  * @dev:	device to register
@@ -458,6 +471,8 @@ void clockevents_register_device(struct clock_event_device *dev)
 		dev->cpumask = cpumask_of(smp_processor_id());
 	}
 
+	__clockevents_update_bounds(dev);
+
 	raw_spin_lock_irqsave(&clockevents_lock, flags);
 
 	list_add(&dev->list, &clockevent_devices);
@@ -488,8 +503,6 @@ static void clockevents_config(struct clock_event_device *dev, u32 freq)
 		sec = 600;
 
 	clockevents_calc_mult_shift(dev, freq, sec);
-	dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
-	dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
 }
 
 /**
@@ -515,6 +528,7 @@ EXPORT_SYMBOL_GPL(clockevents_config_and_register);
 int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 {
 	clockevents_config(dev, freq);
+	__clockevents_update_bounds(dev);
 
 	if (clockevent_state_oneshot(dev))
 		return clockevents_program_event(dev, dev->next_event, false);
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468182 — [RFC v4 18/22] timer_list: print_tickdevice(): calculate ->*_delta_ns dynamically

print_tickdevice(), assembling the per-tick device sections in
/proc/timer_list, is the last user of struct clock_event_device's
->min_delta_ns and ->max_delta_ns members.

In order to make these fully obsolete while retaining userspace ABI,
calculate the displayed values of 'min_delta_ns' and 'max_delta_ns'
on the fly from ->min_delta_ticks_adjusted and ->max_delta_ticks
respectively.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 kernel/time/timer_list.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index ba7d8b2..5fba4f3 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -206,6 +206,10 @@ static void
 print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 {
 	struct clock_event_device *dev = td->evtdev;
+	unsigned long long min_delta_ns =
+		clockevent_delta2ns(dev->min_delta_ticks_adjusted, dev);
+	unsigned long long max_delta_ns =
+		clockevent_delta2ns(dev->max_delta_ticks, dev);
 
 	SEQ_printf(m, "Tick Device: mode:     %d\n", td->mode);
 	if (cpu < 0)
@@ -219,10 +223,8 @@ print_tickdevice(struct seq_file *m, struct tick_device *td, int cpu)
 		return;
 	}
 	SEQ_printf(m, "%s\n", dev->name);
-	SEQ_printf(m, " max_delta_ns:   %llu\n",
-		   (unsigned long long) dev->max_delta_ns);
-	SEQ_printf(m, " min_delta_ns:   %llu\n",
-		   (unsigned long long) dev->min_delta_ns);
+	SEQ_printf(m, " max_delta_ns:   %llu\n", max_delta_ns);
+	SEQ_printf(m, " min_delta_ns:   %llu\n", min_delta_ns);
 	SEQ_printf(m, " mult:           %u\n", dev->mult);
 	SEQ_printf(m, " shift:          %u\n", dev->shift);
 	SEQ_printf(m, " mode:           %d\n", clockevent_get_state(dev));
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468185 — [RFC v4 19/22] clockevents: purge ->min_delta_ns and ->max_delta_ns

The struct clock_event_device ->min_delta_ns and ->max_delta_ns aren't
used anymore.

Purge them.

In __clockevents_update_bounds(), shortcut the
  ->min_delta_ticks => ->min_delta_ns => ->min_delta_ticks_adjusted
calculation detour -- it had been made solely for the purpose of ensuring
that ->min_delta_ticks_adjusted corresponds to something >= 1us.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 include/linux/clockchips.h |  4 ----
 kernel/time/clockevents.c  | 13 +++++--------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 8578e24..4b71882 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -87,8 +87,6 @@ enum clock_event_state {
  * @tick_resume:	resume clkevt device
  * @broadcast:		function to broadcast events
  * @min_delta_ticks:	minimum delta value in ticks stored for reconfiguration
- * @max_delta_ns:	maximum delta value in ns
- * @min_delta_ns:	minimum delta value in ns
  * @name:		ptr to clock event name
  * @rating:		variable to rate clock event devices
  * @irq:		IRQ number (only for non CPU local devices)
@@ -120,8 +118,6 @@ struct clock_event_device {
 	void			(*suspend)(struct clock_event_device *);
 	void			(*resume)(struct clock_event_device *);
 	unsigned long		min_delta_ticks;
-	u64			max_delta_ns;
-	u64			min_delta_ns;
 
 	const char		*name;
 	int			rating;
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index bb3b98c..c59d6f2 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -444,15 +444,12 @@ static void __clockevents_update_bounds(struct clock_event_device *dev)
 		return;
 
 	/*
-	 * cev_delta2ns() never returns values less than 1us and thus,
-	 * we'll never program any ced with anything less.
+	 * Enforce ->min_delta_ticks_adjusted to correspond to a value
+	 * >= 1us.
 	 */
-	dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
-	dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
-	dev->min_delta_ticks_adjusted = (unsigned long)((dev->min_delta_ns *
-						dev->mult) >> dev->shift);
-	dev->min_delta_ticks_adjusted = max(dev->min_delta_ticks_adjusted,
-						dev->min_delta_ticks);
+	dev->min_delta_ticks_adjusted =
+		max(dev->min_delta_ticks,
+			(unsigned long)((1000ULL * dev->mult) >> dev->shift));
 }
 
 /**
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468187 — [RFC v4 13/22] clockevents: check a programmed delta's bounds in terms of cycles

Currently, clockevents_program_event()
- compares a given delta against ->min_delta_ns and ->max_delta_ns
- and transforms the given delta value from ns to device cycles via ->mult.

Future changes introducing NTP time awareness into the clockevents core
would possibly need to update ->mult, ->min_delta_ns and ->max_delta_ns
from a different CPU than clockevents_program_event() runs on: the
->*_delta_ns values depend on ->mult. In order to guarantee atomicity
between these updates, a seqlock would have to be introduced and acquired
for reading in clockevents_program_event().

In order to avoid this, do the bounds checking in terms of the always
invariant ->min_delta_ticks and ->max_delta_ticks values.

The transition from ->max_delta_ns to ->max_delta_ticks is straight
forward since they are 1:1.

For ->min_delta_ns, the situation is slightly different as it can get
larger than its initial value derived from ->min_delta_ticks for two
reason:
- ->min_delta_ns is enforced to be larger than 1us at initialization
- and it can grow over time with CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST,
  c.f. clockevents_increase_min_delta().

Introduce ->min_delta_ticks_adjusted always resembling the current value
of ->min_delta_ns. The original ->min_delta_ticks must be kept for
reconfiguration.

The invariant ->min_delta_ticks <= ->min_delta_ticks_adjusted will always
be guaranteed to hold. This will allow for non-atomic updates of ->mult
and ->min_delta_ticks_adjusted -- as long as we stay within a device's
allowed bounds, we don't care for small deviations.

Make clockevents_program_event() use ->min_delta_ticks_adjusted and
->max_delta_ticks for the bounds enforcement on the delta value.

Finally, slightly reorder the members of struct clock_event_device in order
to keep the now often used ones close together.
Also, since ->max_delta_ticks is being actively used now, remove the
assertion that it is "stored for reconfiguration" from its docstring.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 include/linux/clockchips.h | 14 ++++++++------
 kernel/time/clockevents.c  | 15 ++++++++++++---
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index a116926..8578e24 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -73,8 +73,8 @@ enum clock_event_state {
  * @set_next_event:	set next event function using a clocksource delta
  * @set_next_ktime:	set next event function using a direct ktime value
  * @next_event:		local storage for the next event in oneshot mode
- * @max_delta_ns:	maximum delta value in ns
- * @min_delta_ns:	minimum delta value in ns
+ * @max_delta_ticks:	maximum delta value in ticks
+ * @min_delta_ticks_adjusted:	minimum delta value, increased as needed
  * @mult:		nanosecond to cycles multiplier
  * @shift:		nanoseconds to cycles divisor (power of two)
  * @state_use_accessors:current state of the device, assigned by the core code
@@ -87,7 +87,8 @@ enum clock_event_state {
  * @tick_resume:	resume clkevt device
  * @broadcast:		function to broadcast events
  * @min_delta_ticks:	minimum delta value in ticks stored for reconfiguration
- * @max_delta_ticks:	maximum delta value in ticks stored for reconfiguration
+ * @max_delta_ns:	maximum delta value in ns
+ * @min_delta_ns:	minimum delta value in ns
  * @name:		ptr to clock event name
  * @rating:		variable to rate clock event devices
  * @irq:		IRQ number (only for non CPU local devices)
@@ -101,8 +102,8 @@ struct clock_event_device {
 	int			(*set_next_event)(unsigned long evt, struct clock_event_device *);
 	int			(*set_next_ktime)(ktime_t expires, struct clock_event_device *);
 	ktime_t			next_event;
-	u64			max_delta_ns;
-	u64			min_delta_ns;
+	unsigned long		max_delta_ticks;
+	unsigned long		min_delta_ticks_adjusted;
 	u32			mult;
 	u32			shift;
 	enum clock_event_state	state_use_accessors;
@@ -119,7 +120,8 @@ struct clock_event_device {
 	void			(*suspend)(struct clock_event_device *);
 	void			(*resume)(struct clock_event_device *);
 	unsigned long		min_delta_ticks;
-	unsigned long		max_delta_ticks;
+	u64			max_delta_ns;
+	u64			min_delta_ns;
 
 	const char		*name;
 	int			rating;
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index f352f54..7832050 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -225,6 +225,11 @@ static int clockevents_increase_min_delta(struct clock_event_device *dev)
 	if (dev->min_delta_ns > MIN_DELTA_LIMIT)
 		dev->min_delta_ns = MIN_DELTA_LIMIT;
 
+	dev->min_delta_ticks_adjusted = (unsigned long)((dev->min_delta_ns *
+						dev->mult) >> dev->shift);
+	dev->min_delta_ticks_adjusted = max(dev->min_delta_ticks_adjusted,
+						dev->min_delta_ticks);
+
 	printk_deferred(KERN_WARNING
 			"CE: %s increased min_delta_ns to %llu nsec\n",
 			dev->name ? dev->name : "?",
@@ -332,10 +337,10 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
 	if (delta <= 0)
 		return force ? clockevents_program_min_delta(dev) : -ETIME;
 
-	delta = min(delta, (int64_t) dev->max_delta_ns);
-	delta = max(delta, (int64_t) dev->min_delta_ns);
-
 	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
+	clc = min_t(unsigned long, clc, dev->max_delta_ticks);
+	clc = max_t(unsigned long, clc, dev->min_delta_ticks_adjusted);
+
 	rc = dev->set_next_event((unsigned long) clc, dev);
 
 	return (rc && force) ? clockevents_program_min_delta(dev) : rc;
@@ -453,6 +458,10 @@ static void __clockevents_update_bounds(struct clock_event_device *dev)
 	 */
 	dev->min_delta_ns = cev_delta2ns(dev->min_delta_ticks, dev, false);
 	dev->max_delta_ns = cev_delta2ns(dev->max_delta_ticks, dev, true);
+	dev->min_delta_ticks_adjusted = (unsigned long)((dev->min_delta_ns *
+						dev->mult) >> dev->shift);
+	dev->min_delta_ticks_adjusted = max(dev->min_delta_ticks_adjusted,
+						dev->min_delta_ticks);
 }
 
 /**
-- 
2.9.2

[toc] | [prev] | [next] | [standalone]

#1468188 — [RFC v4 10/22] arch/tile/kernel/time: set ->min_delta_ticks and ->max_delta_ticks

With the yet to come introduction of NTP correction awareness to the
clockevent core, drivers should report their valid ranges in units of
cycles to the latter.

Currently, the tile's timer clockevent device is initialized as follows:

  evt->max_delta_ns = clockevent_delta2ns(MAX_TICK, evt);

and

  .min_delta_ns = 1000,

The first one translates to a ->max_delta_ticks value of MAX_TICK.
For the latter, note that the clockevent core will superimpose a
minimum of 1us by itself -- setting ->min_delta_ticks to 1 is safe here.

Initialize ->min_delta_ticks and ->max_delta_ticks with these values.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 arch/tile/kernel/time.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c
index 178989e..c2fd280 100644
--- a/arch/tile/kernel/time.c
+++ b/arch/tile/kernel/time.c
@@ -154,6 +154,7 @@ static DEFINE_PER_CPU(struct clock_event_device, tile_timer) = {
 	.name = "tile timer",
 	.features = CLOCK_EVT_FEAT_ONESHOT,
 	.min_delta_ns = 1000,
+	.min_delta_ticks = 1,
 	.rating = 100,
 	.irq = -1,
 	.set_next_event = tile_timer_set_next_event,
@@ -169,6 +170,7 @@ void setup_tile_timer(void)
 	/* Fill in fields that are speed-specific. */
 	clockevents_calc_mult_shift(evt, cycles_per_sec, TILE_MINSEC);
 	evt->max_delta_ns = clockevent_delta2ns(MAX_TICK, evt);
+	evt->max_delta_ticks = MAX_TICK;
 
 	/* Mark as being for this cpu only. */
 	evt->cpumask = cpumask_of(smp_processor_id());
-- 
2.9.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web