drivers/char/ip2/i2lib.c: replace direct assignment with set_current_state()

drivers/char/ip2/i2lib.c i2QueueCommands(int type, i2ChanStrPtr p · line 657

Quoted 5 lines · [email protected] wrote:

drivers/char/ip2/i2lib.c +0 -0

@@ -655,7 +655,7 @@ i2QueueCommands(int type, i2ChanStrPtr p
                      timeout--;   // So negative values == forever
 
              if (!in_interrupt()) {

I worry about what this driver is trying to do...

drivers/char/ip2/i2lib.c i2QueueCommands(int type, i2ChanStrPtr p · line 659

Quoted 3 lines

drivers/char/ip2/i2lib.c +1 -1

-                     current->state = TASK_INTERRUPTIBLE;
+                     set_current_state(TASK_INTERRUPTIBLE);
                      schedule_timeout(1);    // short nap

We do this all over the place. Adding new schedule_timeout_interruptible() and schedule_timeout_uninterruptible() would reduce kernel size and neaten things up.

Review i2QueueCommands(int type, i2ChanStrPtr p · line 657

Quoted 5 lines · [email protected] wrote:

(quoted diff) +0 -0

@@ -655,7 +655,7 @@ i2QueueCommands(int type, i2ChanStrPtr p
 			timeout--;   // So negative values == forever
 
 		if (!in_interrupt()) {

I worry about what this driver is trying to do...

Review i2QueueCommands(int type, i2ChanStrPtr p · line 659

Quoted 3 lines

(quoted diff) +1 -1

-			current->state = TASK_INTERRUPTIBLE;
+			set_current_state(TASK_INTERRUPTIBLE);
 			schedule_timeout(1);	// short nap 

We do this all over the place. Adding new schedule_timeout_interruptible() and schedule_timeout_uninterruptible() would reduce kernel size and neaten things up.

How does something like this look? If this looks ok, I'll send out bunches of patches to add users of the new interfaces.

Quoted 3 lines

How does something like this look? If this looks ok, I'll send out bunches of patches to add users of the new interfaces.

I'd drop the FASTCALL stuff... nowadays with regparm that's automatic and the cost of register-vs-stack isn't too big anyway

Also I'd rather not add the non-msec ones... either you're raw and use HZ, or you are "cooked" and use the msec variant.. I dont' see the point of adding an "in the middle" one. (Yes this means that several users need to be transformed to msecs but... I consider that progress ;)

kernel/timer.c +4 -0

+/*
+ * schedule_timeout_msecs - sleep until timeout
+ * @timeout_msecs: timeout value in milliseconds
+ *

Quoted 6 lines · Nishanth Aravamudan

(quoted diff) +4 -0

+/*
+ * schedule_timeout_msecs - sleep until timeout
+ * @timeout_msecs: timeout value in milliseconds
+ *

Review

Quoted 10 lines

(quoted diff) +10 -0

+ * A human-time (but otherwise identical) alternative to
+ * schedule_timeout() The state, therefore, *does* need to be set before
+ * calling this function, but this function should *never* be called
+ * directly. Use the nice wrappers, schedule_{interruptible,
+ * uninterruptible}_timeout_msecs().
+ *
+ * See the comment for schedule_timeout() for details.
+ */
+inline unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
+{

Making this inline will add more kernel text. Can't we uninline it?

I'm surprised that this function is not implemented as a call to the existing schedule_timeout()?

Review

Quoted 4 lines

(quoted diff) +4 -0

+	init_timer(&timer);
+	timer.expires = expire_jifs;
+	timer.data = (unsigned long) current;
+	timer.function = process_timeout;

hm, if we add the needed typecast to TIMER_INITIALIZER() the above could be

	timer = TIMER_INITIALIZER(process_timeout, expire_jifs,
				(unsigned long)current);

Also I'd rather not add the non-msec ones... either you're raw and use HZ, or you are "cooked" and use the msec variant.. I dont' see the point of adding an "in the middle" one. (Yes this means that several users need to be transformed to msecs but... I consider that progress ;)

Hi,

Quoted 4 lines · Arjan van de Ven

Also I'd rather not add the non-msec ones... either you're raw and use HZ, or you are "cooked" and use the msec variant.. I dont' see the point of adding an "in the middle" one. (Yes this means that several users need to be transformed to msecs but... I consider that progress ;)

What's wrong with using jiffies?

It's simple and the current timeout
system is based on it. Calling it something else doesn't suddenly give you more precision.

Quoted 1 line

What's wrong with using jiffies?

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

Hi,

Quoted 5 lines · Arjan van de Ven

Quoted 1 line

What's wrong with using jiffies?

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

We have helper functions for that.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

What's wrong with using jiffies?

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

We have helper functions for that.

I think we just disagree then... I consider this one a helper function as well, one with a simpler API that wraps the more complex and powerful api.

Hi,

Quoted 6 lines · Arjan van de Ven

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

What's wrong with using jiffies?

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

Quoted 5 lines

Quoted 1 line

We have helper functions for that.

I think we just disagree then... I consider this one a helper function as well, one with a simpler API that wraps the more complex and powerful api.

How is it more powerful?

jiffies/HZ is the more powerful API. The msec one which also sets current to (un)interruptible is the simplified wrapper on top.

Hi,

Quoted 2 lines · Arjan van de Ven

jiffies/HZ is the more powerful API. The msec one which also sets current to (un)interruptible is the simplified wrapper on top.

So why do you want to hide it?

Quoted 4 lines

Quoted 2 lines

jiffies/HZ is the more powerful API. The msec one which also sets current to (un)interruptible is the simplified wrapper on top.

So why do you want to hide it?

I don't want to hide it at all. I want to provide a simpler variant for it for the cases where a simpler variant is enough.

It really is a
helper to take some common task and get that closer to what the programmer wants (wallclock time delay)

Hi,

Quoted 2 lines · Arjan van de Ven

jiffies/HZ is the more powerful API. The msec one which also sets current to (un)interruptible is the simplified wrapper on top.

So why do you want to hide it? Make the jiffies based API the primary one and add some convenience functions, where we BTW could convert already constants at compile time.

Hi,

Quoted 8 lines · Arjan van de Ven

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

What's wrong with using jiffies?

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

We have helper functions for that.

Quoted 3 lines

I think we just disagree then... I consider this one a helper function as well, one with a simpler API that wraps the more complex and powerful api.

How is it more powerful? The next step in introducing such API is that people start complaining, they don't get ms precision, which of course is fixed by the next patch, with then results in that the whole timer system becomes more complex for a few misguided users.

Keep the thing as simple as possible and jiffies _are_ simple. Please show me the problem, that needs to be fixed.

Quoted 2 lines

Keep the thing as simple as possible and jiffies _are_ simple. Please show me the problem, that needs to be fixed.

I am not sure why jiffies are any simpler than milliseconds. In the millisecond case, conversion happens in common code and only needs to be audited once. In the jiffies case, I have to check *every* caller to see if they are doing stupid math :)

Hi,

Quoted 3 lines · Nishanth Aravamudan

Quoted 2 lines

Keep the thing as simple as possible and jiffies _are_ simple. Please show me the problem, that needs to be fixed.

Quoted 4 lines

I am not sure why jiffies are any simpler than milliseconds. In the millisecond case, conversion happens in common code and only needs to be audited once. In the jiffies case, I have to check *every* caller to see if they are doing stupid math :)

Jiffies are the basic time unit for kernel timers, hiding that fact gives users only wrong expectations about them.

I don't mind using helper functions, but constant conversion can already happen at compile time and for variable timeouts the user should seriously consider using jiffies directly instead of constantly converting time values.

Quoted 2 lines

Jiffies are the basic time unit for kernel timers, hiding that fact gives users only wrong expectations about them.

We already have msleep() and msleep_interruptible(), which hide jiffies in milliseconds. These interfaces are their parallel in the wait-queue case. If you don't want to use them, or their use is not appropriate, then the callers won't be changed.

My goal is to distinguish between these cases in sleeping-logic:

1) tick-oriented
use schedule_timeout(), add_timer(), etc.

2) time-oriented
use schedule_timeout_msecs()

Hi,

Quoted 7 lines · Nishanth Aravamudan

Quoted 2 lines

Jiffies are the basic time unit for kernel timers, hiding that fact gives users only wrong expectations about them.

We already have msleep() and msleep_interruptible(), which hide jiffies in milliseconds. These interfaces are their parallel in the wait-queue case. If you don't want to use them, or their use is not appropriate, then the callers won't be changed.

I'm not exactly impressed with their implementation, it's completely silly that there is no such convience function based on jiffies, e.g. if you look at the msleep_interruptible(), you'll find quite a few "msleep_interruptible(jiffies_to_msecs())".

msleep_interruptible() is especially bad as there are a few users who check the return value and since it adds one to the timeout, you can create loops which may never timeout (as e.g. in drivers/w1/w1_therm.c:
w1_therm_read_bin()), this is nice example of a bad interface.

These two function should actually look like this:

static inline void msleep(unsigned int msecs)

{
	sleep(msecs_to_jiffies(msecs));
}

static inline int msleep_interruptible(unsigned int msecs)

{   
	sleep_interruptible(msecs_to_jiffies(msecs)) != 0;
}

Quoted 5 lines

My goal is to distinguish between these cases in sleeping-logic:

1) tick-oriented
use schedule_timeout(), add_timer(), etc.

Quoted 2 lines

2) time-oriented
use schedule_timeout_msecs()

There is _no_ difference, the scheduler is based on ticks. Even if we soon have different time sources, the scheduler will continue to measure the time in ticks and for a simple reason - portability. Jiffies _are_ simple, don't throw that away.

Quoted 5 lines

Quoted 5 lines

My goal is to distinguish between these cases in sleeping-logic:

1) tick-oriented
use schedule_timeout(), add_timer(), etc.

Quoted 7 lines

Quoted 2 lines

2) time-oriented
use schedule_timeout_msecs()

There is _no_ difference, the scheduler is based on ticks. Even if we soon have different time sources, the scheduler will continue to measure the time in ticks and for a simple reason - portability. Jiffies _are_ simple, don't throw that away.

I agree that from an internal perspective there is no difference, but from an *interface* perspective they are hugely different, simply on the basis that one uses human-time units and one does not.

I guess we must continue to agree to disagree.

Hi,

Quoted 17 lines · Nishanth Aravamudan

Quoted 5 lines

Quoted 5 lines

My goal is to distinguish between these cases in sleeping-logic:

1) tick-oriented
use schedule_timeout(), add_timer(), etc.

