introduce a syscall for posix clock tuning

arch/blackfin/include/asm/unistd.h +2 -1

@@ -389,8 +389,9 @@
  #define __NR_rt_tgsigqueueinfo 368
  #define __NR_perf_event_open   369
  #define __NR_recvmmsg          370
+#define __NR_clock_adjtime     371
 
-#define __NR_syscall           371
+#define __NR_syscall           372
  #define NR_syscalls            __NR_syscall
 
  /* Old optional stuff no one actually uses */

arch/blackfin/mach-common/entry.S +0 -0

@@ -1628,6 +1628,7 @@ ENTRY(_sys_call_table)

       .long _sys_rt_tgsigqueueinfo
       .long _sys_perf_event_open
       .long _sys_recvmmsg             /* 370 */
+       .long _sys_clock_adjtime

       .rept NR_syscalls-(.-_sys_call_table)/4
       .long _sys_ni_syscall

FYI, this is going to hit a conflict as i'm about to push out an update to wire up the new 2.6.36 syscalls

kernel/compat.c +0 -0

index 5adab05..df1e469 100644

kernel/compat.c +14 -0

+
+       if (tp) {
+               if (get_compat_timespec(&ts, tp))
+                       return -EFAULT;
+               ptr = &ts;
+       }
+       oldfs = get_fs();
+       set_fs(KERNEL_DS);
+       err = sys_clock_adjtime(which_clock, ppb,
+                               (struct timespec __user *) ptr);
+       set_fs(oldfs);
+       return err;
+}
+
  long compat_sys_clock_getres(clockid_t which_clock,
                struct compat_timespec __user *tp)
  {

Quoted 8 lines · Richard Cochran

kernel/compat.c +0 -0

index 5adab05..df1e469 100644
@@ -628,6 +628,26 @@ long compat_sys_clock_gettime(clockid_t which_clock,
        return err;
  }
 

kernel/compat.c long compat_sys_clock_gettime(clockid_t which_clock, · line 636

Quoted 6 lines

kernel/compat.c +6 -0

+long compat_sys_clock_adjtime(clockid_t which_clock, int ppb,
+               struct compat_timespec __user *tp)
+{
+       long err;
+       mm_segment_t oldfs;
+       struct timespec ts, *ptr = NULL;

Shouldn't ptr be initialized with tp?

Quoted 18 lines

kernel/compat.c +14 -0

+
+       if (tp) {
+               if (get_compat_timespec(&ts, tp))
+                       return -EFAULT;
+               ptr = &ts;
+       }
+       oldfs = get_fs();
+       set_fs(KERNEL_DS);
+       err = sys_clock_adjtime(which_clock, ppb,
+                               (struct timespec __user *) ptr);
+       set_fs(oldfs);
+       return err;
+}
+
  long compat_sys_clock_getres(clockid_t which_clock,
                struct compat_timespec __user *tp)
  {

A new syscall is introduced that allows tuning of a POSIX clock. The syscall is implemented for four architectures: arm, blackfin, powerpc, and x86.

The new syscall, clock_adjtime, takes two parameters, a frequency adjustment in parts per billion, and a pointer to a struct timespec containing the clock offset. If the pointer is NULL, a frequency adjustment is performed. Otherwise, the clock offset is immediately corrected by skipping to the new time value.

In addtion, the patch provides way to unregister a posix clock. This function is need to support posix clocks implemented as modules.

kernel/posix-cpu-timers.c +0 -0

 /*
  * Sample a per-thread clock for the given task.

kernel/posix-timers.c +0 -0

index ad72342..089b0d1 100644
@@ -197,6 +197,12 @@ static int common_timer_create(struct k_itimer *new_timer)
 	return 0;
 }

kernel/posix-timers.c +5 -0

@@ -488,6 +494,21 @@ void register_posix_clock(const clockid_t clock_id, struct k_clock *new_clock)
 }
 EXPORT_SYMBOL_GPL(register_posix_clock);
 
+void unregister_posix_clock(const clockid_t clock_id)
+{
+	struct k_clock *clock;
+
+	if ((unsigned) clock_id >= MAX_CLOCKS) {

Quoted 4 lines

kernel/posix-timers.c +4 -0

+static inline int common_clock_adj(const clockid_t which_clock, int ppb,
+				   struct timespec *tp)
+{
+	return -EOPNOTSUPP;

kernel/posix-timers.c static int common_timer_create(struct k_itimer *new_timer) · line 208

Quoted 5 lines

kernel/posix-timers.c +2 -0

+}
+
 static int no_timer_create(struct k_itimer *new_timer)
 {
 	return -EOPNOTSUPP;

So we already return -EOPNOTSUPP in some cases? The man page does not document this. I wonder if we should change that to -EINVAL as well.

Quoted 4 lines · Arnd Bergmann

Quoted 4 lines

kernel/posix-timers.c +4 -0

+static inline int common_clock_adj(const clockid_t which_clock, int ppb,
+                              struct timespec *tp)
+{
+   return -EOPNOTSUPP;

Quoted 8 lines

kernel/posix-timers.c static int common_timer_create(struct k_itimer *new_timer) · line 208

Quoted 5 lines

kernel/posix-timers.c +2 -0

+}
+
 static int no_timer_create(struct k_itimer *new_timer)
 {
    return -EOPNOTSUPP;

So we already return -EOPNOTSUPP in some cases? The man page does not document this. I wonder if we should change that to -EINVAL as well.

ENOTTY is the usual errno for "inappropriate ioctl for device". Due to the way this patch has been chopped up, I can't tell if that's what is intended here.

Quoted 9 lines · Richard Cochran

A new syscall is introduced that allows tuning of a POSIX clock. The syscall is implemented for four architectures: arm, blackfin, powerpc, and x86.

The new syscall, clock_adjtime, takes two parameters, a frequency adjustment in parts per billion, and a pointer to a struct timespec containing the clock offset. If the pointer is NULL, a frequency adjustment is performed. Otherwise, the clock offset is immediately corrected by skipping to the new time value.

It looks well-implemented, and seems to be a reasonable extension to the clock API. I'm looking forward to your ptp patches on top of this to see how it all fits together.

For new syscalls, it's best to take linux-api on Cc. I also added John, since he participated in the discussion.

As I mentioned in the previous mail, I agree the new functionality (adjusting the time by an offset instantaneously) is useful, but I'd prefer it be done initially within the existing adjtimex() interface.

Then if the posix-time clock_id multiplexing version of adjtimex is found to be necessary, the new syscall should be introduced, using the same API (not all clock_ids need to support all the adjtimex modes, but the new interface should be sufficient for NTPd to use).

There are some other conceptual issues this new syscall introduces:

1) While clock_adjtimex(CLOCK_REALTIME,...) would be equivalent to adjtimex(), would clock_adjtimex(CLOCK_MONOTONIC,...) make sense?

Given CLOCK_MONOTONIC and CLOCK_REALTIME are both based off the same notion of time, but offset from each other, any adjustment to one clock would be reflected in the other. However, the API would make it seem like they could be adjusted independently.

2) The same issue in #1 exists for CLOCK_REALTIME/MONOTONIC_COARSE variants.

3) Freq steering for MONOTONIC_RAW would defeat the purpose of the clock_id.

4) Does adjustments to CPU_TIME clock_ids make sense?

Quoted 3 lines · john stultz

As I mentioned in the previous mail, I agree the new functionality (adjusting the time by an offset instantaneously) is useful, but I'd prefer it be done initially within the existing adjtimex() interface.

But the adjtimex does not support nanosecond resolution.

Quoted 4 lines

Then if the posix-time clock_id multiplexing version of adjtimex is found to be necessary, the new syscall should be introduced, using the same API (not all clock_ids need to support all the adjtimex modes, but the new interface should be sufficient for NTPd to use).

Would the new syscall need to take a struct timex?

If so, I think it not worth the effort of adding a syscall. Instead, we can just add "clockid" flags into the mode field.

Quoted 5 lines

There are some other conceptual issues this new syscall introduces:

1) While clock_adjtimex(CLOCK_REALTIME,...) would be equivalent to adjtimex(), would clock_adjtimex(CLOCK_MONOTONIC,...) make sense?

Quoted 4 lines

Given CLOCK_MONOTONIC and CLOCK_REALTIME are both based off the same notion of time, but offset from each other, any adjustment to one clock would be reflected in the other. However, the API would make it seem like they could be adjusted independently.

You could adjust the frequency of either one. As a side effect, the other clock would also be adjusted.

You can only change the time offset on CLOCK_REALTIME, and that would have no effect on CLOCK_MONOTONIC.

Quoted 2 lines

3) Freq steering for MONOTONIC_RAW would defeat the purpose of the clock_id.

If I understand correctly, MONOTONIC_RAW is just access to the hardware counter?