[PATCHv5 0/7] system time changes notification

[PATCHv5 0/7] system time changes notification

[Alexander Shishkin]

Hi,

This is the fifth version of system time change notification mechanism
for linux kernel. The need for it comes from applications which would
like to keep track of time changes without having to wake up every
$TIMEOUT and calling gettimeofday().

An excellent description for one of the usecases, written by Kay Sievers
(http://kerneltrap.org/mailarchive/linux-kernel/2010/8/5/4603531):
"""
This is the example Lennart and I thought about when we considered
adding cron-like stuff to systemd's timer configs, but didn't want to
do silly things like scheduled checks for the actual time, so we
delayed this feature until such a notification becomes available.

Consider we want stuff like "wakeup every day at 3pm", the next wakeup
might be earlier than the timer we calculated last time, on system
time changes. We need to re-calculate it. This is necessary for all
repeating events.

Say we want to wakeup at 3pm, now it's 4pm, so we schedule it in 23
hours. Now the system time changes to 2pm, and we would expect to
wakeup in one hour, but we take 25.
"""

Changes since v4:
 - updated arm and s390 syscall wiring
 - removed RFC
Changes since v3:
 - broken out separate patches adding time_change_notify syscall to
   arm, powerpc, x86, ia64, s390 and blackfin
Changes since v2:
 - replaced sysfs interface with a syscall
 - added sysctl/procfs handle to set a limit to the number of users
 - fixed issues pointed out by Greg.
Changes since v1:
 - updated against 2.6.36-rc1,
 - added notification/filtering options,
 - added Documentation/ABI/sysfs-kernel-time-notify interface description.

Alexander Shishkin (7):
      notify userspace about time changes
      wire up sys_time_change_notify() on ARM
      wire up sys_time_change_notify() on x86
      wire up sys_time_change_notify() on powerpc
      wire up sys_time_change_notify() on blackfin
      wire up sys_time_change_notify() on ia64
      wire up sys_time_change_notify() on s390

 Documentation/time-change-notify-example.c |   64 ++++++++++
 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/ia64/include/asm/unistd.h             |    3 +-
 arch/ia64/kernel/entry.S                   |    1 +
 arch/powerpc/include/asm/systbl.h          |    1 +
 arch/powerpc/include/asm/unistd.h          |    3 +-
 arch/s390/include/asm/unistd.h             |    3 +-
 arch/s390/kernel/compat_wrapper.S          |    6 +
 arch/s390/kernel/syscalls.S                |    1 +
 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/asm-generic/unistd.h               |    4 +-
 include/linux/syscalls.h                   |    1 +
 include/linux/time.h                       |   20 +++
 init/Kconfig                               |    7 +
 kernel/Makefile                            |    1 +
 kernel/sys_ni.c                            |    3 +
 kernel/sysctl.c                            |   11 ++
 kernel/time.c                              |   11 ++-
 kernel/time_notify.c                       |  183 ++++++++++++++++++++++++++++
 25 files changed, 328 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/time-change-notify-example.c
 create mode 100644 kernel/time_notify.c

Regards,
--
Alex

[PATCH 1/7] notify userspace about time changes

[Alexander Shishkin]

Certain userspace applications (like "clock" desktop applets or cron) might
want to be notified when some other application changes the system time. It
might also be important for an application to be able to distinguish between
its own and somebody else's time changes.

This patch implements a notification interface via eventfd mechanism. Proccess
wishing to be notified about time changes should create an eventfd and pass it
to time_change_notify() syscall along with notification options.
After that, any calls to settimeofday()/stime()/adjtimex() made by other
processes will be signalled to this eventfd. Credits for suggesting the eventfd
mechanism for this purpose go to Kirill Shutemov.

This patch adds the syscall to asm-generic/unistd.h.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: John Stultz <johnstul@us.ibm.com>
CC: Martin Schwidefsky <schwidefsky@de.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Jon Hunter <jon-hunter@ti.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: David Howells <dhowells@redhat.com>
CC: Avi Kivity <avi@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: John Kacur <jkacur@redhat.com>
CC: Alexander Shishkin <virtuoso@slind.org>
CC: Chris Friesen <chris.friesen@genband.com>
CC: Kay Sievers <kay.sievers@vrfy.org>
CC: Greg KH <gregkh@suse.de>
CC: linux-kernel@vger.kernel.org
---
 Documentation/time-change-notify-example.c |   64 ++++++++++
 include/asm-generic/unistd.h               |    4 +-
 include/linux/syscalls.h                   |    1 +
 include/linux/time.h                       |   20 +++
 init/Kconfig                               |    7 +
 kernel/Makefile                            |    1 +
 kernel/sys_ni.c                            |    3 +
 kernel/sysctl.c                            |   11 ++
 kernel/time.c                              |   11 ++-
 kernel/time_notify.c                       |  183 ++++++++++++++++++++++++++++
 10 files changed, 302 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/time-change-notify-example.c
 create mode 100644 kernel/time_notify.c

diff --git a/Documentation/time-change-notify-example.c b/Documentation/time-change-notify-example.c
new file mode 100644
index 0000000..e8e4f4d
--- /dev/null
+++ b/Documentation/time-change-notify-example.c
@@ -0,0 +1,64 @@
+/*
+ * Simple program to catch system time changes
+ *
+ * written by Alexander Shishkin <virtuoso@slind.org>
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/eventfd.h>
+#include <sys/syscall.h>
+#include <errno.h>
+#include <unistd.h>
+#include <poll.h>
+
+#ifndef SYS_time_change_notify
+# include "asm/unistd.h"
+# ifdef __NR_time_change_notify
+#  define SYS_time_change_notify __NR_time_change_notify
+# else
+#  error Cannot figure out time_change_notify syscall number.
+# endif
+#endif
+
+static int time_change_notify(int fd, unsigned int flags)
+{
+	return syscall(SYS_time_change_notify, fd, flags);
+}
+
+int main(int argc, char **argv)
+{
+	struct pollfd fds = { .events = POLLIN };
+
+	fds.fd = eventfd(0, 0);
+	if (fds.fd < 0) {
+		perror("eventfd");
+		return EXIT_FAILURE;
+	}
+
+	/* subscribe to all events from all sources */
+	if (time_change_notify(fds.fd, 0xf)) {
+		perror("time_change_notify");
+		return EXIT_FAILURE;
+	}
+
+	while (poll(&fds, 1, -1) > 0) {
+		eventfd_t data;
+		ssize_t r;
+
+		r = read(fds.fd, &data, sizeof data);
+		if (r == -1) {
+			if (errno == EINTR)
+				continue;
+
+			break;
+		}
+
+		printf("system time has changed %llu times\n", data);
+	}
+
+	puts("Done polling system time changes.\n");
+
+	return EXIT_SUCCESS;
+}
+
diff --git a/include/asm-generic/unistd.h b/include/asm-generic/unistd.h
index b969770..c8372db 100644
--- a/include/asm-generic/unistd.h
+++ b/include/asm-generic/unistd.h
@@ -646,9 +646,11 @@ __SYSCALL(__NR_prlimit64, sys_prlimit64)
 __SYSCALL(__NR_fanotify_init, sys_fanotify_init)
 #define __NR_fanotify_mark 263
 __SYSCALL(__NR_fanotify_mark, sys_fanotify_mark)
+#define __NR_time_change_notify 264
+__SYSCALL(__NR_time_change_notify, sys_time_change_notify)
 
 #undef __NR_syscalls
