Re: rfc: high-resolution timer framework

[Andrew Doran <ad%netbsd.org@localhost>]

Hi,

I have some specific comments on the patch.

On Wed, Jun 25, 2008 at 04:45:50PM +0300, Alexander Shishkin wrote:

> This is to announce and request for comments on the so-called
> high-resolution timer framework [1] that we came up with and want to share
> and further improve.
> 
> High-resolution timers are implemented on top of posix timers interface
> and are meant to provide userspace applications with timers as accurate
> as 1ms. Current implementation adds separate CLOCK_HIGHRES timer type,
> which might be moved in place of CLOCK_REALTIME code at some point.
> 
> The implementation comes in three patches for the convenience: first one
> introduces the framework into kernel so that it extends timecounter
> structure with an additional field which is either a pointer to a
> structure with two high-resolution timer methods or NULL in case a
> timecounter in not capable of this. Relevant timecounter drivers are
> changed to set this field to NULL; two other patches introduce experimental
> high-resolution timer drivers based upon lapic and omap general-purpose
> timer.
> 
> [1] http://koowaldah.org/people/ash/netbsd/hrtim-patchset.tar.gz


        +RB_HEAD(hrtim_tree, ptimer);
        +RB_PROTOTYPE(hrtim_tree, ptimer, pt_rb_entry, hrtim_rb_compare);
        +RB_GENERATE(hrtim_tree, ptimer, pt_rb_entry, hrtim_rb_compare);

The rb stuff from sys/rb.h is preferred.

        +/*
        + * sys/kern/kern_hrtim.c
        + *
        + * High-resolution timer framework.

Usually goes after the copyright.

        +static inline void
        +hrtim_signal(struct ptimer *pt, cpuid_t cpuid)
        +static inline void
        +_hrtim_intr_cpu(struct timeval *now, cpuid_t cpuid)  

It's unsafe to drop the lock in the middle of RBTREE_FOREACH: the tree can
change. Also pt_proc can disappear once the lock is dropped to send a signal
because no kind reference is held to it. I realise that the current code has
this problem. You could, for example, wrap the entire block with proc_lock.

        +struct hrtim_head hrtim_heads[MAXCPUS];
        +       cpuid = curcpu()->ci_cpuid;

ci_cpuid is for MD code, cpu_index() is the MI interface. Also it's better
to have store a pointer from struct cpu_data, and add a hrtim_cpu_attach()
to allocate the heads dynamically.

        +void
        +hrtim_init(void)
        +{
        +#ifdef MULTIPROCESSOR

The #ifdef isn't needed, esp. if you have a cpu_attach hook.

        +struct tc_hrtim
        +*tc_hrtim_getbest(void)

Doesn't seem to be any locking here.

        +       if (pt->pt_type == CLOCK_REALTIME) {
        +               mutex_spin_exit(&timer_lock);
                        callout_halt(&pt->pt_ch, &timer_lock);

callout_halt() may try to drop timer_lock, but it will panic because the
lock is unheld. 

         int
        +hrtimerfix(struct timeval *tv)

Why not use timespec throughout?

        +#ifdef LAPIC_HRTIMER
        +void
        +lapic_clockintr(void *arg, struct intrframe *frame)
        +{

It breaks if the machine has more than one CPU, because clock interrupts
will only occur on the boot CPU. Presumably it also prevents the lapic from
being used as a timecounter source, but that's not a big deal. It's also
unfortunate that it's optional.

One solution it is to always use the i8254 for the clock interrupt, and
trigger a broadcast IPI to the other CPUs from the i8254 clock ISR before
calling the MI hardclock().

        +       { DDB_ADD_CMD("hrtim",  db_hrtim_print_cmd,     0,
        +           "Display high-resolution timer shit.", NULL, NULL) },

s/shit/state/

Andrew