Quoted 7 lines

Quoted 2 lines

2) time-oriented
use schedule_timeout_msecs()

There is _no_ difference, the scheduler is based on ticks. Even if we soon have different time sources, the scheduler will continue to measure the time in ticks and for a simple reason - portability. Jiffies _are_ simple, don't throw that away.

I agree that from an internal perspective there is no difference, but from an *interface* perspective they are hugely different, simply on the basis that one uses human-time units and one does not.

Quoted 1 line

I guess we must continue to agree to disagree.

I'm not really sure, what you disagree about.
1 HZ is about one second, which I don't think is such a difficult concept. I already said wrapper functions are fine and for anything smaller than HZ/2 it's probably a good idea nowadays.
My main point is to keep the core functionality in jiffies and provide some wrapper functions. What exactly do you disagree here on?

kernel/timer.c +12 -0

+unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
+{
+	unsigned long expire_jifs;
+
+	if (timeout_msecs == MAX_SCHEDULE_TIMEOUT_MSECS) {
+		expire_jifs = MAX_SCHEDULE_TIMEOUT;
+	} else {
+		/*
+		 * msecs_to_jiffies() is a unit conversion, which truncates
+		 * (rounds down), so we need to add 1.
+		 */
+		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;

Hi,

Quoted 21 lines · Nishanth Aravamudan

(quoted diff) +21 -0

+unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
+{
+	unsigned long expire_jifs;
+
+	if (timeout_msecs == MAX_SCHEDULE_TIMEOUT_MSECS) {
+		expire_jifs = MAX_SCHEDULE_TIMEOUT;
+	} else {
+		/*
+		 * msecs_to_jiffies() is a unit conversion, which truncates
+		 * (rounds down), so we need to add 1.
+		 */
+		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;
+	}
+
+	expire_jifs = schedule_timeout(expire_jifs);
+
+	/*
+	 * don't need to add 1 here, even though there is truncation,
+	 * because we will add 1 if/when the value is sent back in
+	 */
+	return jiffies_to_msecs(expire_jifs);

Quoted 1 line

+}

As I already mentioned for msleep_interruptible this is a really terrible interface.
The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the process is immediately woken up again) makes the caller suspectible to timeout manipulations and requires constant reauditing, that no caller gets it wrong, so it's better to avoid this error source completely.

Constant conversion between different time units is a really bad idea. If the user needs the remaining time, he is really better off to do it himself by checking jiffies and only does an initial conversion from relative to absolute (kernel) time.
This wrapper function really should be an inline function and should look more like this:

static inline int schedule_timeout_msecs(unsigned int timeout_msecs)

{
	return schedule_timeout(msecs_to_jiffies(timeout_msecs) + 1) != 0;
}

Quoted 25 lines · Roman Zippel

Hi,

Quoted 21 lines · Nishanth Aravamudan

(quoted diff) +21 -0

+unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
+{
+	unsigned long expire_jifs;
+
+	if (timeout_msecs == MAX_SCHEDULE_TIMEOUT_MSECS) {
+		expire_jifs = MAX_SCHEDULE_TIMEOUT;
+	} else {
+		/*
+		 * msecs_to_jiffies() is a unit conversion, which truncates
+		 * (rounds down), so we need to add 1.
+		 */
+		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;
+	}
+
+	expire_jifs = schedule_timeout(expire_jifs);
+
+	/*
+	 * don't need to add 1 here, even though there is truncation,
+	 * because we will add 1 if/when the value is sent back in
+	 */
+	return jiffies_to_msecs(expire_jifs);

Quoted 8 lines

Quoted 1 line

+}

As I already mentioned for msleep_interruptible this is a really terrible interface.
The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the process is immediately woken up again) makes the caller suspectible to timeout manipulations and requires constant reauditing, that no caller gets it wrong, so it's better to avoid this error source completely.