-#define __NR_syscalls 264
+#define __NR_syscalls 265
 
 /*
  * All syscalls below here should go away really,
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e6319d1..789f92e 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -819,6 +819,7 @@ asmlinkage long sys_fanotify_init(unsigned int flags, unsigned int event_f_flags
 asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
 				  u64 mask, int fd,
 				  const char  __user *pathname);
+asmlinkage long sys_time_change_notify(int fd, unsigned int flags);
 
 int kernel_execve(const char *filename, const char *const argv[], const char *const envp[]);
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 9f15ac7..d66045e 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -252,6 +252,26 @@ static __always_inline void timespec_add_ns(struct timespec *a, u64 ns)
 	a->tv_sec += __iter_div_u64_rem(a->tv_nsec + ns, NSEC_PER_SEC, &ns);
 	a->tv_nsec = ns;
 }
+
+/* time change events */
+#define TIME_EVENT_SET 0
+#define TIME_EVENT_ADJ 1
+
+#define TIME_CHANGE_NOTIFY_OTHERS	BIT(0)
+#define TIME_CHANGE_NOTIFY_OWN		BIT(1)
+#define TIME_CHANGE_NOTIFY_SET		BIT(2)
+#define TIME_CHANGE_NOTIFY_ADJUST	BIT(3)
+
+#define TIME_CHANGE_NOTIFY_MAX_USERS	1024
+
+#ifdef CONFIG_TIME_NOTIFY
+extern unsigned int time_change_notify_max_users;
+
+void time_notify_all(int type);
+#else
+static inline void time_notify_all(int type) {}
+#endif
+
 #endif /* __KERNEL__ */
 
 #define NFDBITS			__NFDBITS
diff --git a/init/Kconfig b/init/Kconfig
index 2de5b1c..504a51a 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -980,6 +980,13 @@ config PERF_USE_VMALLOC
 	help
 	  See tools/perf/design.txt for details
 
+config TIME_NOTIFY
+	bool "System time changes notification for userspace"
+	depends on EVENTFD
+	help
+	  Enable time change notification events to userspace via
+	  eventfd.
+
 menu "Kernel Performance Events And Counters"
 
 config PERF_EVENTS
diff --git a/kernel/Makefile b/kernel/Makefile
index 0b72d1a..ac53c67 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
 obj-$(CONFIG_PADATA) += padata.o
+obj-$(CONFIG_TIME_NOTIFY) += time_notify.o
 
 ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index bad369e..bb27e93 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -185,3 +185,6 @@ cond_syscall(sys_perf_event_open);
 /* fanotify! */
 cond_syscall(sys_fanotify_init);
 cond_syscall(sys_fanotify_mark);
+
+/* time change notification */
+cond_syscall(sys_time_change_notify);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f88552c..9e7d7a9 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1441,6 +1441,17 @@ static struct ctl_table fs_table[] = {
 		.proc_handler	= proc_doulongvec_minmax,
 	},
 #endif /* CONFIG_AIO */
+#ifdef CONFIG_TIME_NOTIFY
+	{
+		.procname	= "time-change-notify-max-users",
+		.data		= &time_change_notify_max_users,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &ten_thousand,
+	},
+#endif
 #ifdef CONFIG_INOTIFY_USER
 	{
 		.procname	= "inotify",
diff --git a/kernel/time.c b/kernel/time.c
index ba9b338..b4155b8 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -92,7 +92,9 @@ SYSCALL_DEFINE1(stime, time_t __user *, tptr)
 	if (err)
 		return err;
 
-	do_settimeofday(&tv);
+	err = do_settimeofday(&tv);
+	if (!err)
+		time_notify_all(TIME_EVENT_SET);
 	return 0;
 }
 
@@ -177,7 +179,10 @@ int do_sys_settimeofday(struct timespec *tv, struct timezone *tz)
 		/* SMP safe, again the code in arch/foo/time.c should
 		 * globally block out interrupts when it runs.
 		 */
-		return do_settimeofday(tv);
+		error = do_settimeofday(tv);
+		if (!error)
+			time_notify_all(TIME_EVENT_SET);
+		return error;
 	}
 	return 0;
 }
@@ -215,6 +220,8 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
 	if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
 		return -EFAULT;
 	ret = do_adjtimex(&txc);
+	if (!ret)
+		time_notify_all(TIME_EVENT_ADJ);
 	return copy_to_user(txc_p, &txc, sizeof(struct timex)) ? -EFAULT : ret;
 }
 
diff --git a/kernel/time_notify.c b/kernel/time_notify.c
new file mode 100644
index 0000000..1e57eb4
--- /dev/null
+++ b/kernel/time_notify.c
@@ -0,0 +1,183 @@
+/*
+ * linux/kernel/time_notify.c
+ *
+ * Copyright (C) 2010 Nokia Corporation
+ * Alexander Shishkin
+ *
+ * This file implements an interface to communicate time changes to userspace.
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/syscalls.h>
+#include <linux/slab.h>
+#include <linux/eventfd.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+#include <linux/sched.h>
+#include <linux/poll.h>
+#include <linux/err.h>
+
+/* sysctl tunable to limit the number of users */
+unsigned int time_change_notify_max_users = TIME_CHANGE_NOTIFY_MAX_USERS;
+
+/*
+ * A process can "subscribe" to receive a notification via eventfd that
+ * some other process has called stime/settimeofday/adjtimex.
+ */
+struct time_event {
+	struct eventfd_ctx	*eventfd;
+	struct task_struct	*watcher;
+	unsigned int		want_others:1;
+	unsigned int		want_own:1;
+	unsigned int		want_set:1;
+	unsigned int		want_adj:1;
+	struct work_struct	remove;
+	wait_queue_t		wq;
+	wait_queue_head_t	*wqh;
+	poll_table		pt;
+	struct list_head	list;
+};
+
+static LIST_HEAD(event_list);
+static int nevents;
+static DEFINE_SPINLOCK(event_lock);
+
+/*
+ * Do the necessary cleanup when the eventfd is being closed
+ */
+static void time_event_remove(struct work_struct *work)
+{
+	struct time_event *evt = container_of(work, struct time_event, remove);
+
+	BUG_ON(nevents <= 0);
+
+	kfree(evt);
+	nevents--;
+}
+
+static int time_event_wakeup(wait_queue_t *wq, unsigned int mode, int sync,
+			     void *key)
+{
+	struct time_event *evt = container_of(wq, struct time_event, wq);
+	unsigned long flags = (unsigned long)key;
+
+	if (flags & POLLHUP) {
+		__remove_wait_queue(evt->wqh, &evt->wq);
+		spin_lock(&event_lock);
+		list_del(&evt->list);
+		spin_unlock(&event_lock);
+
+		schedule_work(&evt->remove);
+	}
+
+	return 0;
+}
+
+static void time_event_ptable_queue_proc(struct file *file,
+					 wait_queue_head_t *wqh, poll_table *pt)
+{
+	struct time_event *evt = container_of(pt, struct time_event, pt);
+
+	evt->wqh = wqh;
+	add_wait_queue(wqh, &evt->wq);
+}
+
+/*
+ * time_change_notify() registers a given eventfd to receive time change
+ * notifications
+ */
+SYSCALL_DEFINE2(time_change_notify, int, fd, unsigned int, flags)
+{
+	int ret;
+	struct file *file;
+	struct time_event *evt;
+
+	evt = kmalloc(sizeof(*evt), GFP_KERNEL);
+	if (!evt)
+		return -ENOMEM;
+
+	evt->want_others = !!(flags & TIME_CHANGE_NOTIFY_OTHERS);
+	evt->want_own = !!(flags & TIME_CHANGE_NOTIFY_OWN);
+	evt->want_set = !!(flags & TIME_CHANGE_NOTIFY_SET);
+	evt->want_adj = !!(flags & TIME_CHANGE_NOTIFY_ADJUST);
+
+	file = eventfd_fget(fd);
+	if (IS_ERR(file)) {
+		ret = -EINVAL;
+		goto out_free;
+	}
+
+	evt->eventfd = eventfd_ctx_fileget(file);
+	if (IS_ERR(evt->eventfd)) {
+		ret = PTR_ERR(evt->eventfd);
+		goto out_fput;
+	}
+
+	INIT_LIST_HEAD(&evt->list);
+	INIT_WORK(&evt->remove, time_event_remove);
+
+	init_waitqueue_func_entry(&evt->wq, time_event_wakeup);
+	init_poll_funcptr(&evt->pt, time_event_ptable_queue_proc);
+
+	evt->watcher = current;
+
+	spin_lock(&event_lock);
+	if (nevents == time_change_notify_max_users) {
+		spin_unlock(&event_lock);
+		ret = -EBUSY;
+		goto out_fput;
+	}
+
+	nevents++;
+	list_add(&evt->list, &event_list);
+	spin_unlock(&event_lock);
+
+	if (file->f_op->poll(file, &evt->pt) & POLLHUP) {
+		ret = 0;
+		goto out_fput;
+	}
+
+	fput(file);
+
+	return 0;
+
+out_fput:
+	fput(file);
+
+out_free:
+	kfree(evt);
+
+	return ret;
+}
+
+void time_notify_all(int type)
+{
+	struct list_head *tmp;
+
+	spin_lock(&event_lock);
+	list_for_each(tmp, &event_list) {
+		struct time_event *e = container_of(tmp, struct time_event,
+						    list);
+
+		if (type == TIME_EVENT_SET && !e->want_set)
+			continue;
+		else if (type == TIME_EVENT_ADJ && !e->want_adj)
+			continue;
+
+		if (e->watcher == current && !e->want_own)
+			continue;
+		else if (e->watcher != current && !e->want_others)
+			continue;
+
+		eventfd_signal(e->eventfd, 1);
+	}
+	spin_unlock(&event_lock);
+}
+
+static int time_notify_init(void)
+{
+	return 0;
+}
+
+core_initcall(time_notify_init);
-- 
1.7.2.1.45.gb66c2

