lkml.org 
[lkml]   [2004]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Bugfix for CLOCK_REALTIME absolute timer.
Andrew Morton wrote:
> George Anzinger <george@mvista.com> wrote:
>
>>Andrew,
>>
>>Boris and I have kicked this around enough. It think it is ready for prime time.
>>
>
>
>> static void schedule_next_timer(struct k_itimer *timr)
>> {
>>...
>>+ do {
>>+ seq = read_seqbegin(&xtime_lock);
>>+ new_wall_to = wall_to_monotonic;
>>+ posix_get_now(&now);
>>+ } while (read_seqretry(&xtime_lock, seq));
>>+
>>+ if (!list_empty(&timr->abs_timer_entry)) {
>>+ spin_lock(&abs_list.lock);
>>+ add_clockset_delta(timr, &new_wall_to);
>>+ }
>>+
>> do {
>> posix_bump_timer(timr);
>> }while (posix_time_before(&timr->it_timer, &now));
>>
>>+ if (!list_empty(&timr->abs_timer_entry))
>>+ spin_unlock(&abs_list.lock);
>
>
> The locking in here is a bit ugly. Does the lock actually need to be held while
> the timer is being bumped?
>
> And what is the upper bound on that while loop?

Yes, I agree that it is ugly. And on top of that, we are holding the timer lock
which is an irq version. As to the need to hold it through the bump, we want
the timers expire time to match what it should given the clock setting we are
using, otherwise, the clock_was_set() code could come through and change that
under us. (Not a pretty sight.)

Now, the bounding on the while, :( it depends on a) the interval and b) how far
the clock was moved. The shorter the interval or the longer the time interval,
the more we loop. I suppose the best thing to do here is an div, but will, most
of the time, take longer.
>
>
>> tmr->it_id = (timer_t)-1;
>>+ INIT_LIST_HEAD(&tmr->abs_timer_entry);
>> if (unlikely(!(tmr->sigq = sigqueue_alloc()))) {
>
>
> The cat ate your tab key? ;)

Oh, shit. And I thought I had the cat locked away in the back room. ;)
>
>
>>+ if (!list_empty(&timr->abs_timer_entry)) {
>>+ spin_lock(&abs_list.lock);
>>+ list_del_init(&timr->abs_timer_entry);
>>+ spin_unlock(&abs_list.lock);
>>+ }
>
>
> This is repeated often. Does it merit its own function?

Ok.
>
>
>>+static DECLARE_MUTEX(clock_was_set_lock);
>>+#define mutex_enter(x) down(x)
>>+#define mutex_enter_interruptable(x) down_interruptible(x)
>>+#define mutex_exit(x) up(x)
>
>
> Please open-code these operations.

Eh? Seems funny that we have a definition for mutex (DECLARE_MUTEX) and don't
have the defines to use them. But, ok.
>
>

--
George Anzinger george@mvista.com
High-res-timers: http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

-
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: 2005-03-22 14:04    [W:0.037 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site