After some thought today, I realized the +1 case is not specific to milliseconds. It's just that it's only being done *correctly* in the milliseconds case...I think ;)

Description: Ensure that schedule_timeout() requests can not possibly expire early in the timeout case, by adding the requested relative jiffy value to the next value of jiffies. Currently, by adding to the current value of jiffies, we might actually expire a jiffy too early (in a corner case).

Quoted 36 lines · Nishanth Aravamudan

Quoted 22 lines · Roman Zippel

Hi,

Quoted 17 lines · Nishanth Aravamudan

(quoted diff) +17 -0

+unsigned int __sched schedule_timeout_msecs(unsigned int timeout_msecs)
+{
+	unsigned long expire_jifs;
+
+	if (timeout_msecs == MAX_SCHEDULE_TIMEOUT_MSECS) {
+		expire_jifs = MAX_SCHEDULE_TIMEOUT;
+	} else {
+		/*
+		 * msecs_to_jiffies() is a unit conversion, which truncates
+		 * (rounds down), so we need to add 1.
+		 */
+		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;
+	}
+
+	expire_jifs = schedule_timeout(expire_jifs);
+
+	/*

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	 * don't need to add 1 here, even though there is truncation,
+	 * because we will add 1 if/when the value is sent back in
+	 */
+	return jiffies_to_msecs(expire_jifs);
+}

