[PATCH] back out CPU clock additions to posix-timers

[PATCH] back out CPU clock additions to posix-timers

[Roland McGrath]

> Roland McGrath <roland@redhat.com> wrote:
> >
> > I really think we should not let the
> > existing clockid_t encoding change get out in 2.6.10.
> 
> So.. could you please send a patch which disables the userspace-visibility
> of Christoph's changes?

Here you go.  I'm gathering I won't convince you to get the final form of
the new code into 2.6.10, in which case just holding off on the addition by
doing this reversion patch for 2.6.10 and putting the new stuff in 2.6.11
is what I'd like to see.  Cristoph and I will work through the nits and
cleanups the patch I posted needs; I expect we'll have a consensus version
of the patch for you soon.

I ran glibc's clock and timer tests on a vanilla Linus kernel of today plus
this patch, and they behaved properly for a kernel not supporting any
process-CPU clockid_t values.


Thanks,
Roland



[PATCH] back out CPU clock additions to posix-timers

This patch reverts the additions of an ABI supporting thread and process
CPU clocks in the posix-timers code.  This returns us to 2.6.9's condition,
there is no support for any new clockid_t values for process CPU clocks.

This also fixes the return value for clock_nanosleep when unsupported (I
think this is used only by sgi-timer at the moment).  The POSIX-specified
code for valid clocks that don't support the sleep operation is ENOTSUP.
On most architectures the kernel doesn't define ENOTSUP and this name is
defined in userland the same as the kernel's EOPNOTSUPP.

Signed-off-by: Roland McGrath <roland@redhat.com>

--- linux-2.6/kernel/posix-timers.c
+++ linux-2.6/kernel/posix-timers.c
@@ -10,10 +10,6 @@
  * 2004-06-01  Fix CLOCK_REALTIME clock/timer TIMER_ABSTIME bug.
  *			     Copyright (C) 2004 Boris Hu
  *
