lkml.org 
[lkml]   [2006]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [patch 15/23] high-res timers: core
    On Fri, 29 Sep 2006 23:58:34 -0000
    Thomas Gleixner <tglx@linutronix.de> wrote:

    > From: Thomas Gleixner <tglx@linutronix.de>
    >
    > add the core bits of high-res timers support.
    >
    > the design makes use of the existing hrtimers subsystem which manages a
    > per-CPU and per-clock tree of timers, and the clockevents framework, which
    > provides a standard API to request programmable clock events from. The
    > core code does not have to know about the clock details - it makes use
    > of clockevents_set_next_event().
    >
    > the code also provides dyntick functionality: it is implemented via a
    > per-cpu sched_tick hrtimer that is set to HZ frequency, but which is
    > reprogrammed to a longer timeout before going idle, and reprogrammed to
    > HZ again once the CPU goes busy again. (If an non-timer IRQ hits the
    > idle task then it will process jiffies before calling the IRQ code.)
    >
    > the impact to non-high-res architectures is intended to be minimal.
    >
    > ...
    >
    > @@ -108,17 +134,53 @@ struct hrtimer_cpu_base {
    > spinlock_t lock;
    > struct lock_class_key lock_key;
    > struct hrtimer_clock_base clock_base[HRTIMER_MAX_CLOCK_BASES];
    > +#ifdef CONFIG_HIGH_RES_TIMERS
    > + ktime_t expires_next;
    > + int hres_active;
    > + unsigned long check_clocks;
    > + struct list_head cb_pending;
    > + struct hrtimer sched_timer;
    > + struct pt_regs *sched_regs;
    > + unsigned long events;
    > +#endif

    You forgot to update the kerneldoc for this struct.

    Does `events' needs to be long?

    <looks>

    oh, it's a scalar this time ;)

    > +#ifdef CONFIG_HIGH_RES_TIMERS
    > +
    > +extern void hrtimer_clock_notify(void);
    > +extern void clock_was_set(void);
    > +extern void hrtimer_interrupt(struct pt_regs *regs);
    > +
    > +# define hrtimer_cb_get_time(t) (t)->base->get_time()
    > +# define hrtimer_hres_active (__get_cpu_var(hrtimer_bases).hres_active)

    These two could be inline functions?

    That might cause include file ordering problems I guess.

    > +/*
    > + * The resolution of the clocks. The resolution value is returned in
    > + * the clock_getres() system call to give application programmers an
    > + * idea of the (in)accuracy of timers. Timer values are rounded up to
    > + * this resolution values.
    > + */
    > +# define KTIME_HIGH_RES (ktime_t) { .tv64 = CONFIG_HIGH_RES_RESOLUTION }
    > +# define KTIME_MONOTONIC_RES KTIME_HIGH_RES
    > +
    > +#else
    > +
    > +# define KTIME_MONOTONIC_RES KTIME_LOW_RES
    > +
    > /*
    > * clock_was_set() is a NOP for non- high-resolution systems. The
    > * time-sorted order guarantees that a timer does not expire early and
    > * is expired in the next softirq when the clock was advanced.
    > */
    > -#define clock_was_set() do { } while (0)
    > -#define hrtimer_clock_notify() do { } while (0)
    > -extern ktime_t ktime_get(void);
    > -extern ktime_t ktime_get_real(void);
    > +# define clock_was_set() do { } while (0)
    > +# define hrtimer_clock_notify() do { } while (0)

    these could be inlines.

    > +# define hrtimer_cb_get_time(t) (t)->base->softirq_time

    Does this need parenthesisation? Probably it's OK.. An inline function
    would be nicer.

    > +# define hrtimer_hres_active 0

    Perhaps this would be better if it was presented as a function.

    > +/* High resolution timer related functions */
    > +#ifdef CONFIG_HIGH_RES_TIMERS
    > +
    > +static ktime_t last_jiffies_update;

    What's this do?

    > +/*
    > + * Reprogramm the event source with checking both queues for the

    "Reprogramme" ;)
    > + * next event
    > + * Called with interrupts disabled and base->lock held
    > + */
    > +static void hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base)
    > +{
    > + int i;
    > + struct hrtimer_clock_base *base = cpu_base->clock_base;
    > + ktime_t expires;
    > +
    > + cpu_base->expires_next.tv64 = KTIME_MAX;
    > +
    > + for (i = HRTIMER_MAX_CLOCK_BASES; i ; i--, base++) {

    Downcounting loops hurt my brain. Does it actually generate better code?

    > + struct hrtimer *timer;
    > +
    > + if (!base->first)
    > + continue;
    > + timer = rb_entry(base->first, struct hrtimer, node);
    > + expires = ktime_sub(timer->expires, base->offset);
    > + if (expires.tv64 < cpu_base->expires_next.tv64)
    > + cpu_base->expires_next = expires;
    > + }
    > +
    > + if (cpu_base->expires_next.tv64 != KTIME_MAX)
    > + clockevents_set_next_event(cpu_base->expires_next, 1);
    > +}
    > +
    > +/*
    > + * Shared reprogramming for clock_realtime and clock_monotonic
    > + *
    > + * When a new expires first timer is enqueued, we have

    That sentence might need work.

    > +/*
    > + * Retrigger next event is called after clock was set
    > + */
    > +static void retrigger_next_event(void *arg)
    > +{
    > + struct hrtimer_cpu_base *base;
    > + struct timespec realtime_offset;
    > + unsigned long flags, seq;
    > +
    > + do {
    > + seq = read_seqbegin(&xtime_lock);
    > + set_normalized_timespec(&realtime_offset,
    > + -wall_to_monotonic.tv_sec,
    > + -wall_to_monotonic.tv_nsec);
    > + } while (read_seqretry(&xtime_lock, seq));
    > +
    > + base = &per_cpu(hrtimer_bases, smp_processor_id());
    > +
    > + /* Adjust CLOCK_REALTIME offset */
    > + spin_lock_irqsave(&base->lock, flags);
    > + base->clock_base[CLOCK_REALTIME].offset =
    > + timespec_to_ktime(realtime_offset);
    > +
    > + hrtimer_force_reprogram(base);
    > + spin_unlock_irqrestore(&base->lock, flags);
    > +}
    > +
    > +/*
    > + * Clock realtime was set
    > + *
    > + * Change the offset of the realtime clock vs. the monotonic
    > + * clock.
    > + *
    > + * We might have to reprogram the high resolution timer interrupt. On
    > + * SMP we call the architecture specific code to retrigger _all_ high
    > + * resolution timer interrupts. On UP we just disable interrupts and
    > + * call the high resolution interrupt code.
    > + */
    > +void clock_was_set(void)
    > +{
    > + preempt_disable();
    > + if (hrtimer_hres_active) {
    > + retrigger_next_event(NULL);
    > +
    > + if (smp_call_function(retrigger_next_event, NULL, 1, 1))
    > + BUG();
    > + }
    > + preempt_enable();
    > +}

    If you use on_each_cpu() here you know that retrigger_next_event() will be
    called under local_irq_disable(). The preempt_disable() goes away and the
    spin_lock_irqsave() in retrigger_next_event() becomes a spin_lock() and
    everything becomes simpler.

    > +/**
    > + * hrtimer_clock_notify - A clock source or a clock event has been installed
    > + *
    > + * Notify the per cpu softirqs to recheck the clock sources and events
    > + */
    > +void hrtimer_clock_notify(void)
    > +{
    > + int i;
    > +
    > + for (i = 0; i < NR_CPUS; i++)
    > + set_bit(0, &per_cpu(hrtimer_bases, i).check_clocks);
    > +}

    This will go splat if/when the arch chooses to not implement per-cpu
    storage for not-possible CPUs. Use for_each_possible_cpu().

    > +
    > +static const ktime_t nsec_per_hz = { .tv64 = NSEC_PER_SEC / HZ };
    > +

    This could use the same trick as KTIME_HIGH_RES and friends. But perhaps
    the compiler will generate the same code..

    > +/*
    > + * We switched off the global tick source when switching to high resolution
    > + * mode. Update jiffies64.
    > + *
    > + * Must be called with interrupts disabled !
    > + *
    > + * FIXME: We need a mechanism to assign the update to a CPU. In principle this
    > + * is not hard, but when dynamic ticks come into play it starts to be. We don't
    > + * want to wake up a complete idle cpu just to update jiffies, so we need
    > + * something more intellegent than a mere "do this only on CPUx".
    > + */
    > +static void update_jiffies64(ktime_t now)
    > +{
    > + ktime_t delta;
    > +
    > + write_seqlock(&xtime_lock);
    > +
    > + delta = ktime_sub(now, last_jiffies_update);
    > + if (delta.tv64 >= nsec_per_hz.tv64) {
    > +

    stray blank line.

    > + unsigned long orun = 1;

    "orun"?

    > +
    > + delta = ktime_sub(delta, nsec_per_hz);
    > + last_jiffies_update = ktime_add(last_jiffies_update,
    > + nsec_per_hz);
    > +
    > + /* Slow path for long timeouts */
    > + if (unlikely(delta.tv64 >= nsec_per_hz.tv64)) {
    > + s64 incr = ktime_to_ns(nsec_per_hz);
    > + orun = ktime_divns(delta, incr);
    > +
    > + last_jiffies_update = ktime_add_ns(last_jiffies_update,
    > + incr * orun);
    > + jiffies_64 += orun;
    > + orun++;
    > + }

    That's a bit of a hack isn't it? do_timer() owns the modification of
    jiffies_64, so why is this code modifying it as well?

    > + do_timer(orun);

    twice?

    I suspect a bug.

    > + }
    > + write_sequnlock(&xtime_lock);
    > +}
    > +
    > +/*
    > + * We rearm the timer until we get disabled by the idle code
    > + */
    > +static int hrtimer_sched_tick(struct hrtimer *timer)
    > +{
    > + unsigned long flags;
    > + struct hrtimer_cpu_base *cpu_base =
    > + container_of(timer, struct hrtimer_cpu_base, sched_timer);
    > +
    > + local_irq_save(flags);
    > + /*
    > + * Do not call, when we are not in irq context and have
    > + * no valid regs pointer
    > + */
    > + if (cpu_base->sched_regs) {
    > + update_process_times(user_mode(cpu_base->sched_regs));
    > + profile_tick(CPU_PROFILING, cpu_base->sched_regs);
    > + }
    > +
    > + hrtimer_forward(timer, hrtimer_cb_get_time(timer), nsec_per_hz);
    > + local_irq_restore(flags);
    > +
    > + return HRTIMER_RESTART;

    bah. hrtimer_restart is an `enum hrtimer_restart', not an integer.

    > + printk(KERN_INFO "hrtimers: Switched to high resolution mode CPU %d\n",
    > + smp_processor_id());

    "on CPU"

    > +
    > +static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
    > + struct hrtimer_clock_base *base)
    > +{
    > + /*
    > + * When High resolution timers are active try to reprogram. Note, that
    > + * in case the state has HRTIMER_CALLBACK set, no reprogramming and no
    > + * expiry check happens. The timer gets enqueued into the rbtree and
    > + * the reprogramming / expiry check is done in the hrtimer_interrupt or
    > + * in the softirq.
    > + */

    This (useful) comment should be above the function, not inside it.

    > + if (hrtimer_hres_active && hrtimer_reprogram(timer, base)) {
    > +
    > + /* Timer is expired, act upon the callback mode */
    > + switch(timer->cb_mode) {
    > + case HRTIMER_CB_IRQSAFE_NO_RESTART:
    > + /*
    > + * We can call the callback from here. No restart
    > + * happens, so no danger of recursion
    > + */
    > + BUG_ON(timer->function(timer) != HRTIMER_NORESTART);

    Doing assert(thing-which-has-side-effects) is poor form.

    I doubt if the kernel will work if someone goes and disables BUG_ON, but
    it's a laudable objective.


    > + return 1;
    > + case HRTIMER_CB_IRQSAFE_NO_SOFTIRQ:
    > + /*
    > + * This is solely for the sched tick emulation with
    > + * dynamic tick support to ensure that we do not
    > + * restart the tick right on the edge and end up with
    > + * the tick timer in the softirq ! The calling site
    > + * takes care of this.
    > + */
    > + return 1;
    > + case HRTIMER_CB_IRQSAFE:
    > + case HRTIMER_CB_SOFTIRQ:
    > + /*
    > + * Move everything else into the softirq pending list !
    > + */
    > + hrtimer_add_cb_pending(timer, base);
    > + raise_softirq(HRTIMER_SOFTIRQ);
    > + return 1;
    > + default:
    > + BUG();
    > + }
    > + }
    > + return 0;
    > +}
    > +
    > +static inline void hrtimer_resume_jiffie_update(void)

    hrtimer_resume_jiffy_update

    > +{
    > + unsigned long flags;
    > + ktime_t now = ktime_get();
    > +
    > + write_seqlock_irqsave(&xtime_lock, flags);
    > + last_jiffies_update = now;
    > + write_sequnlock_irqrestore(&xtime_lock, flags);
    > +}
    > +
    > +#else
    > +
    > +# define hrtimer_hres_active 0
    > +# define hrtimer_check_clocks() do { } while (0)
    > +# define hrtimer_enqueue_reprogram(t,b) 0
    > +# define hrtimer_force_reprogram(b) do { } while (0)
    > +# define hrtimer_cb_pending(t) 0
    > +# define hrtimer_remove_cb_pending(t) do { } while (0)
    > +# define hrtimer_init_hres(c) do { } while (0)
    > +# define hrtimer_init_timer_hres(t) do { } while (0)
    > +# define hrtimer_resume_jiffie_update() do { } while (0)
    > +
    > +#endif /* CONFIG_HIGH_RES_TIMERS */
    > +
    > /*
    > * Timekeeping resumed notification

    resume

    > +#ifdef CONFIG_HIGH_RES_TIMERS
    > +
    > +/*
    > + * High resolution timer interrupt
    > + * Called with interrupts disabled
    > + */
    > +void hrtimer_interrupt(struct pt_regs *regs)
    > +{
    > + struct hrtimer_clock_base *base;
    > + ktime_t expires_next, now;
    > + int i, raise = 0, cpu = smp_processor_id();
    > + struct hrtimer_cpu_base *cpu_base = &per_cpu(hrtimer_bases, cpu);
    > +
    > + BUG_ON(!cpu_base->hres_active);
    > +
    > + /* Store the regs for an possible sched_timer callback */
    > + cpu_base->sched_regs = regs;
    > + cpu_base->events++;
    > +
    > + retry:
    > + now = ktime_get();
    > +
    > + /* Check, if the jiffies need an update */
    > + update_jiffies64(now);
    > +
    > + expires_next.tv64 = KTIME_MAX;
    > +
    > + base = cpu_base->clock_base;
    > +
    > + for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
    > + ktime_t basenow;
    > + struct rb_node *node;
    > +
    > + spin_lock(&cpu_base->lock);
    > +
    > + basenow = ktime_add(now, base->offset);

    Would it be better to take the lock outside the loop, rather than hammering
    on it like this?


    > + while ((node = base->first)) {
    > + struct hrtimer *timer;
    > +
    > + timer = rb_entry(node, struct hrtimer, node);
    > +
    > + if (basenow.tv64 < timer->expires.tv64) {
    > + ktime_t expires;
    > +
    > + expires = ktime_sub(timer->expires,
    > + base->offset);
    > + if (expires.tv64 < expires_next.tv64)
    > + expires_next = expires;
    > + break;
    > + }
    > +
    > + /* Move softirq callbacks to the pending list */
    > + if (timer->cb_mode == HRTIMER_CB_SOFTIRQ) {
    > + __remove_hrtimer(timer, base, HRTIMER_PENDING, 0);
    > + hrtimer_add_cb_pending(timer, base);
    > + raise = 1;
    > + continue;
    > + }
    > +
    > + __remove_hrtimer(timer, base, HRTIMER_CALLBACK, 0);
    > +
    > + if (timer->function(timer) != HRTIMER_NORESTART) {
    > + BUG_ON(timer->state != HRTIMER_CALLBACK);
    > + /*
    > + * state == HRTIMER_CALLBACK prevents
    > + * reprogramming. We do this when we break out
    > + * of the loop !
    > + */
    > + enqueue_hrtimer(timer, base);
    > + }
    > + timer->state &= ~HRTIMER_CALLBACK;
    > + }
    > + spin_unlock(&cpu_base->lock);
    > + base++;
    > + }
    > +
    > + cpu_base->expires_next = expires_next;
    > +
    > + /* Reprogramming necessary ? */
    > + if (expires_next.tv64 != KTIME_MAX) {
    > + if (clockevents_set_next_event(expires_next, 0))
    > + goto retry;
    > + }
    > +
    > + /* Invalidate regs */
    > + cpu_base->sched_regs = NULL;
    > +
    > + /* Raise softirq ? */
    > + if (raise)
    > + raise_softirq(HRTIMER_SOFTIRQ);
    > +}
    > +
    >
    > ...
    >
    > static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mode)
    > @@ -701,7 +1226,8 @@ static int __sched do_nanosleep(struct h
    > set_current_state(TASK_INTERRUPTIBLE);
    > hrtimer_start(&t->timer, t->timer.expires, mode);
    >
    > - schedule();
    > + if (likely(t->task))
    > + schedule();

    Why? Needs a comment.

    > @@ -0,0 +1,22 @@
    > +#
    > +# Timer subsystem related configuration options
    > +#
    > +config HIGH_RES_TIMERS
    > + bool "High Resolution Timer Support"
    > + depends on GENERIC_TIME
    > + help
    > + This option enables high resolution timer support. If your
    > + hardware is not capable then this option only increases
    > + the size of the kernel image.
    > +
    > +config HIGH_RES_RESOLUTION
    > + int "High Resolution Timer resolution (nanoseconds)"
    > + depends on HIGH_RES_TIMERS
    > + default 1000
    > + help
    > + This sets the resolution in nanoseconds of the high resolution
    > + timers. Too fine a resolution (small a number) will usually
    > + not be observable due to normal system latencies. For an
    > + 800 MHz processor about 10,000 (10 microseconds) is recommended as a
    > + finest resolution. If you don't need that sort of resolution,
    > + larger values may generate less overhead.

    In that case the default is far too low.

    What value are you suggesting that users and vendors set it to?


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

    \
     
     \ /
      Last update: 2006-09-30 10:49    [W:0.060 / U:1.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site