As I already mentioned for msleep_interruptible this is a really terrible interface.
The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the process is immediately woken up again) makes the caller suspectible to timeout manipulations and requires constant reauditing, that no caller gets it wrong, so it's better to avoid this error source completely.

After some thought today, I realized the +1 case is not specific to milliseconds. It's just that it's only being done *correctly* in the milliseconds case...I think ;)

<snip>

Quoted 5 lines

Description: Ensure that schedule_timeout() requests can not possibly expire early in the timeout case, by adding the requested relative jiffy value to the next value of jiffies. Currently, by adding to the current value of jiffies, we might actually expire a jiffy too early (in a corner case).

Sorry, I forgot that sys_nanosleep() also always adds 1 to the request (to account for this same issue, I believe, as POSIX demands no early return from nanosleep() calls). There are some other locations where similar

+ (t.tv_sec || t.tv_nsec)

rounding-ups occur. I'll fix those separately if this patch goes in. I change the one in sys_nanosleep() below to maintain the same latency as we currently have. I also screwed up my layout in the previous submission, sorry about that.

Description: Ensure that schedule_timeout() requests can not possibly expire early in the timeout case, by adding the requested relative jiffy value to the next value of jiffies. Currently, by adding to the current value of jiffies, we might actually expire a jiffy too early (in a corner case). Modify the callers of schedule_timeout() in timer.c to not add 1 themselves.