[PATCH 2/7] wire up sys_time_change_notify() on ARM

[Alexander Shishkin]

sys_time_change_notify() is a new syscall with number and types of
parameters such that no ARM-specific processing is needed.

Tested with 2.6.36-rc3 using Documentation/time-change-notify-example.c.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Howells <dhowells@redhat.com>
CC: Christoph Hellwig <hch@lst.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
 arch/arm/include/asm/unistd.h |    1 +
 arch/arm/kernel/calls.S       |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index c891eb7..45d5dc9 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -396,6 +396,7 @@
 #define __NR_fanotify_init		(__NR_SYSCALL_BASE+367)
 #define __NR_fanotify_mark		(__NR_SYSCALL_BASE+368)
 #define __NR_prlimit64			(__NR_SYSCALL_BASE+369)
+#define __NR_time_change_notify		(__NR_SYSCALL_BASE+370)
 
 /*
  * The following SWIs are ARM private.
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 5c26ecc..633e71a 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -379,6 +379,7 @@
 		CALL(sys_fanotify_init)
 		CALL(sys_fanotify_mark)
 		CALL(sys_prlimit64)
+		CALL(sys_time_change_notify)
 #ifndef syscalls_counted
 .equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
 #define syscalls_counted
-- 
1.7.2.1.45.gb66c2

[PATCH 3/7] wire up sys_time_change_notify() on x86

[Alexander Shishkin]

This patch adds a new syscall of the following form:

int sys_time_change_notify(int fd, unsigned int flags)

This syscall is used for registering an eventfd for system time
change notification.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Christoph Hellwig <hch@lst.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Eric Paris <eparis@redhat.com>
CC: Russell King <rmk+kernel@arm.linux.org.uk>
CC: David Howells <dhowells@redhat.com>
CC: Jiri Slaby <jslaby@suse.cz>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-kernel@vger.kernel.org
---
 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 +
 4 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index 518bb99..6981bdc 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -851,4 +851,5 @@ ia32_sys_call_table:
 	.quad sys_fanotify_init
 	.quad sys32_fanotify_mark
 	.quad sys_prlimit64		/* 340 */
+	.quad sys_time_change_notify
 ia32_syscall_end:
diff --git a/arch/x86/include/asm/unistd_32.h b/arch/x86/include/asm/unistd_32.h
index b766a5e..17ac4f5 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_time_change_notify	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..3d745f9 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_time_change_notify			303
+__SYSCALL(__NR_time_change_notify, sys_time_change_notify)
 
 #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..b1ae9d4 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_time_change_notify
-- 
1.7.2.1.45.gb66c2

[PATCH 4/7] wire up sys_time_change_notify() on powerpc

[Alexander Shishkin]

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andreas Schwab <schwab@linux-m68k.org>
CC: Alexander Shishkin <virtuoso@slind.org>
CC: Christoph Hellwig <hch@lst.de>
CC: Jesper Nilsson <jesper.nilsson@axis.com>
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-kernel@vger.kernel.org
---
 arch/powerpc/include/asm/systbl.h |    1 +
 arch/powerpc/include/asm/unistd.h |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index 3d21266..124b610 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -329,3 +329,4 @@ COMPAT_SYS(rt_tgsigqueueinfo)
 SYSCALL(fanotify_init)
 COMPAT_SYS(fanotify_mark)
 SYSCALL_SPU(prlimit64)
+SYSCALL(time_change_notify)
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index 597e6f9..6ab64da 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -348,10 +348,11 @@
 #define __NR_fanotify_init	323
 #define __NR_fanotify_mark	324
 #define __NR_prlimit64		325
+#define __NR_time_change_notify	326
 
 #ifdef __KERNEL__
 
-#define __NR_syscalls		326
+#define __NR_syscalls		327
 
 #define __NR__exit __NR_exit
 #define NR_syscalls	__NR_syscalls
-- 
1.7.2.1.45.gb66c2

[PATCH 5/7] wire up sys_time_change_notify() on blackfin

[Alexander Shishkin]

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: Mike Frysinger <vapier@gentoo.org>
CC: Alexander Shishkin <virtuoso@slind.org>
CC: Arjan van de Ven <arjan@linux.intel.com>
CC: Paul Mackerras <paulus@samba.org>
CC: Ingo Molnar <mingo@elte.hu>
CC: Robin Getz <robin.getz@analog.com>
CC: Philippe Gerum <rpm@xenomai.org>
CC: "David S. Miller" <davem@davemloft.net>
CC: Yi Li <yi.li@analog.com>
CC: uclinux-dist-devel@blackfin.uclinux.org
CC: linux-kernel@vger.kernel.org
---
 arch/blackfin/include/asm/unistd.h |    3 ++-
 arch/blackfin/mach-common/entry.S  |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/blackfin/include/asm/unistd.h b/arch/blackfin/include/asm/unistd.h
index 14fcd25..e58cfaf 100644
--- a/arch/blackfin/include/asm/unistd.h
+++ b/arch/blackfin/include/asm/unistd.h
@@ -392,8 +392,9 @@
 #define __NR_fanotify_init	371
 #define __NR_fanotify_mark	372
 #define __NR_prlimit64		373
+#define __NR_time_change_notify	374
 
-#define __NR_syscall		374
+#define __NR_syscall		375
 #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 af1bffa..2771e3b 100644
--- a/arch/blackfin/mach-common/entry.S
+++ b/arch/blackfin/mach-common/entry.S
@@ -1631,6 +1631,7 @@ ENTRY(_sys_call_table)
 	.long _sys_fanotify_init
 	.long _sys_fanotify_mark
 	.long _sys_prlimit64
+	.long _sys_time_change_notify
 
 	.rept NR_syscalls-(.-_sys_call_table)/4
 	.long _sys_ni_syscall
-- 
1.7.2.1.45.gb66c2

[PATCH 6/7] wire up sys_time_change_notify() on ia64

[Alexander Shishkin]

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: Tony Luck <tony.luck@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
CC: David Howells <dhowells@redhat.com>
CC: Alexander Shishkin <virtuoso@slind.org>
CC: linux-ia64@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/ia64/include/asm/unistd.h |    3 ++-
 arch/ia64/kernel/entry.S       |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/ia64/include/asm/unistd.h b/arch/ia64/include/asm/unistd.h
