Re: rfc: high-resolution timer framework

[Alexander Shishkin <alexander.shishkin%teleca.com@localhost>]

On Wed, Jun 25, 2008 at 03:25:40 +0000, Andrew Doran wrote:
> Hi,
Hi,

> I have some specific comments on the patch.
> 
> On Wed, Jun 25, 2008 at 04:45:50PM +0300, Alexander Shishkin wrote:
> 
>       +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.
Ok.

>       +/*
>       + * sys/kern/kern_hrtim.c
>       + *
>       + * High-resolution timer framework.
> 
> Usually goes after the copyright.
Ok.

>       +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.
This one seems to be more tricky.

>       +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.
Good point. I was looking for something like this, but must have somehow
overlooked the cpu_attach thing.

>       +struct tc_hrtim
>       +*tc_hrtim_getbest(void)
> 
> Doesn't seem to be any locking here.
Indeed.

>       +       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. 
Hm. Strange is how LOCKDEBUG never complained of such a possibility.
Will have to dig callout code.

>        int
>       +hrtimerfix(struct timeval *tv)
> 
> Why not use timespec throughout?
For historical reasons. It was never reconsidered since a while now. But
I don't see any valid reason to not use timespec instead of timeval.

>       +#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.
Not really. Clock interrupts (and experiments confirm) nowadays occur on
all cpus, since lapic_initclocks() gets called from cpu_hatch(), which
is the final step in secondary cpus' startup code. As for the
timecounter capability, I'm not at all sure this worked as intended
before, since the accounting happens in a field of a cpu_info structure,
thus is local to current cpu and will differ between cpus.

> 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().
This could work, but I'm still in favor of a 8254-less solution to this.

>       +       { DDB_ADD_CMD("hrtim",  db_hrtim_print_cmd,     0,
>       +           "Display high-resolution timer shit.", NULL, NULL) },
> 
> s/shit/state/
Whoops. Shame on me.

Thanks for taking time and providing very valuable comments!

Regards,
--
Alex