Signed-off-by Nishanth Aravamudan <[email protected]>

---

 timer.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

kernel/timer.c +7 -7

@@ -1134,7 +1134,7 @@ fastcall signed long __sched schedule_ti
 		}
 	}
 
-	expire = timeout + jiffies;
+	expire = timeout + jiffies + 1;
 
 	init_timer(&timer);
 	timer.expires = expire;

@@ -1190,9 +1190,10 @@ unsigned int __sched schedule_timeout_ms
 	} else {
 		/*
 		 * msecs_to_jiffies() is a unit conversion, which truncates
-		 * (rounds down), so we need to add 1.
+		 * (rounds down), so we need to add 1, but this is taken
+		 * care of by schedule_timeout() now.
 		 */
-		expire_jifs = msecs_to_jiffies(timeout_msecs) + 1;
+		expire_jifs = msecs_to_jiffies(timeout_msecs);
 	}
 
 	expire_jifs = schedule_timeout(expire_jifs);

@@ -1286,7 +1287,7 @@ asmlinkage long sys_nanosleep(struct tim
 	if ((t.tv_nsec >= 1000000000L) || (t.tv_nsec < 0) || (t.tv_sec < 0))
 		return -EINVAL;
 
-	expire = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec);
+	expire = timespec_to_jiffies(&t);
 	current->state = TASK_INTERRUPTIBLE;
 	expire = schedule_timeout(expire);
 

