sched: sched_clock() clocksource handling.

kernel/time/clocksource.c +9 -0

@@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
 	if (next = curr_clocksource)
 		return NULL;
 
+	if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
+		sched_clocksource = next;
+
 	return next;
 }
 

@@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
 	list_del(&cs->list);
 	if (clocksource_override = cs)
 		clocksource_override = NULL;
+
 	next_clocksource = select_clocksource();
+
+	/*
+	 * If select_clocksource() fails to find another suitable
+	 * clocksource for sched_clocksource and we are unregistering
+	 * it, switch back to jiffies.

Quoted 21 lines · Paul Mundt

kernel/time/clocksource.c +9 -0

@@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
 	if (next = curr_clocksource)
 		return NULL;
 
+	if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
+		sched_clocksource = next;
+
 	return next;
 }
 

@@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
 	list_del(&cs->list);
 	if (clocksource_override = cs)
 		clocksource_override = NULL;
+
 	next_clocksource = select_clocksource();
+
+	/*
+	 * If select_clocksource() fails to find another suitable
+	 * clocksource for sched_clocksource and we are unregistering
+	 * it, switch back to jiffies.

kernel/time/clocksource.c void clocksource_unregister(struct clocksource *cs) · line 464

Quoted 10 lines

kernel/time/clocksource.c +8 -0

+	 */
+	if (sched_clocksource = cs) {
+		rcu_assign_pointer(sched_clocksource, &clocksource_jiffies);
+		spin_unlock_irqrestore(&clocksource_lock, flags);
+		synchronize_rcu();
+		return;
+	}
+
 	spin_unlock_irqrestore(&clocksource_lock, flags);
 }

What if there's multiple CLOCK_SOURCE_USER_FOR_SCHED_CLOCK [ damn, thats a long name to type :-) ] ?

That is, should we have logic in select_clocksource that does:

  if ((next->flags & ..) && next->prio > sched_clocksource->prio)

or whatever, so that it picks the best one?

Same for unregister, should we re-evaluate all clocksources before falling back to basic?

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

Quoted 7 lines · Paul Mundt

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Ah, ok. So unregister calls select_clocksource again? That does leave us a small window with jiffies, but I guess that's ok.

Quoted 3 lines

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

Right, that would want fixing indeed.

Quoted 3 lines

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

No, you only have to do sync_rcu() when stuff that could have referenced is going away and you cannot use call_rcu().

So when selecting a new clocksource, you don't need synchonization because stuff doesn't go away (I think :-)

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Paul Mundt

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Quoted 3 lines

Ah, ok. So unregister calls select_clocksource again? That does leave us a small window with jiffies, but I guess that's ok.

A synchronize_rcu() would fix that up, but I think a small window with jiffies is less painful than sorting out RCU ordering and synchronization for a corner case of a corner case ;-)

Quoted 10 lines

Quoted 3 lines

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

Right, that would want fixing indeed.

Quoted 3 lines

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

Quoted 5 lines

No, you only have to do sync_rcu() when stuff that could have referenced is going away and you cannot use call_rcu().

So when selecting a new clocksource, you don't need synchonization because stuff doesn't go away (I think :-)

Ok, that keeps things more simplified then. How does this look?

Quoted 33 lines · Paul Mundt

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Paul Mundt

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Quoted 3 lines

Ah, ok. So unregister calls select_clocksource again? That does leave us a small window with jiffies, but I guess that's ok.

A synchronize_rcu() would fix that up, but I think a small window with jiffies is less painful than sorting out RCU ordering and synchronization for a corner case of a corner case ;-)

Quoted 15 lines

Quoted 3 lines

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

Right, that would want fixing indeed.

Quoted 3 lines

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

No, you only have to do sync_rcu() when stuff that could have referenced is going away and you cannot use call_rcu().

So when selecting a new clocksource, you don't need synchonization because stuff doesn't go away (I think :-)

Quoted 1 line

Ok, that keeps things more simplified then. How does this look?

Looks fine to me,

Acked-by Peter Zijlstra <[email protected]>

