[PATCH 0/2] [RFC] posix clock tuning

[PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

Recently on the lkml, we discussed how to allow adjustment of posix
clocks. I have tried to understand and implement the ideas expressed
in the various threads.

If there is agreement on this approach, I will resubmit these two
patches as part of a longer series, including support for a new kind
of hardware clock, so you can see how it all fits together. But first,
I would like to concentrate on the new syscall itself.

The patches are against a recent net-next tree. Please don't worry if
the syscall tables are not quite up to date. I can fix that later.

Thanks,
Richard


Richard Cochran (2):
  posix clocks: introduce a syscall for clock tuning.
  posix clocks: introduce a sysfs presence.

 Documentation/ABI/testing/sysfs-timesource |   24 +++++
 arch/arm/include/asm/unistd.h              |    1 +
 arch/arm/kernel/calls.S                    |    1 +
 arch/blackfin/include/asm/unistd.h         |    3 +-
 arch/blackfin/mach-common/entry.S          |    1 +
 arch/powerpc/include/asm/systbl.h          |    1 +
 arch/powerpc/include/asm/unistd.h          |    3 +-
 arch/x86/ia32/ia32entry.S                  |    1 +
 arch/x86/include/asm/unistd_32.h           |    3 +-
 arch/x86/include/asm/unistd_64.h           |    2 +
 arch/x86/kernel/syscall_table_32.S         |    1 +
 drivers/char/mmtimer.c                     |    1 +
 include/linux/posix-timers.h               |   14 +++-
 include/linux/syscalls.h                   |    2 +
 include/linux/time.h                       |    2 +
 include/linux/timex.h                      |    3 +-
 kernel/compat.c                            |  136 ++++++++++++++++++----------
 kernel/posix-cpu-timers.c                  |    6 ++
 kernel/posix-timers.c                      |   98 +++++++++++++++++++--
 19 files changed, 243 insertions(+), 60 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-timesource

[PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[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, the clock ID,
and a pointer to a struct timex. The semantics of the timex struct
have been expanded by one additional mode flag, which allows an
absolute offset correction. When specificied, the clock offset is
immediately corrected by skipping to the new time value.

In addition, the POSIX clock code has been augmented to offer a
dynamic clock creation method. Instead of registering a hard
coded clock ID, modules may call create_posix_clock(), which
returns a new clock ID.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 arch/arm/include/asm/unistd.h      |    1 +
 arch/arm/kernel/calls.S            |    1 +
 arch/blackfin/include/asm/unistd.h |    3 +-
 arch/blackfin/mach-common/entry.S  |    1 +
 arch/powerpc/include/asm/systbl.h  |    1 +
 arch/powerpc/include/asm/unistd.h  |    3 +-
 arch/x86/ia32/ia32entry.S          |    1 +
 arch/x86/include/asm/unistd_32.h   |    3 +-
 arch/x86/include/asm/unistd_64.h   |    2 +
 arch/x86/kernel/syscall_table_32.S |    1 +
 include/linux/posix-timers.h       |   10 +++-
 include/linux/syscalls.h           |    2 +
 include/linux/time.h               |    2 +
 include/linux/timex.h              |    3 +-
 kernel/compat.c                    |  136 +++++++++++++++++++++++-------------
 kernel/posix-cpu-timers.c          |    4 +
 kernel/posix-timers.c              |   58 +++++++++++++--
 17 files changed, 172 insertions(+), 60 deletions(-)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index dd2bf53..6bea0b7 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -392,6 +392,7 @@
 #define __NR_rt_tgsigqueueinfo		(__NR_SYSCALL_BASE+363)
 #define __NR_perf_event_open		(__NR_SYSCALL_BASE+364)
 #define __NR_recvmmsg			(__NR_SYSCALL_BASE+365)
+#define __NR_clock_adjtime		(__NR_SYSCALL_BASE+366)
 
 /*
  * The following SWIs are ARM private.
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 37ae301..8a22fdd 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -375,6 +375,7 @@
 		CALL(sys_rt_tgsigqueueinfo)
 		CALL(sys_perf_event_open)
 /* 365 */	CALL(sys_recvmmsg)
+		CALL(sys_clock_adjtime)
 #ifndef syscalls_counted
 .equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
 #define syscalls_counted
diff --git a/arch/blackfin/include/asm/unistd.h b/arch/blackfin/include/asm/unistd.h
index 22886cb..6671913 100644
--- a/arch/blackfin/include/asm/unistd.h
+++ b/arch/blackfin/include/asm/unistd.h
@@ -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 */
diff --git a/arch/blackfin/mach-common/entry.S b/arch/blackfin/mach-common/entry.S
index a5847f5..252f2fa 100644
--- a/arch/blackfin/mach-common/entry.S
+++ b/arch/blackfin/mach-common/entry.S
@@ -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
diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index a5ee345..e7dce86 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -326,3 +326,4 @@ SYSCALL_SPU(perf_event_open)
 COMPAT_SYS_SPU(preadv)
 COMPAT_SYS_SPU(pwritev)
 COMPAT_SYS(rt_tgsigqueueinfo)
+COMPAT_SYS_SPU(clock_adjtime)
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index f0a1026..7d4d9c8 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -345,10 +345,11 @@
 #define __NR_preadv		320
 #define __NR_pwritev		321
 #define __NR_rt_tgsigqueueinfo	322
+#define __NR_clock_adjtime	323
 
 #ifdef __KERNEL__
 
-#define __NR_syscalls		323
+#define __NR_syscalls		324
 
 #define __NR__exit __NR_exit
 #define NR_syscalls	__NR_syscalls
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index b86feab..2771351 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -845,4 +845,5 @@ ia32_sys_call_table:
 	.quad sys_fanotify_init
 	.quad sys32_fanotify_mark
 	.quad sys_prlimit64		/* 340 */
+	.quad compat_sys_clock_adjtime
 ia32_syscall_end:
diff --git a/arch/x86/include/asm/unistd_32.h b/arch/x86/include/asm/unistd_32.h
index b766a5e..b6f73f1 100644
--- a/arch/x86/include/asm/unistd_32.h
+++ b/arch/x86/include/asm/unistd_32.h
@@ -346,10 +346,11 @@
 #define __NR_fanotify_init	338
 #define __NR_fanotify_mark	339
 #define __NR_prlimit64		340
+#define __NR_clock_adjtime	341
 
 #ifdef __KERNEL__
 
-#define NR_syscalls 341
+#define NR_syscalls 342
 
 #define __ARCH_WANT_IPC_PARSE_VERSION
 #define __ARCH_WANT_OLD_READDIR
diff --git a/arch/x86/include/asm/unistd_64.h b/arch/x86/include/asm/unistd_64.h
index 363e9b8..5ee3085 100644
--- a/arch/x86/include/asm/unistd_64.h
+++ b/arch/x86/include/asm/unistd_64.h
@@ -669,6 +669,8 @@ __SYSCALL(__NR_fanotify_init, sys_fanotify_init)
 __SYSCALL(__NR_fanotify_mark, sys_fanotify_mark)
 #define __NR_prlimit64				302
 __SYSCALL(__NR_prlimit64, sys_prlimit64)
+#define __NR_clock_adjtime			303
+__SYSCALL(__NR_clock_adjtime, sys_clock_adjtime)
 
 #ifndef __NO_STUBS
 #define __ARCH_WANT_OLD_READDIR
diff --git a/arch/x86/kernel/syscall_table_32.S b/arch/x86/kernel/syscall_table_32.S
index b35786d..68c7b9a 100644
--- a/arch/x86/kernel/syscall_table_32.S
+++ b/arch/x86/kernel/syscall_table_32.S
@@ -340,3 +340,4 @@ ENTRY(sys_call_table)
 	.long sys_fanotify_init
 	.long sys_fanotify_mark
 	.long sys_prlimit64		/* 340 */
+	.long sys_clock_adjtime
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 3e23844..08aa4da 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -4,6 +4,7 @@
 #include <linux/spinlock.h>
 #include <linux/list.h>
 #include <linux/sched.h>
+#include <linux/timex.h>
 
 union cpu_time_count {
 	cputime_t cpu;
@@ -67,10 +68,12 @@ struct k_itimer {
 };
 
 struct k_clock {
+	clockid_t id;
 	int res;		/* in nanoseconds */
 	int (*clock_getres) (const clockid_t which_clock, struct timespec *tp);
 	int (*clock_set) (const clockid_t which_clock, struct timespec * tp);
 	int (*clock_get) (const clockid_t which_clock, struct timespec * tp);
+	int (*clock_adj) (const clockid_t which_clock, struct timex *tx);
 	int (*timer_create) (struct k_itimer *timer);
 	int (*nsleep) (const clockid_t which_clock, int flags,
 		       struct timespec *, struct timespec __user *);
@@ -84,7 +87,11 @@ struct k_clock {
 			   struct itimerspec * cur_setting);
 };
 
-void register_posix_clock(const clockid_t clock_id, struct k_clock *new_clock);
+/* Regsiter a posix clock with a "well known" clock id. */
+int register_posix_clock(const clockid_t id, struct k_clock *clock);
+
+/* Create a new posix clock with a dynamic clock id. */
+clockid_t create_posix_clock(struct k_clock *clock);
 
 /* error handlers for timer_create, nanosleep and settime */
 int do_posix_clock_nonanosleep(const clockid_t, int flags, struct timespec *,
@@ -97,6 +104,7 @@ int posix_timer_event(struct k_itimer *timr, int si_private);
 int posix_cpu_clock_getres(const clockid_t which_clock, struct timespec *ts);
 int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *ts);
 int posix_cpu_clock_set(const clockid_t which_clock, const struct timespec *ts);
+int posix_cpu_clock_adj(const clockid_t which_clock, struct timex *tx);
 int posix_cpu_timer_create(struct k_itimer *timer);
 int posix_cpu_nsleep(const clockid_t which_clock, int flags,
 		     struct timespec *rqtp, struct timespec __user *rmtp);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 6e5d197..0f4d57c 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -313,6 +313,8 @@ asmlinkage long sys_clock_settime(clockid_t which_clock,
 				const struct timespec __user *tp);
 asmlinkage long sys_clock_gettime(clockid_t which_clock,
 				struct timespec __user *tp);
+asmlinkage long sys_clock_adjtime(clockid_t which_clock,
+				struct timex __user *tx);
 asmlinkage long sys_clock_getres(clockid_t which_clock,
 				struct timespec __user *tp);
 asmlinkage long sys_clock_nanosleep(clockid_t which_clock, int flags,
diff --git a/include/linux/time.h b/include/linux/time.h
index 9f15ac7..914c48d 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -299,6 +299,8 @@ struct itimerval {
 #define CLOCKS_MASK			(CLOCK_REALTIME | CLOCK_MONOTONIC)
 #define CLOCKS_MONO			CLOCK_MONOTONIC
 
+#define CLOCK_INVALID			-1
+
 /*
  * The various flags for setting POSIX.1b interval timers:
  */
diff --git a/include/linux/timex.h b/include/linux/timex.h
index 32d852f..82d4b24 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -73,7 +73,7 @@ struct timex {
 	long tolerance;		/* clock frequency tolerance (ppm)
 				 * (read only)
 				 */
-	struct timeval time;	/* (read only) */
+	struct timeval time;	/* (read only, except for ADJ_SETOFFSET) */
 	long tick;		/* (modified) usecs between clock ticks */
 
 	long ppsfreq;           /* pps frequency (scaled ppm) (ro) */
@@ -101,6 +101,7 @@ struct timex {
 #define ADJ_ESTERROR		0x0008	/* estimated time error */
 #define ADJ_STATUS		0x0010	/* clock status */
 #define ADJ_TIMECONST		0x0020	/* pll time constant */
+#define ADJ_SETOFFSET		0x0040  /* add 'time' to current time */
 #define ADJ_TAI			0x0080	/* set TAI offset */
 #define ADJ_MICRO		0x1000	/* select microsecond resolution */
 #define ADJ_NANO		0x2000	/* select nanosecond resolution */
diff --git a/kernel/compat.c b/kernel/compat.c
index e167efc..f408ab5 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -52,6 +52,64 @@ static int compat_put_timeval(struct compat_timeval __user *o,
 		put_user(i->tv_usec, &o->tv_usec)) ? -EFAULT : 0;
 }
 
+static int compat_get_timex(struct timex *txc, struct compat_timex __user *utp)
+{
+	memset(txc, 0, sizeof(struct timex));
+
+	if (!access_ok(VERIFY_READ, utp, sizeof(struct compat_timex)) ||
+			__get_user(txc->modes, &utp->modes) ||
+			__get_user(txc->offset, &utp->offset) ||
+			__get_user(txc->freq, &utp->freq) ||
+			__get_user(txc->maxerror, &utp->maxerror) ||
+			__get_user(txc->esterror, &utp->esterror) ||
+			__get_user(txc->status, &utp->status) ||
+			__get_user(txc->constant, &utp->constant) ||
+			__get_user(txc->precision, &utp->precision) ||
+			__get_user(txc->tolerance, &utp->tolerance) ||
+			__get_user(txc->time.tv_sec, &utp->time.tv_sec) ||
+			__get_user(txc->time.tv_usec, &utp->time.tv_usec) ||
+			__get_user(txc->tick, &utp->tick) ||
+			__get_user(txc->ppsfreq, &utp->ppsfreq) ||
+			__get_user(txc->jitter, &utp->jitter) ||
+			__get_user(txc->shift, &utp->shift) ||
+			__get_user(txc->stabil, &utp->stabil) ||
+			__get_user(txc->jitcnt, &utp->jitcnt) ||
+			__get_user(txc->calcnt, &utp->calcnt) ||
+			__get_user(txc->errcnt, &utp->errcnt) ||
+			__get_user(txc->stbcnt, &utp->stbcnt))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int compat_put_timex(struct compat_timex __user *utp, struct timex *txc)
+{
+	if (!access_ok(VERIFY_WRITE, utp, sizeof(struct compat_timex)) ||
+			__put_user(txc->modes, &utp->modes) ||
+			__put_user(txc->offset, &utp->offset) ||
+			__put_user(txc->freq, &utp->freq) ||
+			__put_user(txc->maxerror, &utp->maxerror) ||
+			__put_user(txc->esterror, &utp->esterror) ||
+			__put_user(txc->status, &utp->status) ||
+			__put_user(txc->constant, &utp->constant) ||
+			__put_user(txc->precision, &utp->precision) ||
+			__put_user(txc->tolerance, &utp->tolerance) ||
+			__put_user(txc->time.tv_sec, &utp->time.tv_sec) ||
+			__put_user(txc->time.tv_usec, &utp->time.tv_usec) ||
+			__put_user(txc->tick, &utp->tick) ||
+			__put_user(txc->ppsfreq, &utp->ppsfreq) ||
+			__put_user(txc->jitter, &utp->jitter) ||
+			__put_user(txc->shift, &utp->shift) ||
+			__put_user(txc->stabil, &utp->stabil) ||
+			__put_user(txc->jitcnt, &utp->jitcnt) ||
+			__put_user(txc->calcnt, &utp->calcnt) ||
+			__put_user(txc->errcnt, &utp->errcnt) ||
+			__put_user(txc->stbcnt, &utp->stbcnt) ||
+			__put_user(txc->tai, &utp->tai))
+		return -EFAULT;
+	return 0;
+}
+
 asmlinkage long compat_sys_gettimeofday(struct compat_timeval __user *tv,
 		struct timezone __user *tz)
 {
@@ -617,6 +675,29 @@ long compat_sys_clock_gettime(clockid_t which_clock,
 	return err;
 }
 
+long compat_sys_clock_adjtime(clockid_t which_clock,
+		struct compat_timex __user *utp)
+{
+	struct timex txc;
+	mm_segment_t oldfs;
+	int err, ret;
+
+	err = compat_get_timex(&txc, utp);
+	if (err)
+		return err;
+
+	oldfs = get_fs();
+	set_fs(KERNEL_DS);
+	ret = sys_clock_adjtime(which_clock, (struct timex __user *) &txc);
+	set_fs(oldfs);
+
+	err = compat_put_timex(utp, &txc);
+	if (err)
+		return err;
+
+	return ret;
+}
+
 long compat_sys_clock_getres(clockid_t which_clock,
 		struct compat_timespec __user *tp)
 {
@@ -951,58 +1032,17 @@ asmlinkage long compat_sys_rt_sigsuspend(compat_sigset_t __user *unewset, compat
 asmlinkage long compat_sys_adjtimex(struct compat_timex __user *utp)
 {
 	struct timex txc;
-	int ret;
-
-	memset(&txc, 0, sizeof(struct timex));
+	int err, ret;
 
-	if (!access_ok(VERIFY_READ, utp, sizeof(struct compat_timex)) ||
-			__get_user(txc.modes, &utp->modes) ||
-			__get_user(txc.offset, &utp->offset) ||
-			__get_user(txc.freq, &utp->freq) ||
-			__get_user(txc.maxerror, &utp->maxerror) ||
-			__get_user(txc.esterror, &utp->esterror) ||
-			__get_user(txc.status, &utp->status) ||
-			__get_user(txc.constant, &utp->constant) ||
-			__get_user(txc.precision, &utp->precision) ||
-			__get_user(txc.tolerance, &utp->tolerance) ||
-			__get_user(txc.time.tv_sec, &utp->time.tv_sec) ||
-			__get_user(txc.time.tv_usec, &utp->time.tv_usec) ||
-			__get_user(txc.tick, &utp->tick) ||
-			__get_user(txc.ppsfreq, &utp->ppsfreq) ||
-			__get_user(txc.jitter, &utp->jitter) ||
-			__get_user(txc.shift, &utp->shift) ||
-			__get_user(txc.stabil, &utp->stabil) ||
-			__get_user(txc.jitcnt, &utp->jitcnt) ||
-			__get_user(txc.calcnt, &utp->calcnt) ||
-			__get_user(txc.errcnt, &utp->errcnt) ||
-			__get_user(txc.stbcnt, &utp->stbcnt))
-		return -EFAULT;
+	err = compat_get_timex(&txc, utp);
+	if (err)
+		return err;
 
 	ret = do_adjtimex(&txc);
 
-	if (!access_ok(VERIFY_WRITE, utp, sizeof(struct compat_timex)) ||
-			__put_user(txc.modes, &utp->modes) ||
-			__put_user(txc.offset, &utp->offset) ||
-			__put_user(txc.freq, &utp->freq) ||
-			__put_user(txc.maxerror, &utp->maxerror) ||
-			__put_user(txc.esterror, &utp->esterror) ||
-			__put_user(txc.status, &utp->status) ||
-			__put_user(txc.constant, &utp->constant) ||
-			__put_user(txc.precision, &utp->precision) ||
-			__put_user(txc.tolerance, &utp->tolerance) ||
-			__put_user(txc.time.tv_sec, &utp->time.tv_sec) ||
-			__put_user(txc.time.tv_usec, &utp->time.tv_usec) ||
-			__put_user(txc.tick, &utp->tick) ||
-			__put_user(txc.ppsfreq, &utp->ppsfreq) ||
-			__put_user(txc.jitter, &utp->jitter) ||
-			__put_user(txc.shift, &utp->shift) ||
-			__put_user(txc.stabil, &utp->stabil) ||
-			__put_user(txc.jitcnt, &utp->jitcnt) ||
-			__put_user(txc.calcnt, &utp->calcnt) ||
-			__put_user(txc.errcnt, &utp->errcnt) ||
-			__put_user(txc.stbcnt, &utp->stbcnt) ||
-			__put_user(txc.tai, &utp->tai))
-		ret = -EFAULT;
+	err = compat_put_timex(utp, &txc);
+	if (err)
+		return err;
 
 	return ret;
 }
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 6842eeb..e1c2e7b 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -207,6 +207,10 @@ int posix_cpu_clock_set(const clockid_t which_clock, const struct timespec *tp)
 	return error;
 }
 
+int posix_cpu_clock_adj(const clockid_t which_clock, struct timex *tx)
+{
+	return -EOPNOTSUPP;
+}
 
 /*
  * Sample a per-thread clock for the given task.
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 9ca4973..67fba5c 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -132,6 +132,8 @@ static DEFINE_SPINLOCK(idr_lock);
  */
 
 static struct k_clock posix_clocks[MAX_CLOCKS];
+static DECLARE_BITMAP(clocks_map, MAX_CLOCKS);
+static DEFINE_MUTEX(clocks_mux); /* protects 'posix_clocks' and 'clocks_map' */
 
 /*
  * These ones are defined below.
@@ -197,6 +199,14 @@ static int common_timer_create(struct k_itimer *new_timer)
 	return 0;
 }
 
+static inline int common_clock_adj(const clockid_t which_clock, struct timex *t)
+{
+	if (CLOCK_REALTIME == which_clock)
+		return do_adjtimex(t);
+	else
+		return -EOPNOTSUPP;
+}
+
 static int no_timer_create(struct k_itimer *new_timer)
 {
 	return -EOPNOTSUPP;
@@ -476,18 +486,43 @@ static struct pid *good_sigevent(sigevent_t * event)
 	return task_pid(rtn);
 }
 
-void register_posix_clock(const clockid_t clock_id, struct k_clock *new_clock)
+int register_posix_clock(const clockid_t id, struct k_clock *clock)
 {
-	if ((unsigned) clock_id >= MAX_CLOCKS) {
-		printk("POSIX clock register failed for clock_id %d\n",
-		       clock_id);
-		return;
-	}
+	struct k_clock *kc;
+	int err = 0;
 
-	posix_clocks[clock_id] = *new_clock;
+	mutex_lock(&clocks_mux);
+	if (test_bit(id, clocks_map)) {
+		pr_err("clock_id %d already registered\n", id);
+		err = -EBUSY;
+		goto out;
+	}
+	kc = &posix_clocks[id];
+	*kc = *clock;
+	kc->id = id;
+	set_bit(id, clocks_map);
+out:
+	mutex_unlock(&clocks_mux);
+	return err;
 }
 EXPORT_SYMBOL_GPL(register_posix_clock);
 
+clockid_t create_posix_clock(struct k_clock *clock)
+{
+	clockid_t id;
+
+	mutex_lock(&clocks_mux);
+	id = find_first_zero_bit(clocks_map, MAX_CLOCKS);
+	mutex_unlock(&clocks_mux);
+
+	if (id < MAX_CLOCKS) {
+		register_posix_clock(id, clock);
+		return id;
+	}
+	return CLOCK_INVALID;
+}
+EXPORT_SYMBOL_GPL(create_posix_clock);
+
 static struct k_itimer * alloc_posix_timer(void)
 {
 	struct k_itimer *tmr;
@@ -969,6 +1004,15 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock,
 
 }
 
+SYSCALL_DEFINE2(clock_adjtime, const clockid_t, which_clock,
+		struct timex __user *, tx)
+{
+	if (invalid_clockid(which_clock))
+		return -EINVAL;
+
+	return CLOCK_DISPATCH(which_clock, clock_adj, (which_clock, tx));
+}
+
 SYSCALL_DEFINE2(clock_getres, const clockid_t, which_clock,
 		struct timespec __user *, tp)
 {
-- 
1.7.0.4

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Richard Cochran]

On Fri, Sep 03, 2010 at 11:29:05AM +0200, Richard Cochran wrote:
> +long compat_sys_clock_adjtime(clockid_t which_clock,
> +		struct compat_timex __user *utp)
> +{
> +	struct timex txc;
> +	mm_segment_t oldfs;
> +	int err, ret;
> +
> +	err = compat_get_timex(&txc, utp);
> +	if (err)
> +		return err;
> +
> +	oldfs = get_fs();
> +	set_fs(KERNEL_DS);
> +	ret = sys_clock_adjtime(which_clock, (struct timex __user *) &txc);
> +	set_fs(oldfs);

I don't know what the purpose of get_fs, set_fs is, but the other
clock_ functions have it that way. The adjtimex call does not have
this at all, so I ask, do we need this here, or not?

Thanks,
Richard

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Richard Cochran]

On Fri, Sep 03, 2010 at 11:29:05AM +0200, Richard Cochran wrote:
> 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, the clock ID,
> and a pointer to a struct timex. The semantics of the timex struct
> have been expanded by one additional mode flag, which allows an
> absolute offset correction. When specificied, the clock offset is
> immediately corrected by skipping to the new time value.

Sorry, last sentence should read, "corrected by adding the given time
value to the current time value."

Richard

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Thomas Gleixner]

On Fri, 3 Sep 2010, Richard Cochran wrote:

This patch needs to be split in pieces. The syscall change is totally
unrelated to the dynamic clock id creation. Though I do not like
either of them. :)

> 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, the clock ID,
> and a pointer to a struct timex. The semantics of the timex struct
> have been expanded by one additional mode flag, which allows an
> absolute offset correction. When specificied, the clock offset is
> immediately corrected by skipping to the new time value.

And why do we need a separate syscall for this?

> In addition, the POSIX clock code has been augmented to offer a
> dynamic clock creation method. Instead of registering a hard
> coded clock ID, modules may call create_posix_clock(), which
> returns a new clock ID.

This has been discussed for years and I still fail to see the
requirement for this. The only result is that it allows folks to
create their special purpose clock stuff and keep it out of tree
instead of fixing the problems they have with the existing clock
infrastructure in the kernel.

As far as I understood from the previous discussions, the final goal
is to provide PTP support, right?

But what I see is an approach which tries to implement disconnected
special purpose clocks which have the ability to be adjusted
independently. What's the purpose of this ? Why can't we just use the
existing clocks and make PTP work on them ?

I know that lots of embedded folks think that they need their special
timers and extra magic to make stuff work, but that's the wrong
approach.

What's wrong with the existing clocks? Nothing, except that we have no
way to sync CLOCK_MONOTONIC across several machines. And that's what
you really want if you try to do distributed control and data
acquisition stuff.

That's a single CLOCK_MONOTONIC_GLOBAL and not a bunch of completely
disconnected clock implementations with random clock ids and random
feature sets.

Thoughts ?


	 tglx

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Richard Cochran]

On Thu, Sep 09, 2010 at 12:49:27PM +0200, Thomas Gleixner wrote:
> On Fri, 3 Sep 2010, Richard Cochran wrote:
> 
> This patch needs to be split in pieces. The syscall change is totally
> unrelated to the dynamic clock id creation. Though I do not like
> either of them. :)

That is not a problem. Splitting up the patch, I mean.

> > 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, the clock ID,
> > and a pointer to a struct timex. The semantics of the timex struct
> > have been expanded by one additional mode flag, which allows an
> > absolute offset correction. When specificied, the clock offset is
> > immediately corrected by skipping to the new time value.
> 
> And why do we need a separate syscall for this?

Because we cannot, in general, offer PTP hardware clocks as clock
sources. We need to tune the PTP hardware clock, even if there is no
connection to the Linux kernel system time.

> > In addition, the POSIX clock code has been augmented to offer a
> > dynamic clock creation method. Instead of registering a hard
> > coded clock ID, modules may call create_posix_clock(), which
> > returns a new clock ID.
> 
> This has been discussed for years and I still fail to see the
> requirement for this. The only result is that it allows folks to
> create their special purpose clock stuff and keep it out of tree
> instead of fixing the problems they have with the existing clock
> infrastructure in the kernel.

Do you have any pointers to this discussion?

> But what I see is an approach which tries to implement disconnected
> special purpose clocks which have the ability to be adjusted
> independently. What's the purpose of this ? Why can't we just use the
> existing clocks and make PTP work on them ?
> 
> I know that lots of embedded folks think that they need their special
> timers and extra magic to make stuff work, but that's the wrong
> approach.

Its not just embedded who want better synchronization, but also big
iron for microtrading on Wall Street, for example.

> What's wrong with the existing clocks? Nothing, except that we have no
> way to sync CLOCK_MONOTONIC across several machines. And that's what
> you really want if you try to do distributed control and data
> acquisition stuff.
> 
> That's a single CLOCK_MONOTONIC_GLOBAL and not a bunch of completely
> disconnected clock implementations with random clock ids and random
> feature sets.
> 
> Thoughts ?

There isn't really anything wrong with the existing clock
infrastructure, in my view. I think the stumbling block is the idea
that there can be more than one clock in a computer system, and that
user space needs access to more than just one of them.

It is a fact that PTP hardware clocks are separate from the system
clock, and this situation will presist for some time, if not
indefinitely. It is ironic that the very best PTP clocks, the PHY
clocks, are the farthest away from the system clock.

Using PTP (or any disributed time protocol, eg NTP) involves a number
of options and choices. This complexity belongs in user space. The
kernel should simply offer a way to access the hardware clocks
(mechanism, not policy). For NTP, the kernel has to have a special
role running the clock servo, but this is an exception.

Of course, the kernel wants to present a consistent system time to
user space, hiding the ugly clock details. However, when it comes to
PTP hardware clocks, the kernel needs a little help.  Only one
program, lets call it the ptpd, needs to know about the PTP
clock. What this program does depends on the operational mode and on
the user's preferences.

What follows uses the posix clock api idea just to illustrate. You
could just as well use chardev ioctls. I am not arguing about the
API. Rather I am trying to explain why the kernel must expose multiple
clocks to user space.

1. Master with external time source (like GPS)

   Using the PPS subsystem, the system time is latched on the 1 PPS
   from the GPS. Using the PTP external timestamp feature, the PTP
   clock time is also latched.  The ptpd then adjusts *both* the
   kernel time and the PTP clock time.

   systime = get_pps();
   adj = servo(systime);
   clock_adjtime(clock_realtime, adj);

   ptptime = ptp_external_timestamp();
   adj = servo(ptptime);
   clock_adjtime(clock_ptp, adj);

2. Master with PTP clock as time source

   In this case, there is no external reference clock, and we know
   that the PTP clock's oscillator is more stable than the
   system's. The ptpd enables the 1 PPS from the PTP clock and adjusts
   the system clock according to the latched system time.

   t = get_pps();
   adj = servo(t);
   clock_adjtime(clock_realtime, adj);

3. Master with kernel as time source

   In this case, we are using the system time (which could be from an
   oven quartz, for example). The ptpd enables the 1 PPS from the PTP
   clock and adjusts the PTP clock according to the latched system
   time.

   t = get_pps();
   adj = servo(t);
   clock_adjtime(clock_ptp, adj);

4. Slave with PPS hook

   Here we want to synchronize the system and the PTP clock to a
   remote clock. The ptpd uses timestamps on network packets to feed a
   servo that controls the PTP clock. The ptpd also enables the 1 PPS
   from the PTP clock and adjusts the system clock according to the
   latched system time (like case 2).

In all of these examples, most userland programs will not be aware of
what is going on, just like when NTP is used. Only the ptpd knows that
there are multiple clocks, and that program really *does* need access
to the various clocks.

Finally, there is one case which is dumb from a hardware design point
of view, but still possible. Lets say that we have a PHY based PTP
clock witnout any interrupt to the CPU. You could still use such a
computer in a distributed application by just ignoring the wrong
system time, provided that the kernel offers a way to control the PTP
hardware clock.

Richard

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Thomas Gleixner]

Richard,

On Thu, 9 Sep 2010, Richard Cochran wrote:

> On Thu, Sep 09, 2010 at 12:49:27PM +0200, Thomas Gleixner wrote:
> > On Fri, 3 Sep 2010, Richard Cochran wrote:
> > > In addition, the POSIX clock code has been augmented to offer a
> > > dynamic clock creation method. Instead of registering a hard
> > > coded clock ID, modules may call create_posix_clock(), which
> > > returns a new clock ID.
> > 
> > This has been discussed for years and I still fail to see the
> > requirement for this. The only result is that it allows folks to
> > create their special purpose clock stuff and keep it out of tree
> > instead of fixing the problems they have with the existing clock
> > infrastructure in the kernel.
> 
> Do you have any pointers to this discussion?

Not out of the box. Need to ask the oracle of google [ I wonder if
this expression is politically correct today :) ]

My personal stance on this is clear: Assign fixed ID.

The point is, that if we provide something like CLOCK_PTP, then we can
abstract the real hardware drivers behind this clock id and we get a
consistent feature set for these drivers and a consistent behaviour on
the user space interface. There still might be a hardware driver which
cannot provide a specific feature, but there is nothing wrong to
return -ENOSYS in such a case.

> There isn't really anything wrong with the existing clock
> infrastructure, in my view. I think the stumbling block is the idea
> that there can be more than one clock in a computer system, and that
> user space needs access to more than just one of them.
> 
> It is a fact that PTP hardware clocks are separate from the system
> clock, and this situation will presist for some time, if not
> indefinitely. It is ironic that the very best PTP clocks, the PHY
> clocks, are the farthest away from the system clock.

In terms of hardware, yes.
 
> Using PTP (or any disributed time protocol, eg NTP) involves a number
> of options and choices. This complexity belongs in user space. The
> kernel should simply offer a way to access the hardware clocks
> (mechanism, not policy). For NTP, the kernel has to have a special

I completely agree with that.

> role running the clock servo, but this is an exception.
> 
> Of course, the kernel wants to present a consistent system time to
> user space, hiding the ugly clock details. However, when it comes to
> PTP hardware clocks, the kernel needs a little help.  Only one
> program, lets call it the ptpd, needs to know about the PTP
> clock. What this program does depends on the operational mode and on
> the user's preferences.

No objections
 
> What follows uses the posix clock api idea just to illustrate. You
> could just as well use chardev ioctls. I am not arguing about the
> API. Rather I am trying to explain why the kernel must expose multiple
> clocks to user space.
> 
> 1. Master with external time source (like GPS)
> 2. Master with PTP clock as time source
> 3. Master with kernel as time source
> 4. Slave with PPS hook

Ok, that makes a lot of sense now. Thanks for taking the time and
shedding some light on me!

So you want to utilize the posix-timer infrastructure to make all this
happen. This new clock basically needs clock_[get|set]time plus the
new clock_adjust syscall, nothing more. The normal application stuff
will still use what's already there, right ?

If that's the case, then I'm not going to stand in the way, except for
some implementation details like the new syscall clock_adjust: 

Please make this thing new from ground up and not just a modified copy
of sys_adjtimex.

 1) This avoids the compat syscall crap

 2) It allows you to add the extra control stuff for your PTP magic
     (PPS, whatever) which you would need to expose by some other
     means otherwise.

Thanks,

	tglx

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Thomas Gleixner]

On Thu, 9 Sep 2010, Thomas Gleixner wrote:
> On Thu, 9 Sep 2010, Richard Cochran wrote:
> 
> > On Thu, Sep 09, 2010 at 12:49:27PM +0200, Thomas Gleixner wrote:
> > > On Fri, 3 Sep 2010, Richard Cochran wrote:
> > > > In addition, the POSIX clock code has been augmented to offer a
> > > > dynamic clock creation method. Instead of registering a hard
> > > > coded clock ID, modules may call create_posix_clock(), which
> > > > returns a new clock ID.
> > > 
> > > This has been discussed for years and I still fail to see the
> > > requirement for this. The only result is that it allows folks to
> > > create their special purpose clock stuff and keep it out of tree
> > > instead of fixing the problems they have with the existing clock
> > > infrastructure in the kernel.
> > 
> > Do you have any pointers to this discussion?
> 
> Not out of the box. Need to ask the oracle of google [ I wonder if
> this expression is politically correct today :) ]
> 
> My personal stance on this is clear: Assign fixed ID.
> 
> The point is, that if we provide something like CLOCK_PTP, then we can
> abstract the real hardware drivers behind this clock id and we get a
> consistent feature set for these drivers and a consistent behaviour on
> the user space interface. There still might be a hardware driver which
> cannot provide a specific feature, but there is nothing wrong to
> return -ENOSYS in such a case.

Hmm. Talked to John Stultz about this and got enlightened that there
might be more than one of these beasts. That changes the story
slightly.
 
So yes, I've been wrong as usual and we'll need some way of assigning
those ids dynamically, but I'm still opposed to providing an
unconfined "give me one of those id's" interface for public
consumption.

I'd rather see a controlled environment for device classes like PTP
clocks. That would have a couple of advantages:

 - clear association of the device to a well defined functionality

 - avoidance of duplicated code

 - consistent sysfs interfaces for functionality which is device class
   specific

 - simpler identification for interested applications

 - preventing the random spread of clock id consumers

So the clock device class code would provide the interface for these
class specific hardware drivers and consult the posix timer core code
to give out an id.

And that would apply to any other class of clock devices which do not
fall into the general clocksource category (e.g. RTCs, audio clocks
..)

Thoughts ?

	 tglx

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[john stultz]

On Thu, 2010-09-09 at 23:16 +0200, Thomas Gleixner wrote:
> Hmm. Talked to John Stultz about this and got enlightened that there
> might be more than one of these beasts. That changes the story
> slightly.
> 
> So yes, I've been wrong as usual and we'll need some way of assigning
> those ids dynamically, but I'm still opposed to providing an
> unconfined "give me one of those id's" interface for public
> consumption.
> 
> I'd rather see a controlled environment for device classes like PTP
> clocks. That would have a couple of advantages:
> 
>  - clear association of the device to a well defined functionality
> 
>  - avoidance of duplicated code
> 
>  - consistent sysfs interfaces for functionality which is device class
>    specific
> 
>  - simpler identification for interested applications
> 
>  - preventing the random spread of clock id consumers
> 
> So the clock device class code would provide the interface for these
> class specific hardware drivers and consult the posix timer core code
> to give out an id.
> 
> And that would apply to any other class of clock devices which do not
> fall into the general clocksource category (e.g. RTCs, audio clocks
> ..)
> 
> Thoughts ?

So at first I was concerned, because I thought you were suggesting we
distinguish between the classes of clocks in our userspace interface. 

But after some clarification on IRC, you made it clear that you want
some generic infrastructure that supports what the PTP driver needs that
would then be re-usable by other clocks we want to similarly expose to
userland via a clockid.

So basically you want something akin to the struct clocksource for the
driver to register that would minimize code duplication.

My initial reaction, is that we already have the k_clock structure (from
include/linux/posix-timers.h):
struct k_clock {
	int res;		/* in nanoseconds */
	int (*clock_getres) (const clockid_t which_clock, struct timespec *tp);
	int (*clock_set) (const clockid_t which_clock, struct timespec * tp);
	int (*clock_get) (const clockid_t which_clock, struct timespec * tp);
	int (*timer_create) (struct k_itimer *timer);
	int (*nsleep) (const clockid_t which_clock, int flags,
		       struct timespec *, struct timespec __user *);
	long (*nsleep_restart) (struct restart_block *restart_block);
	int (*timer_set) (struct k_itimer * timr, int flags,
			  struct itimerspec * new_setting,
			  struct itimerspec * old_setting);
	int (*timer_del) (struct k_itimer * timr);
	void (*timer_get) (struct k_itimer * timr,
			   struct itimerspec * cur_setting);
};


This is still very close to the level of the posix clocks/timers api, so
maybe you're wanting something a little lower level, possibly as low as
the clocksource/clockevent structures? 

The only problem there is from the examples, it seems some PTP hardware
can be fairly self-contained. So its a actual ns clock that can be freq
adjusted in hardware, not a constant freq counter that is then converted
to nanoseconds and freq managed in software. So the interface probably
can't be quite as low-level as the clocksource/clockevent structures.

In fact, from the example drivers posted already, it looks like the
k_clock structure maps fairly close to what the hardware uses.

Other clock types that we may want to expose, such as the RTC also can
map fairly close to k_clock. The audio clocks may need some research, as
I suspect they're just a clocksource style counter, so some additional
software cycle->ns layering similar to the timekeeping core (but likely
much simpler) may be needed.

So the question to Richard is, what does the above k_clock not provide
that the PTP driver needs?  If you've already made some stabs at it,
seeing an example of the generic header and a trivial example driver
that includes the k_clock structure, so we can see what it removed from
your earlier chardev implementation would be helpful.

thanks
-john

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Richard Cochran]

On Thu, Sep 09, 2010 at 03:53:13PM -0700, john stultz wrote:
> The only problem there is from the examples, it seems some PTP hardware
> can be fairly self-contained. So its a actual ns clock that can be freq
> adjusted in hardware, not a constant freq counter that is then converted
> to nanoseconds and freq managed in software. So the interface probably
> can't be quite as low-level as the clocksource/clockevent structures.
> 
> In fact, from the example drivers posted already, it looks like the
> k_clock structure maps fairly close to what the hardware uses.

Yes, thats right. The PTP hardware clocks of which I know all follow
the same pattern.

> Other clock types that we may want to expose, such as the RTC also can
> map fairly close to k_clock. The audio clocks may need some research, as
> I suspect they're just a clocksource style counter, so some additional
> software cycle->ns layering similar to the timekeeping core (but likely
> much simpler) may be needed.

We may also find clocks that are free running counters which can
timestamp network packets. In that case, one would also need software
support to implement rate adjustment. I have not seen that yet among
PTP hardware clocks, so I think we should cross that bridge when we
come to it. In any case, the proposed infrastructure does not exclude
the possibility to support that kind of clock.

> So the question to Richard is, what does the above k_clock not provide
> that the PTP driver needs?

Yes.

> If you've already made some stabs at it,
> seeing an example of the generic header and a trivial example driver
> that includes the k_clock structure, so we can see what it removed from
> your earlier chardev implementation would be helpful.

Okay, it seems that we are gradually coming around to a workable
solution. I will repost the posix clock idea including the PTP clock
drivers, so you all can see how it would fit together.

Thanks,
Richard

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[john stultz]

On Thu, 2010-09-09 at 12:49 +0200, Thomas Gleixner wrote:
> On Fri, 3 Sep 2010, Richard Cochran wrote:
> 
> This patch needs to be split in pieces. The syscall change is totally
> unrelated to the dynamic clock id creation. Though I do not like
> either of them. :)
> 
> > 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, the clock ID,
> > and a pointer to a struct timex. The semantics of the timex struct
> > have been expanded by one additional mode flag, which allows an
> > absolute offset correction. When specificied, the clock offset is
> > immediately corrected by skipping to the new time value.
> 
> And why do we need a separate syscall for this?
> 
> > In addition, the POSIX clock code has been augmented to offer a
> > dynamic clock creation method. Instead of registering a hard
> > coded clock ID, modules may call create_posix_clock(), which
> > returns a new clock ID.
> 
> This has been discussed for years and I still fail to see the
> requirement for this. The only result is that it allows folks to
> create their special purpose clock stuff and keep it out of tree
> instead of fixing the problems they have with the existing clock
> infrastructure in the kernel.
> 
> As far as I understood from the previous discussions, the final goal
> is to provide PTP support, right?
> 
> But what I see is an approach which tries to implement disconnected
> special purpose clocks which have the ability to be adjusted
> independently. What's the purpose of this ? Why can't we just use the
> existing clocks and make PTP work on them ?

So this too was my initial gut response. It seems ridiculous to expose
two clock_ids (CLOCK_REALTIME and CLOCK_PTP)that conceptually represent
the same thing (ie: number of seconds,nanoseconds since 1970).

It doesn't help that one of the use cases that Richard suggests is "for
example in an embedded control application. The userland software can
simply ignore the wrong system time." 

As someone who's spent a *lot* of time trying to fix the "wrong system
time" these use cases reek of work-around solutions instead of properly
fixing whatever keeps the system time from being accurately sycned.

However, as I've worked on understanding the issue, I realize that there
is some validity to needing to expose more then one hardware clock the
conceptually is the same as CLOCK_REALTIME. And that most of my gut
reaction to this was me being a bit oversensitive. :)

> I know that lots of embedded folks think that they need their special
> timers and extra magic to make stuff work, but that's the wrong
> approach.
> 
> What's wrong with the existing clocks? Nothing, except that we have no
> way to sync CLOCK_MONOTONIC across several machines. And that's what
> you really want if you try to do distributed control and data
> acquisition stuff.

Err.. s/CLOCK_MONOTONIC/CLOCK_REALTIME/ :)

> That's a single CLOCK_MONOTONIC_GLOBAL and not a bunch of completely
> disconnected clock implementations with random clock ids and random
> feature sets.

Specifically, because the way the correction feedback loops work with
PTP, working against the hardware clock that is generating the
timestamps directly is going to get the best results. 

Additionally, it allows multiple syncing methods to be tried in
userland, rather then trying jam it all in the kernel in order to make
it look like there is only one global system time. The parallel with
audio hardware is also valid, as we currently ignore any skew between
audio hardware clocks and system time, as there's no useful way to
expose those clocks (that I'm aware of atleast, not too familiar with
alsa, but my googling didn't show much).

However, since there may be multiple PTP clocks or audio clocks or
whatever, allocating static clockids for each type isn't quite useful,
as we need to deal with mapping the clockids to hardware. So exposing
the clockids via sysfs allows us to understand the physical mapping of
what bus and device the clock hangs off of.

Now, I'm not a fan of the clockid directory that Richard proposed, I'd
rather add a clock_id to the devices sysfs entry. But these are details
and can be worked out as the patch gets refined.


thanks
-john

Re: [PATCH 1/2] posix clocks: introduce a syscall for clock tuning.

[Thomas Gleixner]

On Thu, 9 Sep 2010, john stultz wrote:
> On Thu, 2010-09-09 at 12:49 +0200, Thomas Gleixner wrote:
> > But what I see is an approach which tries to implement disconnected
> > special purpose clocks which have the ability to be adjusted
> > independently. What's the purpose of this ? Why can't we just use the
> > existing clocks and make PTP work on them ?
> 
> So this too was my initial gut response. It seems ridiculous to expose
> two clock_ids (CLOCK_REALTIME and CLOCK_PTP)that conceptually represent
> the same thing (ie: number of seconds,nanoseconds since 1970).
> 
> It doesn't help that one of the use cases that Richard suggests is "for
> example in an embedded control application. The userland software can
> simply ignore the wrong system time." 
> 
> As someone who's spent a *lot* of time trying to fix the "wrong system
> time" these use cases reek of work-around solutions instead of properly
> fixing whatever keeps the system time from being accurately sycned.
> 
> However, as I've worked on understanding the issue, I realize that there
> is some validity to needing to expose more then one hardware clock the
> conceptually is the same as CLOCK_REALTIME. And that most of my gut
> reaction to this was me being a bit oversensitive. :)

Yup. It still scares me that we might end up with a dozen different
notions of ONE second elapsed on the same machine :)

> However, since there may be multiple PTP clocks or audio clocks or
> whatever, allocating static clockids for each type isn't quite useful,

Yeah, I corrected myself on that one, but I really want to see some
confinement into well defined clock classes rather than the "hooray
here is my clock of the day" approach.

Thanks,

	tglx

[PATCH 2/2] posix clocks: introduce a sysfs presence.

[Richard Cochran]

This patch adds a 'timesource' class into sysfs. Each registered POSIX
clock appears by name under /sys/class/timesource. The idea is to
expose to user space the dynamic mapping between clock devices and
clock IDs.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 Documentation/ABI/testing/sysfs-timesource |   24 ++++++++++++++++
 drivers/char/mmtimer.c                     |    1 +
 include/linux/posix-timers.h               |    4 +++
 kernel/posix-cpu-timers.c                  |    2 +
 kernel/posix-timers.c                      |   40 ++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-timesource

diff --git a/Documentation/ABI/testing/sysfs-timesource b/Documentation/ABI/testing/sysfs-timesource
new file mode 100644
index 0000000..f991de2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-timesource
@@ -0,0 +1,24 @@
+What:		/sys/class/timesource/
+Date:		September 2010
+Contact:	Richard Cochran <richardcochran@gmail.com>
+Description:
+		This directory contains files and directories
+		providing a standardized interface to the available
+		time sources.
+
+What:		/sys/class/timesource/<name>/
+Date:		September 2010
+Contact:	Richard Cochran <richardcochran@gmail.com>
+Description:
+		This directory contains the attributes of a time
+		source registered with the POSIX clock subsystem.
+
+What:		/sys/class/timesource/<name>/id
+Date:		September 2010
+Contact:	Richard Cochran <richardcochran@gmail.com>
+Description:
+		This file contains the clock ID (a non-negative
+		integer) of the named time source registered with the
+		POSIX clock subsystem. This value may be passed as the
+		first argument to the POSIX clock and timer system
+		calls. See man CLOCK_GETRES(2) and TIMER_CREATE(2).
diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
index ea7c99f..e9173e3 100644
--- a/drivers/char/mmtimer.c
+++ b/drivers/char/mmtimer.c
@@ -758,6 +758,7 @@ static int sgi_timer_set(struct k_itimer *timr, int flags,
 }
 
 static struct k_clock sgi_clock = {
+	.name = "sgi_cycle",
 	.res = 0,
 	.clock_set = sgi_clock_set,
 	.clock_get = sgi_clock_get,
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 08aa4da..64e6fee 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -67,7 +67,11 @@ struct k_itimer {
 	} it;
 };
 
+#define KCLOCK_MAX_NAME 32
+
 struct k_clock {
+	char name[KCLOCK_MAX_NAME];
+	struct device *dev;
 	clockid_t id;
 	int res;		/* in nanoseconds */
 	int (*clock_getres) (const clockid_t which_clock, struct timespec *tp);
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index e1c2e7b..df9cbab 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -1611,6 +1611,7 @@ static long thread_cpu_nsleep_restart(struct restart_block *restart_block)
 static __init int init_posix_cpu_timers(void)
 {
 	struct k_clock process = {
+		.name = "process_cputime",
 		.clock_getres = process_cpu_clock_getres,
 		.clock_get = process_cpu_clock_get,
 		.clock_set = do_posix_clock_nosettime,
@@ -1619,6 +1620,7 @@ static __init int init_posix_cpu_timers(void)
 		.nsleep_restart = process_cpu_nsleep_restart,
 	};
 	struct k_clock thread = {
+		.name = "thread_cputime",
 		.clock_getres = thread_cpu_clock_getres,
 		.clock_get = thread_cpu_clock_get,
 		.clock_set = do_posix_clock_nosettime,
diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 67fba5c..719aa11 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -46,6 +46,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/device.h>
 
 /*
  * Management arrays for POSIX timers.	 Timers are kept in slab memory
@@ -135,6 +136,8 @@ static struct k_clock posix_clocks[MAX_CLOCKS];
 static DECLARE_BITMAP(clocks_map, MAX_CLOCKS);
 static DEFINE_MUTEX(clocks_mux); /* protects 'posix_clocks' and 'clocks_map' */
 
+static struct class *timesource_class;
+
 /*
  * These ones are defined below.
  */
@@ -271,20 +274,40 @@ static int posix_get_coarse_res(const clockid_t which_clock, struct timespec *tp
 	*tp = ktime_to_timespec(KTIME_LOW_RES);
 	return 0;
 }
+
+/*
+ * sysfs attributes
+ */
+
+static ssize_t show_clock_id(struct device *dev,
+			     struct device_attribute *attr, char *page)
+{
+	struct k_clock *kc = dev_get_drvdata(dev);
+	return snprintf(page, PAGE_SIZE-1, "%d\n", kc->id);
+}
+
+static struct device_attribute timesource_dev_attrs[] = {
+	__ATTR(id,   0444, show_clock_id,   NULL),
+	__ATTR_NULL,
+};
+
 /*
  * Initialize everything, well, just everything in Posix clocks/timers ;)
  */
 static __init int init_posix_timers(void)
 {
 	struct k_clock clock_realtime = {
+		.name = "realtime",
 		.clock_getres = hrtimer_get_res,
 	};
 	struct k_clock clock_monotonic = {
+		.name = "monotonic",
 		.clock_getres = hrtimer_get_res,
 		.clock_get = posix_ktime_get_ts,
 		.clock_set = do_posix_clock_nosettime,
 	};
 	struct k_clock clock_monotonic_raw = {
+		.name = "monotonic_raw",
 		.clock_getres = hrtimer_get_res,
 		.clock_get = posix_get_monotonic_raw,
 		.clock_set = do_posix_clock_nosettime,
@@ -292,6 +315,7 @@ static __init int init_posix_timers(void)
 		.nsleep = no_nsleep,
 	};
 	struct k_clock clock_realtime_coarse = {
+		.name = "realtime_coarse",
 		.clock_getres = posix_get_coarse_res,
 		.clock_get = posix_get_realtime_coarse,
 		.clock_set = do_posix_clock_nosettime,
@@ -299,6 +323,7 @@ static __init int init_posix_timers(void)
 		.nsleep = no_nsleep,
 	};
 	struct k_clock clock_monotonic_coarse = {
+		.name = "monotonic_coarse",
 		.clock_getres = posix_get_coarse_res,
 		.clock_get = posix_get_monotonic_coarse,
 		.clock_set = do_posix_clock_nosettime,
@@ -306,6 +331,13 @@ static __init int init_posix_timers(void)
 		.nsleep = no_nsleep,
 	};
 
+	timesource_class = class_create(NULL, "timesource");
+	if (IS_ERR(timesource_class)) {
+		pr_err("posix-timers: failed to allocate timesource class\n");
+		return PTR_ERR(timesource_class);
+	}
+	timesource_class->dev_attrs = timesource_dev_attrs;
+
 	register_posix_clock(CLOCK_REALTIME, &clock_realtime);
 	register_posix_clock(CLOCK_MONOTONIC, &clock_monotonic);
 	register_posix_clock(CLOCK_MONOTONIC_RAW, &clock_monotonic_raw);
@@ -500,6 +532,14 @@ int register_posix_clock(const clockid_t id, struct k_clock *clock)
 	kc = &posix_clocks[id];
 	*kc = *clock;
 	kc->id = id;
+	kc->dev = device_create(timesource_class, NULL, MKDEV(0, 0),
+				kc, "%s", kc->name);
+	if (IS_ERR(kc->dev)) {
+		pr_err("failed to create device clock_id %d\n", id);
+		err = PTR_ERR(kc->dev);
+		goto out;
+	}
+	dev_set_drvdata(kc->dev, kc);
 	set_bit(id, clocks_map);
 out:
 	mutex_unlock(&clocks_mux);
-- 
1.7.0.4

Re: [PATCH 2/2] posix clocks: introduce a sysfs presence.

[john stultz]

On Fri, Sep 3, 2010 at 2:30 AM, Richard Cochran
<richardcochran@gmail.com> wrote:
> This patch adds a 'timesource' class into sysfs. Each registered POSIX
> clock appears by name under /sys/class/timesource. The idea is to
> expose to user space the dynamic mapping between clock devices and
> clock IDs.

So I'm not a fan of this one. Timesource is way to close to
clocksource. Really something like "posix_clockid" would make way more
sense.

But really, I'd prefer we not consolidate the clock_ids into a
dictionary style interface, where we list them all.

Really, I'd much prefer that there be a clockid attribute that hangs
off of the sysfs entry for the device where the PTP clock resides (or
is connected to).  That way we solve the issue of how do we map the
device to the clockid, and provide a method for ptpd to get the
clockid.

It also makes these special ids a little less easy to find for normal
applications (which should be ok, as only special applications that
really need to know the underlying hardware values should be mucking
with these clockids).

thanks
-john


> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
> ---
>  Documentation/ABI/testing/sysfs-timesource |   24 ++++++++++++++++
>  drivers/char/mmtimer.c                     |    1 +
>  include/linux/posix-timers.h               |    4 +++
>  kernel/posix-cpu-timers.c                  |    2 +
>  kernel/posix-timers.c                      |   40 ++++++++++++++++++++++++++++
>  5 files changed, 71 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-timesource
>
> diff --git a/Documentation/ABI/testing/sysfs-timesource b/Documentation/ABI/testing/sysfs-timesource
> new file mode 100644
> index 0000000..f991de2
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-timesource
> @@ -0,0 +1,24 @@
> +What:          /sys/class/timesource/
> +Date:          September 2010
> +Contact:       Richard Cochran <richardcochran@gmail.com>
> +Description:
> +               This directory contains files and directories
> +               providing a standardized interface to the available
> +               time sources.
> +
> +What:          /sys/class/timesource/<name>/
> +Date:          September 2010
> +Contact:       Richard Cochran <richardcochran@gmail.com>
> +Description:
> +               This directory contains the attributes of a time
> +               source registered with the POSIX clock subsystem.
> +
> +What:          /sys/class/timesource/<name>/id
> +Date:          September 2010
> +Contact:       Richard Cochran <richardcochran@gmail.com>
> +Description:
> +               This file contains the clock ID (a non-negative
> +               integer) of the named time source registered with the
> +               POSIX clock subsystem. This value may be passed as the
> +               first argument to the POSIX clock and timer system
> +               calls. See man CLOCK_GETRES(2) and TIMER_CREATE(2).
> diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c
> index ea7c99f..e9173e3 100644
> --- a/drivers/char/mmtimer.c
> +++ b/drivers/char/mmtimer.c
> @@ -758,6 +758,7 @@ static int sgi_timer_set(struct k_itimer *timr, int flags,
>  }
>
>  static struct k_clock sgi_clock = {
> +       .name = "sgi_cycle",
>        .res = 0,
>        .clock_set = sgi_clock_set,
>        .clock_get = sgi_clock_get,
> diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
> index 08aa4da..64e6fee 100644
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -67,7 +67,11 @@ struct k_itimer {
>        } it;
>  };
>
> +#define KCLOCK_MAX_NAME 32
> +
>  struct k_clock {
> +       char name[KCLOCK_MAX_NAME];
> +       struct device *dev;
>        clockid_t id;
>        int res;                /* in nanoseconds */
>        int (*clock_getres) (const clockid_t which_clock, struct timespec *tp);
> diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
> index e1c2e7b..df9cbab 100644
> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -1611,6 +1611,7 @@ static long thread_cpu_nsleep_restart(struct restart_block *restart_block)
>  static __init int init_posix_cpu_timers(void)
>  {
>        struct k_clock process = {
> +               .name = "process_cputime",
>                .clock_getres = process_cpu_clock_getres,
>                .clock_get = process_cpu_clock_get,
>                .clock_set = do_posix_clock_nosettime,
> @@ -1619,6 +1620,7 @@ static __init int init_posix_cpu_timers(void)
>                .nsleep_restart = process_cpu_nsleep_restart,
>        };
>        struct k_clock thread = {
> +               .name = "thread_cputime",
>                .clock_getres = thread_cpu_clock_getres,
>                .clock_get = thread_cpu_clock_get,
>                .clock_set = do_posix_clock_nosettime,
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 67fba5c..719aa11 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -46,6 +46,7 @@
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
>  #include <linux/module.h>
> +#include <linux/device.h>
>
>  /*
>  * Management arrays for POSIX timers.  Timers are kept in slab memory
> @@ -135,6 +136,8 @@ static struct k_clock posix_clocks[MAX_CLOCKS];
>  static DECLARE_BITMAP(clocks_map, MAX_CLOCKS);
>  static DEFINE_MUTEX(clocks_mux); /* protects 'posix_clocks' and 'clocks_map' */
>
> +static struct class *timesource_class;
> +
>  /*
>  * These ones are defined below.
>  */
> @@ -271,20 +274,40 @@ static int posix_get_coarse_res(const clockid_t which_clock, struct timespec *tp
>        *tp = ktime_to_timespec(KTIME_LOW_RES);
>        return 0;
>  }
> +
> +/*
> + * sysfs attributes
> + */
> +
> +static ssize_t show_clock_id(struct device *dev,
> +                            struct device_attribute *attr, char *page)
> +{
> +       struct k_clock *kc = dev_get_drvdata(dev);
> +       return snprintf(page, PAGE_SIZE-1, "%d\n", kc->id);
> +}
> +
> +static struct device_attribute timesource_dev_attrs[] = {
> +       __ATTR(id,   0444, show_clock_id,   NULL),
> +       __ATTR_NULL,
> +};
> +
>  /*
>  * Initialize everything, well, just everything in Posix clocks/timers ;)
>  */
>  static __init int init_posix_timers(void)
>  {
>        struct k_clock clock_realtime = {
> +               .name = "realtime",
>                .clock_getres = hrtimer_get_res,
>        };
>        struct k_clock clock_monotonic = {
> +               .name = "monotonic",
>                .clock_getres = hrtimer_get_res,
>                .clock_get = posix_ktime_get_ts,
>                .clock_set = do_posix_clock_nosettime,
>        };
>        struct k_clock clock_monotonic_raw = {
> +               .name = "monotonic_raw",
>                .clock_getres = hrtimer_get_res,
>                .clock_get = posix_get_monotonic_raw,
>                .clock_set = do_posix_clock_nosettime,
> @@ -292,6 +315,7 @@ static __init int init_posix_timers(void)
>                .nsleep = no_nsleep,
>        };
>        struct k_clock clock_realtime_coarse = {
> +               .name = "realtime_coarse",
>                .clock_getres = posix_get_coarse_res,
>                .clock_get = posix_get_realtime_coarse,
>                .clock_set = do_posix_clock_nosettime,
> @@ -299,6 +323,7 @@ static __init int init_posix_timers(void)
>                .nsleep = no_nsleep,
>        };
>        struct k_clock clock_monotonic_coarse = {
> +               .name = "monotonic_coarse",
>                .clock_getres = posix_get_coarse_res,
>                .clock_get = posix_get_monotonic_coarse,
>                .clock_set = do_posix_clock_nosettime,
> @@ -306,6 +331,13 @@ static __init int init_posix_timers(void)
>                .nsleep = no_nsleep,
>        };
>
> +       timesource_class = class_create(NULL, "timesource");
> +       if (IS_ERR(timesource_class)) {
> +               pr_err("posix-timers: failed to allocate timesource class\n");
> +               return PTR_ERR(timesource_class);
> +       }
> +       timesource_class->dev_attrs = timesource_dev_attrs;
> +
>        register_posix_clock(CLOCK_REALTIME, &clock_realtime);
>        register_posix_clock(CLOCK_MONOTONIC, &clock_monotonic);
>        register_posix_clock(CLOCK_MONOTONIC_RAW, &clock_monotonic_raw);
> @@ -500,6 +532,14 @@ int register_posix_clock(const clockid_t id, struct k_clock *clock)
>        kc = &posix_clocks[id];
>        *kc = *clock;
>        kc->id = id;
> +       kc->dev = device_create(timesource_class, NULL, MKDEV(0, 0),
> +                               kc, "%s", kc->name);
> +       if (IS_ERR(kc->dev)) {
> +               pr_err("failed to create device clock_id %d\n", id);
> +               err = PTR_ERR(kc->dev);
> +               goto out;
> +       }
> +       dev_set_drvdata(kc->dev, kc);
>        set_bit(id, clocks_map);
>  out:
>        mutex_unlock(&clocks_mux);
> --
> 1.7.0.4
>
> --
> 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/
>

Re: [PATCH 2/2] posix clocks: introduce a sysfs presence.

[Alan Cox]

On Fri, 3 Sep 2010 11:30:13 +0200
Richard Cochran <richardcochran@gmail.com> wrote:

> This patch adds a 'timesource' class into sysfs. Each registered POSIX
> clock appears by name under /sys/class/timesource. The idea is to
> expose to user space the dynamic mapping between clock devices and
> clock IDs.

This is intrinsically racy so not a good API at all.

By the time I have acquired an id from sysfs it may have been re-issued
or changed.

POSIX habit of enumerating objects not properties of objects is one of
the brain dead aspects of POSIX that we should have nothing to do with.

Also we have existing time sources that don't follow the poxix clock
model - I can open /dev/rtc and I can open the hpet and so on.

I like /sys/class/time* *but* you need to be able to open the sysfs
device and apply operations to it in order for it to work when your closk
can be dynamically created and destroyed and to get a sane Unix API.

To start with try applying permissions to clock sources via the POSIX
API. That is something that will be required for some applications.

I need to be able to open sys/clock/foo/something and get a meaningful
handle. Sure it's quite likely the operations it supports are related to
the POSIX timer ops.

Alan

Re: [PATCH 2/2] posix clocks: introduce a sysfs presence.

[Richard Cochran]

On Fri, Sep 10, 2010 at 12:00:08AM +0100, Alan Cox wrote:
> Also we have existing time sources that don't follow the poxix clock
> model - I can open /dev/rtc and I can open the hpet and so on.
> 
> I like /sys/class/time* *but* you need to be able to open the sysfs
> device and apply operations to it in order for it to work when your closk
> can be dynamically created and destroyed and to get a sane Unix API.
> 
> To start with try applying permissions to clock sources via the POSIX
> API. That is something that will be required for some applications.
> 
> I need to be able to open sys/clock/foo/something and get a meaningful
> handle. Sure it's quite likely the operations it supports are related to
> the POSIX timer ops.

Do you mean this:

   id = read(/sys/clock/foo/lock); /* clock is busy, cannot be removed */
   ...
   clock_gettime(id, ts);
   ...
   write(/sys/clock/foo/release, id); /* all done with clock */

I am not sure what you are describing. Can you give an example, like
pseudocode or something?

Thanks,
Richard

Re: [PATCH 2/2] posix clocks: introduce a sysfs presence.

[Greg KH]

On Fri, Sep 10, 2010 at 11:31:35AM +0200, Richard Cochran wrote:
> On Fri, Sep 10, 2010 at 12:00:08AM +0100, Alan Cox wrote:
> > Also we have existing time sources that don't follow the poxix clock
> > model - I can open /dev/rtc and I can open the hpet and so on.
> > 
> > I like /sys/class/time* *but* you need to be able to open the sysfs
> > device and apply operations to it in order for it to work when your closk
> > can be dynamically created and destroyed and to get a sane Unix API.
> > 
> > To start with try applying permissions to clock sources via the POSIX
> > API. That is something that will be required for some applications.
> > 
> > I need to be able to open sys/clock/foo/something and get a meaningful
> > handle. Sure it's quite likely the operations it supports are related to
> > the POSIX timer ops.
> 
> Do you mean this:
> 
>    id = read(/sys/clock/foo/lock); /* clock is busy, cannot be removed */

That's not for sysfs, if you want to do something like this, create
clockfs please :)

thanks,

greg k-h

Re: [PATCH 0/2] [RFC] posix clock tuning

[Christoph Lameter]

On Fri, 3 Sep 2010, Richard Cochran wrote:

> Recently on the lkml, we discussed how to allow adjustment of posix
> clocks. I have tried to understand and implement the ideas expressed
> in the various threads.
>
> If there is agreement on this approach, I will resubmit these two
> patches as part of a longer series, including support for a new kind
> of hardware clock, so you can see how it all fits together. But first,
> I would like to concentrate on the new syscall itself.
>
> The patches are against a recent net-next tree. Please don't worry if
> the syscall tables are not quite up to date. I can fix that later.

Could you explain why the existing interfaces are not sufficient that do
what you describe?

man adjtime
man adjtimex
man clock_settime
?

Why would you need this to support a new hardware clock? You want the
clock to run at a different speed from system time?

Re: [PATCH 0/2] [RFC] posix clock tuning

[Christian Riesch]

Christoph Lameter wrote:
> On Fri, 3 Sep 2010, Richard Cochran wrote:
>> Recently on the lkml, we discussed how to allow adjustment of posix
>> clocks. I have tried to understand and implement the ideas expressed
>> in the various threads.
....
> Could you explain why the existing interfaces are not sufficient that do
> what you describe?

Richard's idea is to support clock hardware for IEEE 1588 (PTP, 
Precision Time Protocol). Have a look at the earlier discussions:

http://lkml.org/lkml/2010/8/16/90
and
http://lkml.org/lkml/2010/8/23/49

Christian

Re: [PATCH 0/2] [RFC] posix clock tuning

[Christoph Lameter]

On Sat, 4 Sep 2010, Christian Riesch wrote:

> Richard's idea is to support clock hardware for IEEE 1588 (PTP, Precision Time
> Protocol). Have a look at the earlier discussions:

Ok great. A summary of that belongs into the intro of the patchset.

Re: [PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

On Sat, Sep 04, 2010 at 08:37:22PM -0500, Christoph Lameter wrote:
> On Sat, 4 Sep 2010, Christian Riesch wrote:
> 
> > Richard's idea is to support clock hardware for IEEE 1588 (PTP, Precision Time
> > Protocol). Have a look at the earlier discussions:
> 
> Ok great. A summary of that belongs into the intro of the patchset.

I left off all mention of PTP, since Alan Cox said that all this talk
of PTP is not helpful. I think that the idea is introduce a method to
tune a clock referenced by its clockid_t. In other words, a patch for
tuning posix clocks, just as the subject lines indicate.

Richard

Re: [PATCH 0/2] [RFC] posix clock tuning

[Christoph Lameter]

On Sat, 4 Sep 2010, Christian Riesch wrote:

> Richard's idea is to support clock hardware for IEEE 1588 (PTP, Precision Time
> Protocol). Have a look at the earlier discussions:
>
> http://lkml.org/lkml/2010/8/16/90

A superficial scan does not give me any definite agreement on an approach.
The patchset also needs to be CCed to John Stultz. Without a summary I
still am not clear what this is supposed to address.

Tuning of the PTP clock can be done with driver specific configuration
rather than a new POSIX clock. That is at least how SGI_CYCLE is operating
which is similar.

> http://lkml.org/lkml/2010/8/23/49

Ok this goes more into something that may prove useful but it would be
very helpful still to have a clear discussion as to why this is all
needed. From what I can tell the functionality is already there and
another clock device (CLOCK_SGI_CYCLE) is already providing a model for
how a PTP clock could be controlled and used.

Re: [PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

On Sat, Sep 04, 2010 at 08:47:39PM -0500, Christoph Lameter wrote:
> On Sat, 4 Sep 2010, Christian Riesch wrote:
> 
> > Richard's idea is to support clock hardware for IEEE 1588 (PTP, Precision Time
> > Protocol). Have a look at the earlier discussions:
> >
> > http://lkml.org/lkml/2010/8/16/90
> 
> A superficial scan does not give me any definite agreement on an approach.

Welcome to the club. I am also not too sure how to bring everything
that was said together.

I have attempted to synthesize the various ideas from the previous
threads into a concrete form (a new patch set) in order to see:

1. If I understood what the commentators were asking for.
2. Whether people now agree on the approach.

> Ok this goes more into something that may prove useful but it would be
> very helpful still to have a clear discussion as to why this is all
> needed. From what I can tell the functionality is already there and
> another clock device (CLOCK_SGI_CYCLE) is already providing a model for
> how a PTP clock could be controlled and used.

John Stultz warned against that approach in his mail from August 18.

    http://lkml.org/lkml/2010/8/18/264

At this point, I think it is clear that, in order to support IEEE
1588, some kind of changes are needed. I have argued and discussed the
issues at some length. I am willing to continue the discussion, but I
would also appreciate more than a "superficial scan." PTP is quite a
large topic that probably cannot be adequately treated in a single
email.

Having said that, if you are convinced that the existing interfaces
are good enough to support PTP (and possible future methods, as
discussed in the threads), then I am all ears to find out how to do
make it work.

Thanks,
Richard

Re: [PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

On Sun, Sep 05, 2010 at 08:22:40AM +0200, Richard Cochran wrote:
> At this point, I think it is clear that, in order to support IEEE
> 1588, some kind of changes are needed. I have argued and discussed the

Just to be absolutely clear, I should have said, "hardare clocks for
IEEE 1588." The aim of the patch set is to support a new kind of
hardware.

> Having said that, if you are convinced that the existing interfaces
> are good enough to support PTP (and possible future methods, as

Again, I meant "PTP hardware clocks."

When I get some more time, I'll try to put together an executive
summary of PTP and of all the discussions that have taken place over
the last few months on linux-netdev and the lkml.

Richard

Re: [PATCH 0/2] [RFC] posix clock tuning

[Christoph Lameter]

On Sun, 5 Sep 2010, Richard Cochran wrote:

> Again, I meant "PTP hardware clocks."
>
> When I get some more time, I'll try to put together an executive
> summary of PTP and of all the discussions that have taken place over
> the last few months on linux-netdev and the lkml.

I know PTP, I just dont know why you would need to extend the existing
apis for support. Maybe you could just simply write a PTP clock driver? If
you follow the way hpet is implemented then you dont even need a clock id.
See drivers/char/hpet.c. I cannot imagine why someone would want to set
PTP to a different time and speed (vs. realtime not the hardware
configuration) than the standard clock. The API you proposed so far is
not needed as far as I can tell.

I really would like a PTP clock as soon as possible. Very very important
for time sync via the network.

Then there is CLOCK_SGI_CYCLE. Another hw clock that can be set and gotten
already with the standard posix interface. See drivers/char/mmtimer.c. The
main benefit from CLOCK_SGI_CYCLE is that you can set it manually (maybe
you want an offset to realtime? But I do not know of any usecase for
setting the clock) and that the clock can cause hardware IRQ events
to cause timer timers in user space (would be great to know if PTP needs
this?). The clock is also distributed in the local memory space of
each node. Plus the clock device can be configured in some fashion
(but then hpet also has some ioctls).

I would not think that PTP needs any of this specialized functionality.
John point about not wanting to proliferate different APIs and useless
clock ids is valid. An intial implementation using hpet style stuff would
be satisfactory and as far as I can tell would be rather trivial to do.

Standardizing the ioctls between the different clock drivers would be
useful so that they can all be controlled with some common API. But it
would be great to first have a release of a clock_ptp listing all the
needed ioctl so that we can see what the common subset is.

Re: [PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

(Taking John Stultz on CC.)

On Sun, Sep 05, 2010 at 06:13:33PM -0500, Christoph Lameter wrote:
> On Sun, 5 Sep 2010, Richard Cochran wrote:
> > When I get some more time, I'll try to put together an executive
> > summary of PTP and of all the discussions that have taken place over
> > the last few months on linux-netdev and the lkml.
> 
> I know PTP,

Ah, thats good.

> I just dont know why you would need to extend the existing
> apis for support. Maybe you could just simply write a PTP clock driver? If

That is what I already did in this patch set:

   http://lkml.org/lkml/2010/8/16/90

However, people asked, why not just use the posix clock API?  And so,
I implemented that idea, too. Now the question is, which API is the
better choice. I like the posix clock idea, but I am also fine with
the character device. That was my original approach.

> you follow the way hpet is implemented then you dont even need a clock id.
> See drivers/char/hpet.c. I cannot imagine why someone would want to set
> PTP to a different time and speed (vs. realtime not the hardware
> configuration) than the standard clock. The API you proposed so far is
> not needed as far as I can tell.

No one wants to set a different time, but the fact remains that we
have two separate clocks. The two clocks need to be synchronized, but
how do you do this?

My idea was this. Simply use the PPS subsystem to synchronize the
Linux system time to the PTP time in the PTP hardware clock. That is
just like using a GPS or radio clock. The only requirement is that the
hardware clocks provides one interrupt.

It will happen that some hardware will be build without any interrupt
between the PTP hardware clock and the CPU. Even so, such a setup can
still be useful, for example in an embedded control application. The
userland software can simply ignore the wrong system time. I think the
PTP hardware clock infrastructure should be rich enough to support
this situation.

> I really would like a PTP clock as soon as possible. Very very important
> for time sync via the network.

Good, me too!
 
> Then there is CLOCK_SGI_CYCLE. Another hw clock that can be set and gotten
> already with the standard posix interface. See drivers/char/mmtimer.c. The
> main benefit from CLOCK_SGI_CYCLE is that you can set it manually (maybe
> you want an offset to realtime? But I do not know of any usecase for
> setting the clock) and that the clock can cause hardware IRQ events
> to cause timer timers in user space (would be great to know if PTP needs
> this?).

Some PTP hardware clocks provide interrupts to the main CPU. These can
used to implement a PPS input, user alarms, and user timers.

> I would not think that PTP needs any of this specialized functionality.

Well, to make the super-synchronized time in the PTP hardware clock
really useful, there needs to be some extra functionality, like
timestamping external signals or producing periodic or one-shot output
signals.

If you don't have or need such functionality, then running PTP with
plain old software timestamping (no special PTP hardware clock) should
be good enough for most purposes.

> John point about not wanting to proliferate different APIs and useless
> clock ids is valid. An intial implementation using hpet style stuff would
> be satisfactory and as far as I can tell would be rather trivial to do.
> 
> Standardizing the ioctls between the different clock drivers would be
> useful so that they can all be controlled with some common API. But it
> would be great to first have a release of a clock_ptp listing all the
> needed ioctl so that we can see what the common subset is.

Okay, I think I have already done that in the above patch set. Would
you care to comment on it?

As I said before, I prefer the posix clock idea, but the character
device implementation is fine, too. Heck, we can offer both
avenues. My goal is to support PTP hardware clocks in the kernel.

Thanks,
Richard

Re: [PATCH 0/2] [RFC] posix clock tuning

[Thomas Gleixner]

On Fri, 3 Sep 2010, Richard Cochran wrote:

> Recently on the lkml, we discussed how to allow adjustment of posix
> clocks. I have tried to understand and implement the ideas expressed
> in the various threads.

Cc'ing the relevant maintainers to such discussions and patches might
be helpful. Seems I need to do some archive digging.

> If there is agreement on this approach, I will resubmit these two
> patches as part of a longer series, including support for a new kind
> of hardware clock, so you can see how it all fits together. But first,
> I would like to concentrate on the new syscall itself.
> 
> The patches are against a recent net-next tree. Please don't worry if
> the syscall tables are not quite up to date. I can fix that later.

How is this related to net-next ? 

Thanks,

	tglx

Re: [PATCH 0/2] [RFC] posix clock tuning

[Richard Cochran]

On Thu, Sep 09, 2010 at 11:58:09AM +0200, Thomas Gleixner wrote:
> On Fri, 3 Sep 2010, Richard Cochran wrote:
> 
> > Recently on the lkml, we discussed how to allow adjustment of posix
> > clocks. I have tried to understand and implement the ideas expressed
> > in the various threads.
> 
> Cc'ing the relevant maintainers to such discussions and patches might
> be helpful. Seems I need to do some archive digging.

Sorry about that. Still learning how to work on the list.  The story
so far is...

** PTP hardware clock as a character device (54 messages)

   http://lkml.org/lkml/2010/8/16/90

   The general mood of this was something like: "You have made a
   chardev for PTP clocks. The ioctls look similar to the posix clock
   api. Why not just use the posix api?"

** posix clock tuning syscall, static clockid (13 messages)

   http://lkml.org/lkml/2010/8/23/49

   The general mood of this one was: "I don't like a static clockid
   for PTP clocks. But a dynamic id would be just fine."

** posix clock tuning syscall, dynamic clockid (16 messages)

   http://lkml.org/lkml/2010/9/3/119 (This thread)

   The mood seems to be, "why not use a chardev for that?"

>From my point of view, the discussion is going round in circles. There
seems to be agreement that supporting PTP hardware clocks is a good
idea. The objections seem to be about the form of the API. In general,
there are three avenues.

1. character device (like hpet)
2. posix clock api
3. sysfs

Or possibly some combination of the three.

I have my preference, but I am willing to work it according to
taste. Would you have some preference?

> > If there is agreement on this approach, I will resubmit these two
> > patches as part of a longer series, including support for a new kind
> > of hardware clock, so you can see how it all fits together. But first,
> > I would like to concentrate on the new syscall itself.
> > 
> > The patches are against a recent net-next tree. Please don't worry if
> > the syscall tables are not quite up to date. I can fix that later.
> 
> How is this related to net-next ? 

The whole hardware timestamping and PTP clock issue is somewhat
network related. In particular, needed to start there to get some
background support for timestamping within PHY devices.

Thanks,

Richard

Re: [PATCH 0/2] [RFC] posix clock tuning

[Thomas Gleixner]

On Thu, 9 Sep 2010, Richard Cochran wrote:
> From my point of view, the discussion is going round in circles. There

Sorry for coming late and adding more to the general confusion. :)

> seems to be agreement that supporting PTP hardware clocks is a good
> idea. The objections seem to be about the form of the API. In general,
> there are three avenues.
> 
> 1. character device (like hpet)
> 2. posix clock api
> 3. sysfs
> 
> Or possibly some combination of the three.

I fail to see the requirement for any of those. Either the hardware
clock is suitable as a clocksource for general consumption, then it
should just be used as a clock source. If it's not - e.g. because it's
too slow to access - then it just should serve as a reference in the
NTP style and steer the timekeeping into sync.

As I said in the other reply, I know that you want a global
synchronized clock aside of CLOCK_REALTIME, but that's an easy to
solve problem.

See my other reply for details.

Thanks,

	tglx

Re: [PATCH 0/2] [RFC] posix clock tuning

[Alan Cox]

> > 1. character device (like hpet)
> > 2. posix clock api
> > 3. sysfs
> > 
> > Or possibly some combination of the three.
> 
> I fail to see the requirement for any of those. Either the hardware
> clock is suitable as a clocksource for general consumption, then it
> should just be used as a clock source. If it's not - e.g. because it's
> too slow to access - then it just should serve as a reference in the
> NTP style and steer the timekeeping into sync.

I think you are confusing clocks, time stamps and the system time.

System time is a single default reference source defined by POSIX in
terms of sort of UTC.

There are lots of other time sources which don't necessarily tally with
oen another and there are times you have to respect more than one of them
because you are not the master clock nor can you dictate synchronization
between the two pieces of infrastructure you span

Consider a large rock gig - your control systems for the speakers and PA
are tightly synchronized and delivering audio with very precise delays to
different amp/speaker combinations. At the same time if you are managing
effects and the like then the instrument timings for those will be coming
off another clock, which is also very precise and differently sourced.

So what we actually have is

- multiple input devices that report timestamps (the PC clock, GPS, PTP,
  time synchronized busses, synchronous ethernet, network provided reports
  etc)

- some of those inputs get turned into clocks with varying degrees of
  hardware and software processing which are consumed by various bits of
  software and drivers

- a subset of those clocks in some form are algmated into a generic
  system time. which provides a meaningful representation of general time
  to the majority of apps that simple can't care less

Apps and drivers need access to all three layers.