index 954d398..7445473 100644
--- a/arch/ia64/include/asm/unistd.h
+++ b/arch/ia64/include/asm/unistd.h
@@ -315,11 +315,12 @@
 #define __NR_fanotify_init		1323
 #define __NR_fanotify_mark		1324
 #define __NR_prlimit64			1325
+#define __NR_time_change_notify		1326
 
 #ifdef __KERNEL__
 
 
-#define NR_syscalls			302 /* length of syscall table */
+#define NR_syscalls			303 /* length of syscall table */
 
 /*
  * The following defines stop scripts/checksyscalls.sh from complaining about
diff --git a/arch/ia64/kernel/entry.S b/arch/ia64/kernel/entry.S
index 244704a..48a5682 100644
--- a/arch/ia64/kernel/entry.S
+++ b/arch/ia64/kernel/entry.S
@@ -1771,6 +1771,7 @@ sys_call_table:
 	data8 sys_fanotify_init
 	data8 sys_fanotify_mark
 	data8 sys_prlimit64			// 1325
+	data8 sys_time_change_notify
 
 	.org sys_call_table + 8*NR_syscalls	// guard against failures to increase NR_syscalls
 #endif /* __IA64_ASM_PARAVIRTUALIZED_NATIVE */
-- 
1.7.2.1.45.gb66c2

[PATCH 7/7] wire up sys_time_change_notify() on s390

[Alexander Shishkin]

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
CC: Heiko Carstens <heiko.carstens@de.ibm.com>
CC: linux390@de.ibm.com
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Jesper Nilsson <jesper.nilsson@axis.com>
CC: David Howells <dhowells@redhat.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Christoph Hellwig <hch@lst.de>
CC: linux-s390@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 arch/s390/include/asm/unistd.h    |    3 ++-
 arch/s390/kernel/compat_wrapper.S |    6 ++++++
 arch/s390/kernel/syscalls.S       |    1 +
 3 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
index 1049ef2..f815a2d 100644
--- a/arch/s390/include/asm/unistd.h
+++ b/arch/s390/include/asm/unistd.h
@@ -272,7 +272,8 @@
 #define __NR_fanotify_init	332
 #define __NR_fanotify_mark	333
 #define __NR_prlimit64		334
-#define NR_syscalls 335
+#define __NR_time_change_notify	335
+#define NR_syscalls 336
 
 /* 
  * There are some system calls that are not present on 64 bit, some
diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
index 8e60fb2..4570418 100644
--- a/arch/s390/kernel/compat_wrapper.S
+++ b/arch/s390/kernel/compat_wrapper.S
@@ -1877,3 +1877,9 @@ sys_prlimit64_wrapper:
 	llgtr	%r4,%r4			# const struct rlimit64 __user *
 	llgtr	%r5,%r5			# struct rlimit64 __user *
 	jg	sys_prlimit64		# branch to system call
+
+	.globl sys_time_change_notify_wrapper
+sys_time_change_notify_wrapper:
+	lgfr	%r2,%r2			# int
+	llgfr	%r3,%r3			# unsigned int
+	jg	sys_time_change_notify	# branch to system call
diff --git a/arch/s390/kernel/syscalls.S b/arch/s390/kernel/syscalls.S
index a8fee1b..50ca1a7 100644
--- a/arch/s390/kernel/syscalls.S
+++ b/arch/s390/kernel/syscalls.S
@@ -343,3 +343,4 @@ SYSCALL(sys_perf_event_open,sys_perf_event_open,sys_perf_event_open_wrapper)
 SYSCALL(sys_fanotify_init,sys_fanotify_init,sys_fanotify_init_wrapper)
 SYSCALL(sys_fanotify_mark,sys_fanotify_mark,sys_fanotify_mark_wrapper)
 SYSCALL(sys_prlimit64,sys_prlimit64,sys_prlimit64_wrapper)
+SYSCALL(sys_time_change_notify,sys_time_change_notify,sys_time_change_notify_wrapper) /* 335 */
-- 
1.7.2.1.45.gb66c2

Re: [PATCH 1/7] notify userspace about time changes

[john stultz]

On Fri, 2010-09-17 at 01:10 +0300, Alexander Shishkin wrote:
> Certain userspace applications (like "clock" desktop applets or cron) might
> want to be notified when some other application changes the system time. It
> might also be important for an application to be able to distinguish between
> its own and somebody else's time changes.
> 
> This patch implements a notification interface via eventfd mechanism. Proccess
> wishing to be notified about time changes should create an eventfd and pass it
> to time_change_notify() syscall along with notification options.
> After that, any calls to settimeofday()/stime()/adjtimex() made by other
> processes will be signalled to this eventfd. Credits for suggesting the eventfd
> mechanism for this purpose go to Kirill Shutemov.

One thing I have thought of since the last time you posted this, maybe
it would be worth adding a clockid field to the syscall?

Basically, we're looking at extending the posix clocks interfaces to
allow for additional clock hardware to be exposed (See the discussion on
PTP and my CLOCK_RTC patch today for examples and details).

So it seems possible that folks would want a similar interface to catch
updates to non CLOCK_REALTIME clocks.

Does this seem reasonable?

thanks
-john

Re: [PATCH 1/7] notify userspace about time changes

[Alexander Shishkin]

On Thu, Sep 16, 2010 at 03:30:40 -0700, john stultz wrote:
> On Fri, 2010-09-17 at 01:10 +0300, Alexander Shishkin wrote:
> > Certain userspace applications (like "clock" desktop applets or cron) might
> > want to be notified when some other application changes the system time. It
> > might also be important for an application to be able to distinguish between
> > its own and somebody else's time changes.
> > 
> > This patch implements a notification interface via eventfd mechanism. Proccess
> > wishing to be notified about time changes should create an eventfd and pass it
> > to time_change_notify() syscall along with notification options.
> > After that, any calls to settimeofday()/stime()/adjtimex() made by other
> > processes will be signalled to this eventfd. Credits for suggesting the eventfd
> > mechanism for this purpose go to Kirill Shutemov.
> 
> One thing I have thought of since the last time you posted this, maybe
> it would be worth adding a clockid field to the syscall?
> 
> Basically, we're looking at extending the posix clocks interfaces to
> allow for additional clock hardware to be exposed (See the discussion on
> PTP and my CLOCK_RTC patch today for examples and details).
> 
> So it seems possible that folks would want a similar interface to catch
> updates to non CLOCK_REALTIME clocks.
> 
> Does this seem reasonable?

The whole idea seems interesting, I'll try to add this in the next iteration
of this patchset.
The first thing that comes it mind is -- are there going to be additional
events that the user might like to be notified of, like rtc_set_alarm()?
Doesn't seem too likely, though.

Regards,
--
Alex

Re: [PATCH 1/7] notify userspace about time changes

[john stultz]

On Fri, 2010-09-17 at 01:58 +0300, Alexander Shishkin wrote:
> On Thu, Sep 16, 2010 at 03:30:40 -0700, john stultz wrote:
> > On Fri, 2010-09-17 at 01:10 +0300, Alexander Shishkin wrote:
> > > Certain userspace applications (like "clock" desktop applets or cron) might
> > > want to be notified when some other application changes the system time. It
> > > might also be important for an application to be able to distinguish between
> > > its own and somebody else's time changes.
> > > 
> > > This patch implements a notification interface via eventfd mechanism. Proccess
> > > wishing to be notified about time changes should create an eventfd and pass it
> > > to time_change_notify() syscall along with notification options.
> > > After that, any calls to settimeofday()/stime()/adjtimex() made by other
> > > processes will be signalled to this eventfd. Credits for suggesting the eventfd
> > > mechanism for this purpose go to Kirill Shutemov.
> > 
> > One thing I have thought of since the last time you posted this, maybe
> > it would be worth adding a clockid field to the syscall?
> > 
> > Basically, we're looking at extending the posix clocks interfaces to
> > allow for additional clock hardware to be exposed (See the discussion on
> > PTP and my CLOCK_RTC patch today for examples and details).
> > 
> > So it seems possible that folks would want a similar interface to catch
> > updates to non CLOCK_REALTIME clocks.
> > 
> > Does this seem reasonable?
> 
> The whole idea seems interesting, I'll try to add this in the next iteration
> of this patchset.