unsigned long long __attribute__((weak)) sched_clock(void) {

kernel/sched_clock.c +0 -2

-       return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-                                       * (NSEC_PER_SEC / HZ);

Quoted 9 lines · Paul Mundt

unsigned long long __attribute__((weak)) sched_clock(void) {

kernel/sched_clock.c +5 -2

-       return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-                                       * (NSEC_PER_SEC / HZ);
+       unsigned long long time;
+       struct clocksource *clock;
+
+       rcu_read_lock();
+       clock = rcu_dereference(sched_clocksource);

kernel/sched_clock.c line 52

Quoted 5 lines

kernel/sched_clock.c +4 -0

+       time = cyc2ns(clock, clocksource_read(clock));
+       rcu_read_unlock();
+
+       return time;
 }

My concerns with the locking here still stand. Nothing you've said or done bolsters the clocksource in modules argument. I think what your

Quoted 12 lines · Paul Mundt

unsigned long long __attribute__((weak)) sched_clock(void) {

kernel/sched_clock.c +8 -2

-       return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-                                       * (NSEC_PER_SEC / HZ);
+       unsigned long long time;
+       struct clocksource *clock;
+
+       rcu_read_lock();
+       clock = rcu_dereference(sched_clocksource);
+       time = cyc2ns(clock, clocksource_read(clock));
+       rcu_read_unlock();
+

Quoted 2 lines

+ return time;
}

My concerns with the locking here still stand. Nothing you've said or done bolsters the clocksource in modules argument. I think what your planning for sh clocksources seems very inelegant. I would imagine a better solution is out there. I'd prefer if you just leave sched_clock alone.

Quoted 13 lines · Daniel Walker

Quoted 12 lines · Paul Mundt

unsigned long long __attribute__((weak)) sched_clock(void) {

kernel/sched_clock.c +8 -2

-       return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-                                       * (NSEC_PER_SEC / HZ);
+       unsigned long long time;
+       struct clocksource *clock;
+
+       rcu_read_lock();
+       clock = rcu_dereference(sched_clocksource);
+       time = cyc2ns(clock, clocksource_read(clock));
+       rcu_read_unlock();
+

Quoted 9 lines

Quoted 2 lines

+ return time;
}

My concerns with the locking here still stand. Nothing you've said or done bolsters the clocksource in modules argument. I think what your planning for sh clocksources seems very inelegant. I would imagine a better solution is out there. I'd prefer if you just leave sched_clock alone.

This is the first I've heard you mention locking concerns, and as usual there is not enough technical content (or any, really) to go on to even reply to this. Whether you consider my solution for sh clocksources elegant or not is irrelevant, as I wasn't soliciting feedback, and it's a problem that has to be dealt with regardless of whether it's a pretty one or not.

Quoted 7 lines · Paul Mundt

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Ah, ok. So unregister calls select_clocksource again? That does leave us a small window with jiffies, but I guess that's ok.

Quoted 3 lines

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

Right, that would want fixing indeed.

Quoted 3 lines

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

No, you only have to do sync_rcu() when stuff that could have referenced is going away and you cannot use call_rcu().

So when selecting a new clocksource, you don't need synchonization because stuff doesn't go away (I think :-)

Quoted 22 lines · Peter Zijlstra

Quoted 7 lines · Paul Mundt

We already do via select_clocksource(), if we are unregistering the current one then a new one with the flag set is selected. Before that, the override is likewise given preference, and we fall back on jiffies if there is nothing else. I suppose we could try and find the "best" one, but I think the override and manual clocksource selection should be fine for this.

Ah, ok. So unregister calls select_clocksource again? That does leave us a small window with jiffies, but I guess that's ok.

Quoted 3 lines

Now that you mention it though, the sched_clocksource() assignment within select_clocksource() happens underneath the clocksource_lock, but is not using rcu_assign_pointer().

Right, that would want fixing indeed.

Quoted 3 lines

If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do select_clocksource() will have to synchronize_rcu()?

Quoted 5 lines

No, you only have to do sync_rcu() when stuff that could have referenced is going away and you cannot use call_rcu().

So when selecting a new clocksource, you don't need synchonization because stuff doesn't go away (I think :-)

Hmm, no. In the unregister case stuff _IS_ going away. That's why you unregister in the first place, right ?

sched: sched_clock() clocksource handling.

There are presently a number of issues and limitations with how the clocksource and sched_clock() interaction works today. Configurations tend to be grouped in to one of the following:

- Platform provides a clocksource unsuitable for sched_clock() and prefers to use the generic jiffies-backed implementation.

- Platform provides its own clocksource and sched_clock() that wraps in to it.

- Platform uses a generic clocksource (ie, drivers/clocksource/) combined with the generic jiffies-backed sched_clock().

- Platform supports multiple sched_clock()-capable clocksources.

This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address these issues, which can be set for any sched_clock()-capable clocksource.

The generic sched_clock() implementation is likewise switched over to always read from a designated sched_clocksource, which is default initialized to the jiffies clocksource and updated based on the availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses the generic cyc2ns() logic on the clocksource ->read(), most of the platform-specific sched_clock() implementations can subsequently be killed off.

Quoted 20 lines · Paul Mundt

sched: sched_clock() clocksource handling.

There are presently a number of issues and limitations with how the clocksource and sched_clock() interaction works today. Configurations tend to be grouped in to one of the following:

- Platform provides a clocksource unsuitable for sched_clock() and prefers to use the generic jiffies-backed implementation.

- Platform provides its own clocksource and sched_clock() that wraps in to it.

- Platform uses a generic clocksource (ie, drivers/clocksource/) combined with the generic jiffies-backed sched_clock().

- Platform supports multiple sched_clock()-capable clocksources.

This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address these issues, which can be set for any sched_clock()-capable clocksource.

Quoted 7 lines

The generic sched_clock() implementation is likewise switched over to always read from a designated sched_clocksource, which is default initialized to the jiffies clocksource and updated based on the availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses the generic cyc2ns() logic on the clocksource ->read(), most of the platform-specific sched_clock() implementations can subsequently be killed off.

80ea3bac3a47bc73efa334d0dd57099d0ff14216 ("ARM: OMAP: sched_clock() corrected") seems to have switched omap from cyc2ns() to an open coded calculation based on mult_orig because cyc2ns() uses the NTP-adjusted mult instead. Does that apply here or is that change incorrect?

As there was no additional feedback on the most recent version of this patch, I'm resubmitting it for inclusion. As far as I know there are no more outstanding concerns.

--

sched: sched_clock() clocksource handling.

There are presently a number of issues and limitations with how the clocksource and sched_clock() interaction works today. Configurations tend to be grouped in to one of the following:

- Platform provides a clocksource unsuitable for sched_clock() and prefers to use the generic jiffies-backed implementation.

- Platform provides its own clocksource and sched_clock() that wraps in to it.

- Platform uses a generic clocksource (ie, drivers/clocksource/) combined with the generic jiffies-backed sched_clock().

- Platform supports multiple sched_clock()-capable clocksources.

This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address these issues, which can be set for any sched_clock()-capable clocksource.

The generic sched_clock() implementation is likewise switched over to always read from a designated sched_clocksource, which is default initialized to the jiffies clocksource and updated based on the availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses the generic cyc2ns() logic on the clocksource ->read(), most of the platform-specific sched_clock() implementations can subsequently be killed off.

Signed-off-by Paul Mundt <[email protected]>

2) How long does it have to be monotonic for?