@@ -1675,7 +1676,7 @@ unregister_time_interpolator(struct time
  */
 void msleep(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies(msecs);
 
 	while (timeout) {
 		set_current_state(TASK_UNINTERRUPTIBLE);

@@ -1691,7 +1692,7 @@ EXPORT_SYMBOL(msleep);
  */
 unsigned long msleep_interruptible(unsigned int msecs)
 {
-	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
+	unsigned long timeout = msecs_to_jiffies(msecs);
 
 	while (timeout && !signal_pending(current)) {
 		set_current_state(TASK_INTERRUPTIBLE);
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Uh... PLEASE tell me you are NOT changing timespec_to_jiffies() (and timeval_to_jiffies() to add 1. This is NOT the right thing to do. For repeating times (see setitimer code) we need the actual time as we KNOW where the jiffies edge is in the repeating case. The +1 is needed ONLY for the initial time, not the repeating time.

Uh... PLEASE tell me you are NOT changing timespec_to_jiffies() (and timeval_to_jiffies() to add 1. This is NOT the right thing to do. For repeating times (see setitimer code) we need the actual time as we KNOW where the jiffies edge is in the repeating case. The +1 is needed ONLY for the initial time, not the repeating time.

See:
http://marc.theaimsgroup.com/?l=linux-kernel&m=112208357906156&w=2

Quoted 3 lines · George Anzinger

Uh... PLEASE tell me you are NOT changing timespec_to_jiffies() (and timeval_to_jiffies() to add 1. This is NOT the right thing to do. For repeating times (see setitimer code) we need the actual time as we KNOW where the jiffies edge is in the repeating case. The +1 is needed ONLY for the initial time, not the repeating time.

Quoted 2 lines

See:
http://marc.theaimsgroup.com/?l=linux-kernel&m=112208357906156&w=2

I followed that thread, George, but I think it's a different case with schedule_timeout() [maybe this indicates drivers/other users should maybe be using itimers, but I'll get to that in a sec].

With schedule_timeout(), we are just given a relative jiffies value. We have no context as to which task is requesting the delay, per se, meaning we don't (can't) know from the interface whether this is the first delay in a sequence, or a brand new one, without changing all users to have some sort of control structure. The callers of schedule_timeout() don't even get a pointer to the timer added internally.

So, adding 1 to all sleeps seems like it might be reasonable, as looping sleeps probably need to use a different interface. I had worked a bit ago on something like poll_event() with the kernel-janitors group, which would abstract out the repeated sleeps. Basically wait_event() without wait-queues... Maybe we could make such an interface just use itimers? I've attached my old patch for poll_event(), just for reference.

My point, I guess, is that in the schedule_timeout() case, we don't know where the jiffies edge is, as we either expire or receive a wait-queue event/signal, we never mod_timer() the internal timer... So we have to assume that we need to sleep the request. But maybe Roman's idea of sleeping a certain number of jiffy edges is sufficient. I am not yet convinced driver authors want/need such an interface, though, still thinking it over.

Quoted 13 lines · Nishanth Aravamudan

Quoted 9 lines · George Anzinger

Uh... PLEASE tell me you are NOT changing timespec_to_jiffies() (and timeval_to_jiffies() to add 1. This is NOT the right thing to do. For repeating times (see setitimer code) we need the actual time as we KNOW where the jiffies edge is in the repeating case. The +1 is needed ONLY for the initial time, not the repeating time.

See:
http://marc.theaimsgroup.com/?l=linux-kernel&m=112208357906156&w=2

Quoted 3 lines

I followed that thread, George, but I think it's a different case with schedule_timeout() [maybe this indicates drivers/other users should maybe be using itimers, but I'll get to that in a sec].

I think I miss understood back then :).

Quoted 9 lines

With schedule_timeout(), we are just given a relative jiffies value. We have no context as to which task is requesting the delay, per se, meaning we don't (can't) know from the interface whether this is the first delay in a sequence, or a brand new one, without changing all users to have some sort of control structure. The callers of schedule_timeout() don't even get a pointer to the timer added internally.

Quoted 6 lines

So, adding 1 to all sleeps seems like it might be reasonable, as looping sleeps probably need to use a different interface. I had worked a bit ago on something like poll_event() with the kernel-janitors group, which would abstract out the repeated sleeps. Basically wait_event() without wait-queues... Maybe we could make such an interface just use itimers? I've attached my old patch for poll_event(), just for reference.

I think not. itimers is really pointed at a particular system call and has resources in the task structure to do it. These would be hard to share...

Quoted 7 lines

My point, I guess, is that in the schedule_timeout() case, we don't know where the jiffies edge is, as we either expire or receive a wait-queue event/signal, we never mod_timer() the internal timer... So we have to assume that we need to sleep the request. But maybe Roman's idea of sleeping a certain number of jiffy edges is sufficient. I am not yet convinced driver authors want/need such an interface, though, still thinking it over.

IMNSHO we should not get too parental with kernel only interfaces. Adding 1 is easy enough for the caller and even easier to explain in the instructions (i.e. this call sleeps for X jiffies edges). This allows the caller to do more if needed and, should he ever just want to sync to the next jiffie he does not have to deal with backing out that +1.

Quoted 1 line

IMNSHO we should not get too parental with kernel only interfaces. Adding 1 is easy enough for the caller and even easier to explain in the instructions (i.e. this call sleeps for X jiffies edges). This allows the caller to do more if needed and, should he ever just want to sync to the next jiffie he does not have to deal with backing out that +1.

I don't want to be too parental either, but I also am trying to avoid code duplication. Lots of drivers basically do something like poll_event() does (or could do with some changes), i.e. looping a constant amount multiple times, checking something every so often. The patch was just a thought, though. I will keep evaluating drivers and see if it's a useful interface to have eventually.

I guess I'm just concerned with making an unintuitive interface. As was brought up at OLS, drivers are a major source of bugs/buggy code. The simpler, more useful we can make interfaces, the better, I think. I'm not claiming you disagree, I just want to make my own motives clear. While fixing up the schedule_timeout() comment would make it clear what schedule_timeout() achieves, I'm not sure how useful such an interface is, if every caller adds 1 :) I need to mull it over, though... Lots to consider. I also, of course, want to stay flexible for the reasons you mention (letting the driver adjust the timeout as they expect to).

Nishanth Aravamudan wrote:
~

Quoted 14 lines

Quoted 5 lines

IMNSHO we should not get too parental with kernel only interfaces. Adding 1 is easy enough for the caller and even easier to explain in the instructions (i.e. this call sleeps for X jiffies edges). This allows the caller to do more if needed and, should he ever just want to sync to the next jiffie he does not have to deal with backing out that +1.

I don't want to be too parental either, but I also am trying to avoid code duplication. Lots of drivers basically do something like poll_event() does (or could do with some changes), i.e. looping a constant amount multiple times, checking something every so often. The patch was just a thought, though. I will keep evaluating drivers and see if it's a useful interface to have eventually.

Quoted 9 lines

I guess I'm just concerned with making an unintuitive interface. As was brought up at OLS, drivers are a major source of bugs/buggy code. The simpler, more useful we can make interfaces, the better, I think. I'm not claiming you disagree, I just want to make my own motives clear. While fixing up the schedule_timeout() comment would make it clear what schedule_timeout() achieves, I'm not sure how useful such an interface is, if every caller adds 1 :) I need to mull it over, though... Lots to consider. I also, of course, want to stay flexible for the reasons you mention (letting the driver adjust the timeout as they expect to).

I would leave the +1 alone and put in the correct documentation. This way _more_ folks will be made aware of the mid jiffie issue. Far to often we see (and let get in) patches that mess up user interfaces around this issue. The recent changes to itimer come to mind...

Sorry, I forgot that sys_nanosleep() also always adds 1 to the request (to account for this same issue, I believe, as POSIX demands no early return from nanosleep() calls). There are some other locations where similar

+ (t.tv_sec || t.tv_nsec)

Nishanth Aravamudan wrote:
~

Quoted 5 lines

Sorry, I forgot that sys_nanosleep() also always adds 1 to the request (to account for this same issue, I believe, as POSIX demands no early return from nanosleep() calls). There are some other locations where similar

Quoted 1 line

+ (t.tv_sec || t.tv_nsec)

This is not the same as "always add 1". We don't do it this way just to have fun with C. If you change schedule_timeout() to add the 1, nanosleep() will need to do things differently to get the same behavior.
(And, YES users do pass in zero sleep times.)

Quoted 4 lines

The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the process is immediately woken up again) makes the caller suspectible to timeout manipulations and requires constant reauditing, that no caller gets it wrong, so it's better to avoid this error source completely.

Hi,

Andrew, please drop this patch.
Nish, please stop fucking around with kernel APIs.

Quoted 5 lines

Quoted 5 lines

The "jiffies_to_msecs(msecs_to_jiffies(timeout_msecs) + 1)" case (when the process is immediately woken up again) makes the caller suspectible to timeout manipulations and requires constant reauditing, that no caller gets it wrong, so it's better to avoid this error source completely.

Nish, did you read this? Is my English this bad?

Quoted 10 lines

kernel/timer.c +1 -1

@@ -1134,7 +1134,7 @@ fastcall signed long __sched schedule_ti
 		}
 	}
 
-	expire = timeout + jiffies;
+	expire = timeout + jiffies + 1;
 

kernel/timer.c fastcall signed long __sched schedule_ti · line 1140

Quoted 2 lines

kernel/timer.c +0 -0

 	init_timer(&timer);
 	timer.expires = expire;

And a little later it does:

	timeout = expire - jiffies;

which means callers can get back a larger timeout.

Nish, did you check and fix _all_ users? I can easily find a number of users which immediately use the return value as next timeout.

There are _a_lot_ of schedule_timeout(1) for small busy loops, these are asking for "please schedule until next tick". Did you check that these are still ok?

schedule_timeout() is arguably a broken API, but can we please _first_ come up with a plan to fix this, before we break even more?

The comment for schedule_timeout() claims:

* Make the current task sleep until @timeout jiffies have
* elapsed.

Currently, it does not do so. I was simply trying to make the function do what it claims it does.

My point was that the +1 issues (potential
infinite timeouts) are a problem with *jiffies* not milliseconds. And thus need to be pushed down to the jiffies layer. I think my explanation was pretty clear.

Hi,

Quoted 5 lines · Nishanth Aravamudan

The comment for schedule_timeout() claims:

* Make the current task sleep until @timeout jiffies have
* elapsed.

Quoted 2 lines

Currently, it does not do so. I was simply trying to make the function do what it claims it does.

What makes you think the comment is correct? This comment was added at 2.4.3, while schedule_timeout() has this behaviour since it was added at 2.1.127.

Quoted 4 lines

My point was that the +1 issues (potential
infinite timeouts) are a problem with *jiffies* not milliseconds. And thus need to be pushed down to the jiffies layer. I think my explanation was pretty clear.

Not really, could you go into more details why it's "a problem with *jiffies* not milliseconds"?

Quoted 3 lines

What makes you think the comment is correct? This comment was added at 2.4.3, while schedule_timeout() has this behaviour since it was added at 2.1.127.

Fair enough. Should we change the comment?

If a user requests schedule_timeout(HZ/100); which, if HZ is 1000, is 10 jiffies, yes? But, since we are between ticks, we want to actually add that request to the next interval, not to the current one. Otherwise, we do have the possibility of returning early. Currently, we require callers to add the +1 to their request, and thus they only add it to the first one. The problem with my patch which pushed it to schedule_timeout(), is that we will do +1 to every request. I'm not sure, without some sort of "persistent" timeout control structure, how we get around that, though, in the schedule_timeout() case. Does that make any more sense?

Andrew, please drop this patch.

Quoted 2 lines · Roman Zippel

Andrew, please drop this patch.

Well I was sitting on them with the intention of taking a look later and trying to work out what the heck you two have been going on about.

But maybe dropping them means that we'll later get a clear and concise description of the problem and its solution, so I'll do that.

(This was supposed to be just a simple "consolidate set_current_state+schedule_timeout into a single function call" patch. What happened?)

Add schedule_timeout_{,un}intr() interfaces

Hi,

Quoted 2 lines · Arjan van de Ven

Quoted 1 line

What's wrong with using jiffies?

Quoted 3 lines

A lot of the (driver) users want a wallclock based timeout. For that, miliseconds is a more obvious API with less chance to get the jiffies/HZ conversion wrong by the driver writer.

We have helper functions for that. The point about using jiffies is to make it _very_ clear, that the timeout is imprecise and for most users this is sufficient.

	set_current_state(TASK_{,UN}INTERRUPTIBLE);
	schedule_timeout(msecs_to_jiffies(some_constant_msecs));

just have an interface that allows

	schedule_timeout_msecs_{,un}interruptible(some_constant_msecs);

and push the jiffies conversion to common code?

Hi,

Quoted 5 lines · Nishanth Aravamudan

	set_current_state(TASK_{,UN}INTERRUPTIBLE);
	schedule_timeout(msecs_to_jiffies(some_constant_msecs));

just have an interface that allows

Quoted 3 lines

	schedule_timeout_msecs_{,un}interruptible(some_constant_msecs);

and push the jiffies conversion to common code?

What's wrong with just:

	schedule_timeout_{,un}interruptible(msecs_to_jiffies(some_constant_msecs));

The majority of users use a constant, which can already be converted at compile tile.
Additionally such an interface also had to return a ms value and instead of that constant conversion, the user is better off to work with jiffies directly.

Quoted 3 lines

What's wrong with just:

	schedule_timeout_{,un}interruptible(msecs_to_jiffies(some_constant_msecs));

Nothing, I suppose. I just prefer directly using msecs. I understand your point more now, I think. You are worried about those people that actually use the return value of schedule_timeout().

Quoted 2 lines

The majority of users use a constant, which can already be converted at compile tile.

2) Sleep in a loop, keeping track of remaining timeout each iteration:

	while (timeout) {
		do_some_stuff();
		timeout = schedule_timeout(timeout);
		if (some_condition)
			break;
	}

Hi,

On Fri, 22 Jul 2005, Arjan van de Ven wrote:

Quoted 4 lines

Also I'd rather not add the non-msec ones... either you're raw and use HZ, or you are "cooked" and use the msec variant.. I dont' see the point of adding an "in the middle" one. (Yes this means that several users need to be transformed to msecs but... I consider that progress ;)

What's wrong with using jiffies? It's simple and the current timeout system is based on it. Calling it something else doesn't suddenly give you more precision.