Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S264239AbUFBWIi (ORCPT ); Wed, 2 Jun 2004 18:08:38 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S264256AbUFBWIh (ORCPT ); Wed, 2 Jun 2004 18:08:37 -0400 Received: from gateway-1237.mvista.com ([12.44.186.158]:41970 "EHLO av.mvista.com") by vger.kernel.org with ESMTP id S264695AbUFBWGw (ORCPT ); Wed, 2 Jun 2004 18:06:52 -0400 Message-ID: <40BE4F31.6090001@mvista.com> Date: Wed, 02 Jun 2004 15:05:37 -0700 From: George Anzinger Reply-To: ganzinger@mvista.com Organization: MontaVista Software User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20030225 X-Accept-Language: en-us, en MIME-Version: 1.0 To: "Hu, Boris" CC: drepper@redhat.com, "Li, Adam" , linux-kernel@vger.kernel.org Subject: Re: [PATCH] One possible bugfix for CLOCK_REALTIME timer. References: <37FBBA5F3A361C41AB7CE44558C3448E04560E0C@PDSMSX403.ccr.corp.intel.com> In-Reply-To: <37FBBA5F3A361C41AB7CE44558C3448E04560E0C@PDSMSX403.ccr.corp.intel.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8506 Lines: 234 Hu, Boris wrote: > <> George, > > There is one bug of CLOCK_REALTIME timer reported by Adam at > http://sources.redhat.com/ml/libc-alpha/2004-05/msg00177.html. > > Here is one possible bugfix and it is against linux-2.6.6. All > TIMER_ABSTIME cloks will be collected in k_clock struct and updated in > sys_clock_settime. Could you review it? Thanks. Thanks for the poke :). Could you make the following changes: First, put the list in the posix timer structure (k_itimer), not in timer_list. This means one more dereference when doing things, but it does not push into the timer_list structure which is mostly used for other things. Second, I don't see the timer being removed from the list (should happen when ever it is inactive). Timers that repeat should be out of the list while waiting for the signal to be picked up and put back in when add_timer is again called. Also, you should test to see if the clock is one that can be set prior to putting the timer in the abs timer list. We must not correct timers on CLOCK_MONOTONIC. Now, for the correction. Sys_clock_settime() is the wrong place for this as the clock can also be set a number of other ways. The right place is in clock_was_set(), which is called if time is set in any of the several ways. The next thing is to determine how far the clock was moved. I think the best way to do this is to keep a copy of the wall_to_monotonic var in a private location. This should be set to be exactly wall_to_monotonic when the system is booted (in the same function you are setting up the clock lists) and at the end of clock_was_set(). When clock_was_set() is called the difference between this value and wall_to_monotonic is exactly how far the clock was moved. (Be careful on the sign of this movement.) Finally, be careful about races. Timers can expire while clock_was_set() is running. The removal code should take the timer lock as well as the abs_time list lock (at least I think this would be wise, but I could be wrong here). And a minor issue, the community seems to prefer the C comment style to the C++ style of comments... Thanks for your effort in this matter. George > > > diff -urN -X rt.ia32/base/dontdiff > linux-2.6.6/include/linux/posix-timers.h > linux-2.6.6.dev/include/linux/posix-timers.h > --- linux-2.6.6/include/linux/posix-timers.h 2004-05-10 > 10:32:29.000000000 +0800 > +++ linux-2.6.6.dev/include/linux/posix-timers.h 2004-06-02 > 10:30:57.000000000 +0800 > @@ -1,9 +1,14 @@ > #ifndef _linux_POSIX_TIMERS_H > #define _linux_POSIX_TIMERS_H > > +#include > +#include > + > struct k_clock { > int res; /* in nano seconds */ > - int (*clock_set) (struct timespec * tp); > + struct list_head abs_timer_list; > + spinlock_t abs_timer_lock; > + int (*clock_set) (struct timespec * tp); > int (*clock_get) (struct timespec * tp); > int (*nsleep) (int flags, > struct timespec * new_setting, > diff -urN -X rt.ia32/base/dontdiff linux-2.6.6/include/linux/timer.h > linux-2.6.6.dev/include/linux/timer.h > --- linux-2.6.6/include/linux/timer.h 2004-05-10 10:32:54.000000000 > +0800 > +++ linux-2.6.6.dev/include/linux/timer.h 2004-06-02 > 19:16:08.000000000 +0800 > @@ -9,6 +9,7 @@ > > struct timer_list { > struct list_head entry; > + struct list_head abs_timer_entry; > unsigned long expires; > > spinlock_t lock; > diff -urN -X rt.ia32/base/dontdiff linux-2.6.6/kernel/posix-timers.c > linux-2.6.6.dev/kernel/posix-timers.c > --- linux-2.6.6/kernel/posix-timers.c 2004-05-10 10:32:37.000000000 > +0800 > +++ linux-2.6.6.dev/kernel/posix-timers.c 2004-06-02 > 19:12:31.000000000 +0800 > @@ -7,6 +7,9 @@ > * > * Copyright (C) 2002 2003 by MontaVista > Software. > * > + * 2004-06-01 Fix CLOCK_REALTIME clock/timer TIMER_ABSTIME bug. > + * Copyright (C) 2004 Boris Hu > + * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > * the Free Software Foundation; either version 2 of the License, or > (at > @@ -200,7 +203,9 @@ > */ > static __init int init_posix_timers(void) > { > - struct k_clock clock_realtime = {.res = CLOCK_REALTIME_RES }; > + struct k_clock clock_realtime = {.res = CLOCK_REALTIME_RES, > + .abs_timer_lock = SPIN_LOCK_UNLOCKED > > + }; > struct k_clock clock_monotonic = {.res = CLOCK_REALTIME_RES, > .clock_get = do_posix_clock_monotonic_gettime, > .clock_set = do_posix_clock_monotonic_settime > @@ -388,6 +393,7 @@ > return; > } > posix_clocks[clock_id] = *new_clock; > + INIT_LIST_HEAD(&posix_clocks[clock_id].abs_timer_list); > } > > static struct k_itimer * alloc_posix_timer(void) > @@ -843,8 +849,15 @@ > if (((timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE)) > { > if (timr->it_timer.expires == jiffies) > timer_notify_task(timr); > - else > + else { > add_timer(&timr->it_timer); > + if (flags & TIMER_ABSTIME) { > + spin_lock(&clock->abs_timer_lock); > + > list_add_tail(&(timr->it_timer.abs_timer_entry), > + > &(clock->abs_timer_list)); > + spin_unlock(&clock->abs_timer_lock); > + } > + } > } > return 0; > } > @@ -1085,16 +1098,61 @@ > sys_clock_settime(clockid_t which_clock, const struct timespec __user > *tp) > { > struct timespec new_tp; > + struct timespec before, now; > + struct k_clock *clock; > + long ret; > + struct timer_list *timer, *tmp; > + struct timespec delta; > + u64 exp = 0; > > - if ((unsigned) which_clock >= MAX_CLOCKS || > + if ((unsigned) which_clock >= MAX_CLOCKS || > !posix_clocks[which_clock].res) > return -EINVAL; > - if (copy_from_user(&new_tp, tp, sizeof (*tp))) > + > + clock = &posix_clocks[which_clock]; > + > + if (copy_from_user(&new_tp, tp, sizeof (*tp))) > return -EFAULT; > if (posix_clocks[which_clock].clock_set) > return posix_clocks[which_clock].clock_set(&new_tp); > > - return do_sys_settimeofday(&new_tp, NULL); > + do_posix_gettime(clock, &before); > + > + ret = do_sys_settimeofday(&new_tp, NULL); > + > + /* > + * Check if there exist TIMER_ABSTIME timers to update. > + */ > + if (which_clock != CLOCK_REALTIME || > + list_empty(&clock->abs_timer_list)) > + return ret; > + > + do_posix_gettime(clock, &now); > + delta.tv_sec = before.tv_sec - now.tv_sec; > + delta.tv_nsec = before.tv_nsec - now.tv_nsec; > + while (delta.tv_nsec >= NSEC_PER_SEC) { > + delta.tv_sec ++; > + delta.tv_nsec -= NSEC_PER_SEC; > + } > + while (delta.tv_nsec < 0) { > + delta.tv_sec --; > + delta.tv_nsec += NSEC_PER_SEC; > + } > + > + tstojiffie(&delta, clock->res, &exp); > + > + spin_lock(&(clock->abs_timer_lock)); > + list_for_each_entry_safe(timer, tmp, > + &clock->abs_timer_list, > + abs_timer_entry) { > + if (timer && del_timer(timer)) { //the timer has not > run > + timer->expires += exp; > + add_timer(timer); > + } else > + list_del(&timer->abs_timer_entry); > + } > + spin_unlock(&(clock->abs_timer_lock)); > + return ret; > } > > asmlinkage long > > Good Luck ! > Boris Hu (Hu Jiangtao) > Intel China Software Center > 86-021-5257-4545#1277 > iNET: 8-752-1277 > ************************************ > There are my thoughts, not my employer's. > ************************************ > "gpg --recv-keys --keyserver wwwkeys.pgp.net 0FD7685F" > {0FD7685F:CFD6 6F5C A2CB 7881 725B CEA0 956F 9F14 0FD7 685F} > > -- 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/