Is it ok if it wraps every few seconds?

/*
* Scheduler clock - returns current time in nanosec units.

kernel/sched_clock.c +0 -2

@@ -38,8 +40,15 @@
  */
 unsigned long long __attribute__((weak)) sched_clock(void)
 {
-	return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-					* (NSEC_PER_SEC / HZ);

Quoted 8 lines

/*
* Scheduler clock - returns current time in nanosec units.

kernel/sched_clock.c +0 -2

@@ -38,8 +40,15 @@
  */
 unsigned long long __attribute__((weak)) sched_clock(void)
 {
-	return (unsigned long long)(jiffies - INITIAL_JIFFIES)
-					* (NSEC_PER_SEC / HZ);

kernel/sched_clock.c line 52

Quoted 10 lines

kernel/sched_clock.c +9 -0

+	unsigned long long time;
+	struct clocksource *clock;
+
+	rcu_read_lock();
+	clock = rcu_dereference(sched_clocksource);
+	time = cyc2ns(clock, clocksource_read(clock));
+	rcu_read_unlock();
+
+	return time;
 }

So in the above, cyc2ns could overflow prior to a u64 wrap.

cyc2ns does the following:

	(cycles * cs->mult) >> cs->shift;

The (cycles*cs->mult) bit may overflow for large cycle values, and its likely that could be fairly quickly, as ideally we have a large shift value to keep the precision high so mult will also be large.

I just went through some of the math here with Jon Hunter in this
thread: http://lkml.org/lkml/2009/5/15/466

None the less, either sched_clock will have to handle overflows or we'll need to do something like the timekeeping code where there's an periodic accumulation step that keeps the unaccumulated cycles small.

That said, the x86 sched_clock() uses cycles_2_ns() which is similar (but with a smaller scale value). So its likely it would also overflow prior to the u64 boundary as well.

Quoted 6 lines

kernel/time/jiffies.c +0 -0

index c3f6c30..727d881 100644
@@ -52,7 +52,7 @@
 

kernel/time/jiffies.c line 56

Quoted 5 lines

kernel/time/jiffies.c +1 -1

 static cycle_t jiffies_read(struct clocksource *cs)
 {
-	return (cycle_t) jiffies;
+	return (cycle_t) (jiffies - INITIAL_JIFFIES);
 }

Also, on 32bit systems this will may overflow ~monthly. However, this isn't different then the existing sched_clock() implementation, so either its been already handled and sched_clock is more robust then I thought or there's a bug there.