Very cool!

> The first thing that comes it mind is -- are there going to be additional
> events that the user might like to be notified of, like rtc_set_alarm()?
> Doesn't seem too likely, though.

Well, the posix interface patch is trying to abstract over the hardware
details like rtc_set_alarm.

So instead it would be timer_create/timer_settime/etc. These interfaces
deal with timer structures that are per-process, so I don't think we
need a notification hook for them.

thanks
-john

Re: [PATCH 1/7] notify userspace about time changes

[Thomas Gleixner]

On Fri, 17 Sep 2010, Alexander Shishkin wrote:

> Certain userspace applications (like "clock" desktop applets or cron) might
> want to be notified when some other application changes the system time. It
> might also be important for an application to be able to distinguish between
> its own and somebody else's time changes.

That's overengineered. An application which fiddles with the time
should be able to deal with that inside of the application.

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index e6319d1..789f92e 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -819,6 +819,7 @@ asmlinkage long sys_fanotify_init(unsigned int flags, unsigned int event_f_flags
>  asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>  				  u64 mask, int fd,
>  				  const char  __user *pathname);
> +asmlinkage long sys_time_change_notify(int fd, unsigned int flags);

Please do not create a new syscall with a weird flag field. As I said
above the selection of who changed the time is not necessary at all,
so what you want is the selection of which change event and that
should be an enum.
 
>  int kernel_execve(const char *filename, const char *const argv[], const char *const envp[]);
>  
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 9f15ac7..d66045e 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -252,6 +252,26 @@ static __always_inline void timespec_add_ns(struct timespec *a, u64 ns)
>  	a->tv_sec += __iter_div_u64_rem(a->tv_nsec + ns, NSEC_PER_SEC, &ns);
>  	a->tv_nsec = ns;
>  }
> +
> +/* time change events */
> +#define TIME_EVENT_SET 0
> +#define TIME_EVENT_ADJ 1

Enum please

> +#define TIME_CHANGE_NOTIFY_OTHERS	BIT(0)
> +#define TIME_CHANGE_NOTIFY_OWN		BIT(1)
> +#define TIME_CHANGE_NOTIFY_SET		BIT(2)
> +#define TIME_CHANGE_NOTIFY_ADJUST	BIT(3)
> +
> +#define TIME_CHANGE_NOTIFY_MAX_USERS	1024

What's this random limit for ?

> diff --git a/init/Kconfig b/init/Kconfig
> index 2de5b1c..504a51a 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -980,6 +980,13 @@ config PERF_USE_VMALLOC
>  	help
>  	  See tools/perf/design.txt for details
>  
> +config TIME_NOTIFY
> +	bool "System time changes notification for userspace"
> +	depends on EVENTFD
> +	help
> +	  Enable time change notification events to userspace via
> +	  eventfd.
> +

How is that config switch related to PERF ?   

>  menu "Kernel Performance Events And Counters"
>  
>  config PERF_EVENTS
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 0b72d1a..ac53c67 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -104,6 +104,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
>  obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
>  obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
>  obj-$(CONFIG_PADATA) += padata.o
> +obj-$(CONFIG_TIME_NOTIFY) += time_notify.o

Please move this to kernel/time/

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f88552c..9e7d7a9 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1441,6 +1441,17 @@ static struct ctl_table fs_table[] = {
>  		.proc_handler	= proc_doulongvec_minmax,
>  	},
>  #endif /* CONFIG_AIO */
> +#ifdef CONFIG_TIME_NOTIFY
> +	{
> +		.procname	= "time-change-notify-max-users",
> +		.data		= &time_change_notify_max_users,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &ten_thousand,
> +	},

Do we really need another sysctl which nobody ever will use ?  

> +#endif
>  #ifdef CONFIG_INOTIFY_USER
>  	{
>  		.procname	= "inotify",
> diff --git a/kernel/time.c b/kernel/time.c
> index ba9b338..b4155b8 100644
> --- a/kernel/time.c
> +++ b/kernel/time.c
> @@ -92,7 +92,9 @@ SYSCALL_DEFINE1(stime, time_t __user *, tptr)
>  	if (err)
>  		return err;
>  
> -	do_settimeofday(&tv);
> +	err = do_settimeofday(&tv);
> +	if (!err)
> +		time_notify_all(TIME_EVENT_SET);

Crap. We really do _NOT_ want to sprinkle this into all syscalls which
can be used to set the time. You already missed the case in
do_sys_settimeofday() when only a tz change happens. Can't we simply
put that into do_settimeofday() and cover all cases ?

>  }
> @@ -215,6 +220,8 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
>  	if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
>  		return -EFAULT;
>  	ret = do_adjtimex(&txc);
> +	if (!ret)
> +		time_notify_all(TIME_EVENT_ADJ);

That's nonsense. adjtimex is not necessarily setting the time. adjtimex
can just read the current values w/o doing any modification. Also if
adjtimex merily adjusts the clock drift parameters then we do NOT set
the clock. So this is rather pointless AFAICT.