- * 2004-07-27 Provide POSIX compliant clocks
- *		CLOCK_PROCESS_CPUTIME_ID and CLOCK_THREAD_CPUTIME_ID.
- *		by Christoph Lameter
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or (at
@@ -193,8 +189,6 @@ static int do_posix_gettime(struct k_clo
 static u64 do_posix_clock_monotonic_gettime_parts(
 	struct timespec *tp, struct timespec *mo);
 int do_posix_clock_monotonic_gettime(struct timespec *tp);
-static int do_posix_clock_process_gettime(struct timespec *tp);
-static int do_posix_clock_thread_gettime(struct timespec *tp);
 static struct k_itimer *lock_timer(timer_t timer_id, unsigned long *flags);
 
 static inline void unlock_timer(struct k_itimer *timr, unsigned long flags)
@@ -215,25 +209,9 @@ static __init int init_posix_timers(void
 		.clock_get = do_posix_clock_monotonic_gettime,
 		.clock_set = do_posix_clock_nosettime
 	};
-	struct k_clock clock_thread = {.res = CLOCK_REALTIME_RES,
-		.abs_struct = NULL,
-		.clock_get = do_posix_clock_thread_gettime,
-		.clock_set = do_posix_clock_nosettime,
-		.timer_create = do_posix_clock_notimer_create,
-		.nsleep = do_posix_clock_nonanosleep
-	};
-	struct k_clock clock_process = {.res = CLOCK_REALTIME_RES,
-		.abs_struct = NULL,
-		.clock_get = do_posix_clock_process_gettime,
-		.clock_set = do_posix_clock_nosettime,
-		.timer_create = do_posix_clock_notimer_create,
-		.nsleep = do_posix_clock_nonanosleep
-	};
 
 	register_posix_clock(CLOCK_REALTIME, &clock_realtime);
 	register_posix_clock(CLOCK_MONOTONIC, &clock_monotonic);
-	register_posix_clock(CLOCK_PROCESS_CPUTIME_ID, &clock_process);
-	register_posix_clock(CLOCK_THREAD_CPUTIME_ID, &clock_thread);
 
 	posix_timers_cache = kmem_cache_create("posix_timers_cache",
 					sizeof (struct k_itimer), 0, 0, NULL, NULL);
@@ -1220,69 +1198,18 @@ int do_posix_clock_nosettime(struct time
 	return -EINVAL;
 }
 
-int do_posix_clock_notimer_create(struct k_itimer *timer) {
-	return -EINVAL;
-}
-
-int do_posix_clock_nonanosleep(int which_lock, int flags,struct timespec * t) {
-/* Single Unix specficiation says to return ENOTSUP but we do not have that */
-	return -EINVAL;
-}
-
-static unsigned long process_ticks(task_t *p) {
-	unsigned long ticks;
-	task_t *t;
-
-	spin_lock(&p->sighand->siglock);
-	/* The signal structure is shared between all threads */
-	ticks = p->signal->utime + p->signal->stime;
-
-	/* Add up the cpu time for all the still running threads of this process */
-	t = p;
-	do {
-		ticks += t->utime + t->stime;
-		t = next_thread(t);
-	} while (t != p);
-
-	spin_unlock(&p->sighand->siglock);
-	return ticks;
-}
-
-static inline unsigned long thread_ticks(task_t *p) {
-	return p->utime + current->stime;
-}
-
-/*
- * Single Unix Specification V3:
- *
- * Implementations shall also support the special clockid_t value
- * CLOCK_THREAD_CPUTIME_ID, which represents the CPU-time clock of the calling
- * thread when invoking one of the clock_*() or timer_*() functions. For these
- * clock IDs, the values returned by clock_gettime() and specified by
- * clock_settime() shall represent the amount of execution time of the thread
- * associated with the clock.
- */
-static int do_posix_clock_thread_gettime(struct timespec *tp)
+int do_posix_clock_notimer_create(struct k_itimer *timer)
 {
-	jiffies_to_timespec(thread_ticks(current), tp);
-	return 0;
+	return -EINVAL;
 }
 
-/*
- * Single Unix Specification V3:
- *
- * Implementations shall also support the special clockid_t value
- * CLOCK_PROCESS_CPUTIME_ID, which represents the CPU-time clock of the
- * calling process when invoking one of the clock_*() or timer_*() functions.
- * For these clock IDs, the values returned by clock_gettime() and specified
- * by clock_settime() represent the amount of execution time of the process
- * associated with the clock.
- */
-
-static int do_posix_clock_process_gettime(struct timespec *tp)
+int do_posix_clock_nonanosleep(int which_clock, int flags, struct timespec *t)
 {
-	jiffies_to_timespec(process_ticks(current), tp);
-	return 0;
+#ifndef ENOTSUP
+	return -EOPNOTSUPP;	/* aka ENOTSUP in userland for POSIX */
+#else  /*  parisc does define it separately.  */
+	return -ENOTSUP;
+#endif
 }
 
 asmlinkage long
@@ -1290,10 +1217,6 @@ sys_clock_settime(clockid_t which_clock,
 {
 	struct timespec new_tp;
 
-	/* Cannot set process specific clocks */
-	if (which_clock<0)
-		return -EINVAL;
-
 	if ((unsigned) which_clock >= MAX_CLOCKS ||
 					!posix_clocks[which_clock].res)
 		return -EINVAL;
@@ -1307,29 +1230,6 @@ sys_clock_settime(clockid_t which_clock,
 
 static int do_clock_gettime(clockid_t which_clock, struct timespec *tp)
 {
-	/* Process process specific clocks */
-	if (which_clock < 0) {
-		task_t *t;
-		int pid = -which_clock;
-
-		if (pid < PID_MAX_LIMIT) {
-			if ((t = find_task_by_pid(pid))) {
-				jiffies_to_timespec(process_ticks(t), tp);
-				return 0;
-			}
-			return -EINVAL;
-		}
-		if (pid < 2*PID_MAX_LIMIT) {
-			if ((t = find_task_by_pid(pid - PID_MAX_LIMIT))) {
-				jiffies_to_timespec(thread_ticks(t), tp);
-				return 0;
-			}
-			return -EINVAL;
-		}
-		/* More process specific clocks could follow here */
-		return -EINVAL;
-	}
-
 	if ((unsigned) which_clock >= MAX_CLOCKS ||
 					!posix_clocks[which_clock].res)
 		return -EINVAL;
@@ -1356,9 +1256,6 @@ sys_clock_getres(clockid_t which_clock, 
 {
 	struct timespec rtn_tp;
 
-	/* All process clocks have the resolution of CLOCK_PROCESS_CPUTIME_ID */
-	if (which_clock < 0 ) which_clock = CLOCK_PROCESS_CPUTIME_ID;
-
 	if ((unsigned) which_clock >= MAX_CLOCKS ||
 					!posix_clocks[which_clock].res)
 		return -EINVAL;

Re: [PATCH] back out CPU clock additions to posix-timers

[Christoph Lameter]

On Sat, 18 Dec 2004, Roland McGrath wrote:

> > Roland McGrath <roland@redhat.com> wrote:
> > >
> > > I really think we should not let the
> > > existing clockid_t encoding change get out in 2.6.10.
> >
> > So.. could you please send a patch which disables the userspace-visibility
> > of Christoph's changes?

I am not sure what the point of these is? The userspace visibility of the
regular posix clocks (positive clockid's) does not change. The way we
encode process cputime clocks as negative values changes but there is
nothing that supports my encoding yet (apart from my test code) since
the glibc patch was never accepted.

I would like to keep the support for all 4 standard posix clocks through
clockids 0-3 as listed in the kernel headers and simply have the cputime
clocks redirect to your code appropriately to get the current values for
each process. IMHO that makes the interface cleaner, is cleaner for glibc
since it allows simply to pass positive clockids through  and also
maintains compatibility for positive clockids to the existing situation.

Re: [PATCH] back out CPU clock additions to posix-timers

[Christoph Lameter]

Here is a patch that simply removes support for negative clockids from
2.6.10-rc. The support for positive clock_id should stay the same in
future kernels and follow straighforwardly the posix API without special
encodigns for CLOCK_*_CPUTIME_ID. Roland's patches would implement
negative clockid's to access process clocks but keep the existing
interface for positive clock_ids.

Index: linux-2.6.9/kernel/posix-timers.c
===================================================================
--- linux-2.6.9.orig/kernel/posix-timers.c	2004-12-17 14:40:16.000000000 -0800
+++ linux-2.6.9/kernel/posix-timers.c	2004-12-19 19:35:43.000000000 -0800
@@ -1307,28 +1307,8 @@

 static int do_clock_gettime(clockid_t which_clock, struct timespec *tp)
 {
-	/* Process process specific clocks */
-	if (which_clock < 0) {
-		task_t *t;
-		int pid = -which_clock;
-
-		if (pid < PID_MAX_LIMIT) {
-			if ((t = find_task_by_pid(pid))) {
-				jiffies_to_timespec(process_ticks(t), tp);
-				return 0;
-			}
-			return -EINVAL;
-		}
-		if (pid < 2*PID_MAX_LIMIT) {
-			if ((t = find_task_by_pid(pid - PID_MAX_LIMIT))) {
-				jiffies_to_timespec(thread_ticks(t), tp);
-				return 0;
-			}
-			return -EINVAL;
-		}
-		/* More process specific clocks could follow here */
+	if (which_clock < 0)
 		return -EINVAL;
-	}

 	if ((unsigned) which_clock >= MAX_CLOCKS ||
 					!posix_clocks[which_clock].res)
@@ -1356,8 +1336,8 @@
 {
 	struct timespec rtn_tp;

-	/* All process clocks have the resolution of CLOCK_PROCESS_CPUTIME_ID */
-	if (which_clock < 0 ) which_clock = CLOCK_PROCESS_CPUTIME_ID;
+	if (which_clock < 0 )
+		return -EINVAL;

 	if ((unsigned) which_clock >= MAX_CLOCKS ||
 					!posix_clocks[which_clock].res)

Re: [PATCH] back out CPU clock additions to posix-timers

[Roland McGrath]

> I am not sure what the point of these is? 

Andrew doesn't want to put the new code in until after 2.6.10 is released.
He asked me to back out the userspace-visible changes since 2.6.9, so
2.6.10 will have no additions rather than having something different from
what we settle on finally.  


Thanks,
Roland

Re: [PATCH] back out CPU clock additions to posix-timers

[Roland McGrath]

Since Andrew is taking the conservative line, I think it's more prudent to
omit the whole thing from 2.6.10 rather than have a tentative definition of
what those two clock IDs mean that changes later.  

Re: [PATCH] back out CPU clock additions to posix-timers

[Christoph Lameter]

On Sun, 19 Dec 2004, Roland McGrath wrote:

> Since Andrew is taking the conservative line, I think it's more prudent to
> omit the whole thing from 2.6.10 rather than have a tentative definition of
> what those two clock IDs mean that changes later.

The conservative line is to keep a consistent definition of the interface
following posix as closely as possible. The definition of the 4 clockids
CLOCK_*_CPUTIME_ID, CLOCK_REALTIME and CLOCK_MONOTONIC should stay
constant and be implemented in a consistent way by the kernel. That is
the case now and should not be changed by any future patches.

Re: [PATCH] back out CPU clock additions to posix-timers

[Roland McGrath]

> The conservative line is to keep a consistent definition of the interface
> following posix as closely as possible. 

The conservative line for Linux is to not change the interface from 2.6.9,
period.  If Andrew prefers the partial changes, I don't have a strong
objection.  But it is by no means the conservative thing to do.

Re: [PATCH] back out CPU clock additions to posix-timers

[Christoph Lameter]

On Mon, 20 Dec 2004, Roland McGrath wrote:

> > The conservative line is to keep a consistent definition of the interface
> > following posix as closely as possible.
>
> The conservative line for Linux is to not change the interface from 2.6.9,
> period.  If Andrew prefers the partial changes, I don't have a strong
> objection.  But it is by no means the conservative thing to do.

Conservative means minimal changes to the existing tested code of
2.6.10-rcX and reduction of a potential future flux in the kernel API.