> +/*
> + * A process can "subscribe" to receive a notification via eventfd that
> + * some other process has called stime/settimeofday/adjtimex.
> + */
> +struct time_event {
> +	struct eventfd_ctx	*eventfd;
> +	struct task_struct	*watcher;
> +	unsigned int		want_others:1;
> +	unsigned int		want_own:1;
> +	unsigned int		want_set:1;
> +	unsigned int		want_adj:1;
> +	struct work_struct	remove;
> +	wait_queue_t		wq;
> +	wait_queue_head_t	*wqh;
> +	poll_table		pt;
> +	struct list_head	list;
> +};
> +
> +static LIST_HEAD(event_list);
> +static int nevents;
> +static DEFINE_SPINLOCK(event_lock);
> +
> +/*
> + * Do the necessary cleanup when the eventfd is being closed
> + */
> +static void time_event_remove(struct work_struct *work)
> +{
> +	struct time_event *evt = container_of(work, struct time_event, remove);
> +
> +	BUG_ON(nevents <= 0);

  Errm. The accounting belongs to the place where we remove the event
  from the event list. This counter can go away anyway.

  You leak the refcount on the eventfd_ctx here.

> +
> +	kfree(evt);
> +	nevents--;
> +}
> +
> +static int time_event_wakeup(wait_queue_t *wq, unsigned int mode, int sync,
> +			     void *key)
> +{
> +	struct time_event *evt = container_of(wq, struct time_event, wq);
> +	unsigned long flags = (unsigned long)key;
> +
> +	if (flags & POLLHUP) {
> +		__remove_wait_queue(evt->wqh, &evt->wq);

  Please don't do that. Merily delete it from the list and schedule it
  for removal. Then in the remove function use
  eventfd_ctx_remove_wait_queue() to cleanup the eventfd.

> +		spin_lock(&event_lock);
> +		list_del(&evt->list);
> +		spin_unlock(&event_lock);
> +
> +		schedule_work(&evt->remove);
> +	}
> +
> +	return 0;
> +}
> +
> +static void time_event_ptable_queue_proc(struct file *file,
> +					 wait_queue_head_t *wqh, poll_table *pt)
> +{
> +	struct time_event *evt = container_of(pt, struct time_event, pt);
> +
> +	evt->wqh = wqh;
> +	add_wait_queue(wqh, &evt->wq);
> +}
> +
> +/*
> + * time_change_notify() registers a given eventfd to receive time change
> + * notifications
> + */
> +SYSCALL_DEFINE2(time_change_notify, int, fd, unsigned int, flags)
> +{
> +	int ret;
> +	struct file *file;
> +	struct time_event *evt;

  Reverting the order of these makes is 5 times easier to read.

> +
> +	evt = kmalloc(sizeof(*evt), GFP_KERNEL);
> +	if (!evt)
> +		return -ENOMEM;
> +
> +	evt->want_others = !!(flags & TIME_CHANGE_NOTIFY_OTHERS);
> +	evt->want_own = !!(flags & TIME_CHANGE_NOTIFY_OWN);
> +	evt->want_set = !!(flags & TIME_CHANGE_NOTIFY_SET);
> +	evt->want_adj = !!(flags & TIME_CHANGE_NOTIFY_ADJUST);

  Shudder. Please get rid of this.

> +	file = eventfd_fget(fd);
> +	if (IS_ERR(file)) {
> +		ret = -EINVAL;

  Grr. PTR_ERR(file) perhaps ?

> +		goto out_free;
> +	}
> +
> +	evt->eventfd = eventfd_ctx_fileget(file);
> +	if (IS_ERR(evt->eventfd)) {
> +		ret = PTR_ERR(evt->eventfd);
> +		goto out_fput;
> +	}
> +
> +	INIT_LIST_HEAD(&evt->list);
> +	INIT_WORK(&evt->remove, time_event_remove);
> +
> +	init_waitqueue_func_entry(&evt->wq, time_event_wakeup);
> +	init_poll_funcptr(&evt->pt, time_event_ptable_queue_proc);
> +
> +	evt->watcher = current;

  No need for this

> +	spin_lock(&event_lock);
> +	if (nevents == time_change_notify_max_users) {
> +		spin_unlock(&event_lock);
> +		ret = -EBUSY;


  Gah. You leak the refcount to eventfd.

> +		goto out_fput;
> +	}
> +
> +	nevents++;
> +	list_add(&evt->list, &event_list);
> +	spin_unlock(&event_lock);
> +
> +	if (file->f_op->poll(file, &evt->pt) & POLLHUP) {
> +		ret = 0;

  Ditto.

> +		goto out_fput;
> +	}
> +
> +	fput(file);
> +
> +	return 0;
> +
> +out_fput:
> +	fput(file);
> +
> +out_free:
> +	kfree(evt);
> +
> +	return ret;
> +}
> +
> +void time_notify_all(int type)
> +{
> +	struct list_head *tmp;
> +
> +	spin_lock(&event_lock);
> +	list_for_each(tmp, &event_list) {
> +		struct time_event *e = container_of(tmp, struct time_event,
> +						    list);
> +
> +		if (type == TIME_EVENT_SET && !e->want_set)
> +			continue;
> +		else if (type == TIME_EVENT_ADJ && !e->want_adj)
> +			continue;
> +
> +		if (e->watcher == current && !e->want_own)
> +			continue;
> +		else if (e->watcher != current && !e->want_others)
> +			continue;

  This needs a major cleanup.

> +		eventfd_signal(e->eventfd, 1);
> +	}
> +	spin_unlock(&event_lock);
> +}
> +
> +static int time_notify_init(void)
> +{
> +	return 0;
> +}
> +
> +core_initcall(time_notify_init);

What's the exact purpose of this initcall ?

Thanks,

	tglx

Re: [PATCH 1/7] notify userspace about time changes

[Kay Sievers]

On Fri, Sep 17, 2010 at 12:33, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Certain userspace applications (like "clock" desktop applets or cron) might
>> want to be notified when some other application changes the system time. It
>> might also be important for an application to be able to distinguish between
>> its own and somebody else's time changes.
>
> A program that cannot work out if it or someone else changed the time is
> very very broken indeed !

Yeah, that seems a bit weird to me too.

> Clocks apps don't care because they check the actual time so notice it
> shfited. Cron and anacron appear to contain the needed internal handling.

Cron wakes up every minute to check if the time has changed. We don't
want such silly behavior, but there are no other options at the moment
for scheduling re-occurring events.

> Anything sleeping until a time occurs maybe ? In which case its a lot
> simpler and cleaner than events to provide a new itimer which wakes the
> process when the wall time hits the time specified in the timer.

That seems not sufficient. The details should be in the changelog of the patch.

Kay

Re: [PATCH 1/7] notify userspace about time changes

[Alan Cox]

> Certain userspace applications (like "clock" desktop applets or cron) might
> want to be notified when some other application changes the system time. It
> might also be important for an application to be able to distinguish between
> its own and somebody else's time changes.

A program that cannot work out if it or someone else changed the time is
very very broken indeed !

> This patch implements a notification interface via eventfd mechanism. Proccess
> wishing to be notified about time changes should create an eventfd and pass it
> to time_change_notify() syscall along with notification options.

This seems complete overkill and it doesn't really help applications much
that I can see because of suspend/resume.

What are your actual use cases ?

Clocks apps don't care because they check the actual time so notice it
shfited. Cron and anacron appear to contain the needed internal handling.

Anything sleeping until a time occurs maybe ? In which case its a lot
simpler and cleaner than events to provide a new itimer which wakes the
process when the wall time hits the time specified in the timer.

Alan

Re: [PATCH 1/7] notify userspace about time changes

[Thomas Gleixner]

On Fri, 17 Sep 2010, Alan Cox wrote:

> > Certain userspace applications (like "clock" desktop applets or cron) might
> > want to be notified when some other application changes the system time. It
> > might also be important for an application to be able to distinguish between
> > its own and somebody else's time changes.
> 
> A program that cannot work out if it or someone else changed the time is
> very very broken indeed !
> 
> > This patch implements a notification interface via eventfd mechanism. Proccess
> > wishing to be notified about time changes should create an eventfd and pass it
> > to time_change_notify() syscall along with notification options.
> 
> This seems complete overkill and it doesn't really help applications much
> that I can see because of suspend/resume.
> 
> What are your actual use cases ?
> 
> Clocks apps don't care because they check the actual time so notice it
> shfited. Cron and anacron appear to contain the needed internal handling.
> 
> Anything sleeping until a time occurs maybe ? In which case its a lot
> simpler and cleaner than events to provide a new itimer which wakes the
> process when the wall time hits the time specified in the timer.

We already have that. posix timers provide this.

The only case I can imagine where a notification might be interesting
is when something armed an absolute timer on CLOCK_REALTIME and time
is set backwards.

Thanks,

	tglx

Re: [PATCH 1/7] notify userspace about time changes

[Alexander Shishkin]

On Fri, Sep 17, 2010 at 12:22:36 +0200, Kay Sievers wrote:
> On Fri, Sep 17, 2010 at 12:33, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> Certain userspace applications (like "clock" desktop applets or cron) might
> >> want to be notified when some other application changes the system time. It
> >> might also be important for an application to be able to distinguish between
> >> its own and somebody else's time changes.
> >
> > A program that cannot work out if it or someone else changed the time is
> > very very broken indeed !
> 
> Yeah, that seems a bit weird to me too.

Ok, I get it. Filtering has to go.

> > Clocks apps don't care because they check the actual time so notice it
> > shfited. Cron and anacron appear to contain the needed internal handling.
> 
> Cron wakes up every minute to check if the time has changed. We don't
> want such silly behavior, but there are no other options at the moment
> for scheduling re-occurring events.
> 
> > Anything sleeping until a time occurs maybe ? In which case its a lot
> > simpler and cleaner than events to provide a new itimer which wakes the
> > process when the wall time hits the time specified in the timer.
> 
> That seems not sufficient. The details should be in the changelog of the patch.

Yes, I did include your description of the problem in the 0/7, but
that doesn't seem sufficient. I really need to compile all the
usecases from previous threads and include them in 1/7 as well.

Regards,
--
Alex

Re: [PATCHv5 0/7] system time changes notification

[Thomas Gleixner]

On Fri, 17 Sep 2010, Alexander Shishkin wrote:

> Hi,
> 
> This is the fifth version of system time change notification mechanism
> for linux kernel. The need for it comes from applications which would
> like to keep track of time changes without having to wake up every
> $TIMEOUT and calling gettimeofday().
> 
> An excellent description for one of the usecases, written by Kay Sievers
> (http://kerneltrap.org/mailarchive/linux-kernel/2010/8/5/4603531):
> """
> This is the example Lennart and I thought about when we considered
> adding cron-like stuff to systemd's timer configs, but didn't want to
> do silly things like scheduled checks for the actual time, so we
> delayed this feature until such a notification becomes available.
> 
> Consider we want stuff like "wakeup every day at 3pm", the next wakeup
> might be earlier than the timer we calculated last time, on system
> time changes. We need to re-calculate it. This is necessary for all
> repeating events.
> 
> Say we want to wakeup at 3pm, now it's 4pm, so we schedule it in 23
> hours. Now the system time changes to 2pm, and we would expect to
> wakeup in one hour, but we take 25.

And that's why we have posix-timers with the ability to arm absolute
timers. They already deal with the clock being set.

man timer_settime

Thanks,

	tglx

Re: [PATCHv5 0/7] system time changes notification

[Thomas Gleixner]

On Fri, 17 Sep 2010, Kyle Moffett wrote:
> On Fri, Sep 17, 2010 at 08:57, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Fri, 17 Sep 2010, Alexander Shishkin wrote:
> > > Consider we want stuff like "wakeup every day at 3pm", the next wakeup
> > > might be earlier than the timer we calculated last time, on system
> > > time changes. We need to re-calculate it. This is necessary for all
> > > repeating events.
> > >
> > > Say we want to wakeup at 3pm, now it's 4pm, so we schedule it in 23
> > > hours. Now the system time changes to 2pm, and we would expect to
> > > wakeup in one hour, but we take 25.
> >
> > And that's why we have posix-timers with the ability to arm absolute
> > timers. They already deal with the clock being set.
> >
> > man timer_settime
> >
> 
> This still doesn't help with the specified use-case.  For example, consider
> the case of configuring my crontab to run a command every day at 2PM.
> 
> Let's assume that I then start the cron daemon at 2:05PM, and it establishes
> an absolute timer at 2PM the following day.
>
> I then realize that the clock is wrong, and correct it to read 1:55PM.
> 
> The problem is... cron's timer is still set to trigger at 2PM *tomorrow*,
> instead of being readjusted at the time the clock is changed to go off at
> 2PM *today*.
> 
> Right now there's really no way to fix that other than polling every so
> often to recheck the system time is what you expect.

Hmm, ok.

So what you really want is a timer which drops back to user space with
an appropriate error code when something fiddled with the clock.

That's reasonably easy to implement as an extension at least for
clock_nanosleep. For the signal based timers it'd be probably quite
nasty, but doable.

Thanks,

	tglx

Re: [PATCH 5/7] wire up sys_time_change_notify() on blackfin

[Mike Frysinger]

On Thu, Sep 16, 2010 at 18:10, Alexander Shishkin wrote:
> +#define __NR_time_change_notify        374

we're in the process of allocating NR 374 for cacheflush (for
userspace trampoline handling with gcc), so this will need to be
changed
-mike

Re: [PATCH 1/7] notify userspace about time changes

[Alexander Shishkin]

On Fri, Sep 17, 2010 at 11:29:40 +0200, Thomas Gleixner wrote:
> On Fri, 17 Sep 2010, Alexander Shishkin wrote:
> 
> > Certain userspace applications (like "clock" desktop applets or cron) might
> > want to be notified when some other application changes the system time. It
> > might also be important for an application to be able to distinguish between
> > its own and somebody else's time changes.
> 
> That's overengineered. An application which fiddles with the time
> should be able to deal with that inside of the application.

Good point.

> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index e6319d1..789f92e 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -819,6 +819,7 @@ asmlinkage long sys_fanotify_init(unsigned int flags, unsigned int event_f_flags
> >  asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
> >  				  u64 mask, int fd,
> >  				  const char  __user *pathname);
> > +asmlinkage long sys_time_change_notify(int fd, unsigned int flags);
> 
> Please do not create a new syscall with a weird flag field. As I said
> above the selection of who changed the time is not necessary at all,
> so what you want is the selection of which change event and that
> should be an enum.

What if the application wants to be notified about events of both types?
enum { A, B, BOTH }; is more awkward than having them bitwise or'ed. Especially
if one day another event type turns up.
Userspace can, of course, have separate eventfds for different events, but
isn't that sort of interface limitation for no gain?

> >  int kernel_execve(const char *filename, const char *const argv[], const char *const envp[]);
> >  
> > diff --git a/include/linux/time.h b/include/linux/time.h
> > index 9f15ac7..d66045e 100644
> > --- a/include/linux/time.h
> > +++ b/include/linux/time.h
> > @@ -252,6 +252,26 @@ static __always_inline void timespec_add_ns(struct timespec *a, u64 ns)
> >  	a->tv_sec += __iter_div_u64_rem(a->tv_nsec + ns, NSEC_PER_SEC, &ns);
> >  	a->tv_nsec = ns;
> >  }
> > +
> > +/* time change events */
> > +#define TIME_EVENT_SET 0
> > +#define TIME_EVENT_ADJ 1
> 
> Enum please
> 
> > +#define TIME_CHANGE_NOTIFY_OTHERS	BIT(0)
> > +#define TIME_CHANGE_NOTIFY_OWN		BIT(1)
> > +#define TIME_CHANGE_NOTIFY_SET		BIT(2)
> > +#define TIME_CHANGE_NOTIFY_ADJUST	BIT(3)
> > +
> > +#define TIME_CHANGE_NOTIFY_MAX_USERS	1024
> 
> What's this random limit for ?

Well, I guess as long as some sensible rlimits are in effect, this is
indeed not needed.

> > diff --git a/init/Kconfig b/init/Kconfig
> > index 2de5b1c..504a51a 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -980,6 +980,13 @@ config PERF_USE_VMALLOC
> >  	help
> >  	  See tools/perf/design.txt for details
> >  
> > +config TIME_NOTIFY
> > +	bool "System time changes notification for userspace"
> > +	depends on EVENTFD
> > +	help
> > +	  Enable time change notification events to userspace via
> > +	  eventfd.
> > +
> 
> How is that config switch related to PERF ?   
> 
> >  menu "Kernel Performance Events And Counters"
> >  
> >  config PERF_EVENTS
> > diff --git a/kernel/Makefile b/kernel/Makefile
> > index 0b72d1a..ac53c67 100644
> > --- a/kernel/Makefile
> > +++ b/kernel/Makefile
> > @@ -104,6 +104,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_event.o
> >  obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
> >  obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
> >  obj-$(CONFIG_PADATA) += padata.o
> > +obj-$(CONFIG_TIME_NOTIFY) += time_notify.o
> 
> Please move this to kernel/time/

Ok.

> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index f88552c..9e7d7a9 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -1441,6 +1441,17 @@ static struct ctl_table fs_table[] = {
> >  		.proc_handler	= proc_doulongvec_minmax,
> >  	},
> >  #endif /* CONFIG_AIO */
> > +#ifdef CONFIG_TIME_NOTIFY
> > +	{
> > +		.procname	= "time-change-notify-max-users",
> > +		.data		= &time_change_notify_max_users,
> > +		.maxlen		= sizeof(unsigned int),
> > +		.mode		= 0644,
> > +		.proc_handler	= proc_dointvec_minmax,
> > +		.extra1		= &zero,
> > +		.extra2		= &ten_thousand,
> > +	},
> 
> Do we really need another sysctl which nobody ever will use ?  

If we're sure that we don't want to limit the number of users, we
don't need this as well.

> > +#endif
> >  #ifdef CONFIG_INOTIFY_USER
> >  	{
> >  		.procname	= "inotify",
> > diff --git a/kernel/time.c b/kernel/time.c
> > index ba9b338..b4155b8 100644
> > --- a/kernel/time.c
> > +++ b/kernel/time.c
> > @@ -92,7 +92,9 @@ SYSCALL_DEFINE1(stime, time_t __user *, tptr)
> >  	if (err)
> >  		return err;
> >  
> > -	do_settimeofday(&tv);
> > +	err = do_settimeofday(&tv);
> > +	if (!err)
> > +		time_notify_all(TIME_EVENT_SET);
> 
> Crap. We really do _NOT_ want to sprinkle this into all syscalls which
> can be used to set the time. You already missed the case in

I do hope that there aren't any new ones coming.

> do_sys_settimeofday() when only a tz change happens. Can't we simply
> put that into do_settimeofday() and cover all cases ?

Yes, that's a better idea.

> >  }
> > @@ -215,6 +220,8 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
> >  	if(copy_from_user(&txc, txc_p, sizeof(struct timex)))
> >  		return -EFAULT;
> >  	ret = do_adjtimex(&txc);
> > +	if (!ret)
> > +		time_notify_all(TIME_EVENT_ADJ);
> 
> That's nonsense. adjtimex is not necessarily setting the time. adjtimex
> can just read the current values w/o doing any modification. Also if
> adjtimex merily adjusts the clock drift parameters then we do NOT set
> the clock. So this is rather pointless AFAICT.

Well, I have an application here that would very much like to be notified
if something tries to adjust the clock drift. But yes, I should check for
txc->modes here.

> > +/*
> > + * A process can "subscribe" to receive a notification via eventfd that
> > + * some other process has called stime/settimeofday/adjtimex.
> > + */
> > +struct time_event {
> > +	struct eventfd_ctx	*eventfd;
> > +	struct task_struct	*watcher;
> > +	unsigned int		want_others:1;
> > +	unsigned int		want_own:1;
> > +	unsigned int		want_set:1;
> > +	unsigned int		want_adj:1;
> > +	struct work_struct	remove;
> > +	wait_queue_t		wq;
> > +	wait_queue_head_t	*wqh;
> > +	poll_table		pt;
> > +	struct list_head	list;
> > +};
> > +
> > +static LIST_HEAD(event_list);
> > +static int nevents;
> > +static DEFINE_SPINLOCK(event_lock);
> > +
> > +/*
> > + * Do the necessary cleanup when the eventfd is being closed
> > + */
> > +static void time_event_remove(struct work_struct *work)
> > +{
> > +	struct time_event *evt = container_of(work, struct time_event, remove);
> > +
> > +	BUG_ON(nevents <= 0);
> 
>   Errm. The accounting belongs to the place where we remove the event
>   from the event list. This counter can go away anyway.
> 
>   You leak the refcount on the eventfd_ctx here.
> 
> > +
> > +	kfree(evt);
> > +	nevents--;
> > +}
> > +
> > +static int time_event_wakeup(wait_queue_t *wq, unsigned int mode, int sync,
> > +			     void *key)
> > +{
> > +	struct time_event *evt = container_of(wq, struct time_event, wq);
> > +	unsigned long flags = (unsigned long)key;
> > +
> > +	if (flags & POLLHUP) {
> > +		__remove_wait_queue(evt->wqh, &evt->wq);
> 
>   Please don't do that. Merily delete it from the list and schedule it
>   for removal. Then in the remove function use
>   eventfd_ctx_remove_wait_queue() to cleanup the eventfd.

Ok, but I can't quite see the difference in this particular case, when we
only care about POLLHUP (apart from using eventfd interface for waitqueue
removal). There's a spinlock in that function, but I'm not quite sure what
can race when we're in POLLHUP situation. What am I missing?

> > +		spin_lock(&event_lock);
> > +		list_del(&evt->list);
> > +		spin_unlock(&event_lock);
> > +
> > +		schedule_work(&evt->remove);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void time_event_ptable_queue_proc(struct file *file,
> > +					 wait_queue_head_t *wqh, poll_table *pt)
> > +{
> > +	struct time_event *evt = container_of(pt, struct time_event, pt);
> > +
> > +	evt->wqh = wqh;
> > +	add_wait_queue(wqh, &evt->wq);
> > +}
> > +
> > +/*
> > + * time_change_notify() registers a given eventfd to receive time change
> > + * notifications
> > + */
> > +SYSCALL_DEFINE2(time_change_notify, int, fd, unsigned int, flags)
> > +{
> > +	int ret;
> > +	struct file *file;
> > +	struct time_event *evt;
> 
>   Reverting the order of these makes is 5 times easier to read.

Very true.

> > +
> > +	evt = kmalloc(sizeof(*evt), GFP_KERNEL);
> > +	if (!evt)
> > +		return -ENOMEM;
> > +
> > +	evt->want_others = !!(flags & TIME_CHANGE_NOTIFY_OTHERS);
> > +	evt->want_own = !!(flags & TIME_CHANGE_NOTIFY_OWN);
> > +	evt->want_set = !!(flags & TIME_CHANGE_NOTIFY_SET);
> > +	evt->want_adj = !!(flags & TIME_CHANGE_NOTIFY_ADJUST);
> 
>   Shudder. Please get rid of this.

Ok.

> > +	file = eventfd_fget(fd);
> > +	if (IS_ERR(file)) {
> > +		ret = -EINVAL;
> 
>   Grr. PTR_ERR(file) perhaps ?

Indeed.

> > +		goto out_free;
> > +	}
> > +
> > +	evt->eventfd = eventfd_ctx_fileget(file);
> > +	if (IS_ERR(evt->eventfd)) {
> > +		ret = PTR_ERR(evt->eventfd);
> > +		goto out_fput;
> > +	}
> > +
> > +	INIT_LIST_HEAD(&evt->list);
> > +	INIT_WORK(&evt->remove, time_event_remove);
> > +
> > +	init_waitqueue_func_entry(&evt->wq, time_event_wakeup);
> > +	init_poll_funcptr(&evt->pt, time_event_ptable_queue_proc);
> > +
> > +	evt->watcher = current;
> 
>   No need for this
> 
> > +	spin_lock(&event_lock);
> > +	if (nevents == time_change_notify_max_users) {
> > +		spin_unlock(&event_lock);
> > +		ret = -EBUSY;
> 
> 
>   Gah. You leak the refcount to eventfd.
> 
> > +		goto out_fput;
> > +	}
> > +
> > +	nevents++;
> > +	list_add(&evt->list, &event_list);
> > +	spin_unlock(&event_lock);
> > +
> > +	if (file->f_op->poll(file, &evt->pt) & POLLHUP) {
> > +		ret = 0;
> 
>   Ditto.
> 
> > +		goto out_fput;
> > +	}
> > +
> > +	fput(file);
> > +
> > +	return 0;
> > +
> > +out_fput:
> > +	fput(file);
> > +
> > +out_free:
> > +	kfree(evt);
> > +
> > +	return ret;
> > +}
> > +
> > +void time_notify_all(int type)
> > +{
> > +	struct list_head *tmp;
> > +
> > +	spin_lock(&event_lock);
> > +	list_for_each(tmp, &event_list) {
> > +		struct time_event *e = container_of(tmp, struct time_event,
> > +						    list);
> > +
> > +		if (type == TIME_EVENT_SET && !e->want_set)
> > +			continue;
> > +		else if (type == TIME_EVENT_ADJ && !e->want_adj)
> > +			continue;
> > +
> > +		if (e->watcher == current && !e->want_own)
> > +			continue;
> > +		else if (e->watcher != current && !e->want_others)
> > +			continue;
> 
>   This needs a major cleanup.
> 
> > +		eventfd_signal(e->eventfd, 1);
> > +	}
> > +	spin_unlock(&event_lock);
> > +}
> > +
> > +static int time_notify_init(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +core_initcall(time_notify_init);
> 
> What's the exact purpose of this initcall ?

This should have been removed, yes.

Regards,
--
Alex