+ futex-restartable-futex_wait.patch added to -mm tree

PATCH In review [email protected] 6 participants 22 messages Mar 11, 2007 – Mar 12, 2007 1 message not in the archive Futex

lore

Share & tools

lore

[email protected]

author tree notice

2007-03-11 08:15 UTC #0 lore raw

The patch titled
futex: restartable futex_wait
has been added to the -mm tree. Its filename is
futex-restartable-futex_wait.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find out what to do about this

------------------------------------------------------
Subject: futex: restartable futex_wait
From: Nick Piggin <[email protected]>

LTP test sigaction_16_24 fails, because it expects sem_wait to be restarted if SA_RESTART is set. sem_wait is implemented with futex_wait, that currently doesn't support being restarted. Ulrich confirms that the call should be restartable.

Implement a restart_block method to handle the relative timeout, and allow restarts.

Signed-off-by Nick Piggin <[email protected]>

Cc Ulrich Drepper <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Roland McGrath <[email protected]>

Cc Oleg Nesterov <[email protected]>

Acked-by Ingo Molnar <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 kernel/futex.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

kernel/futex.c renamed from kernel/futex.c~futex-restartable-futex_wait +24 -1

diff -puN kernel/futex.c~futex-restartable-futex_wait kernel/futex.c
@@ -978,6 +978,7 @@ static void unqueue_me_pi(struct futex_q
 	drop_futex_key_refs(&q->key);
 }
 
+static long futex_wait_restart(struct restart_block *restart);
 static int futex_wait(u32 __user *uaddr, u32 val, unsigned long time)
 {
 	struct task_struct *curr = current;

@@ -1077,11 +1078,22 @@ static int futex_wait(u32 __user *uaddr,
 		return 0;
 	if (time == 0)
 		return -ETIMEDOUT;
+
 	/*
 	 * We expect signal_pending(current), but another thread may
 	 * have handled it for us already.
 	 */
-	return -EINTR;
+	if (time == MAX_SCHEDULE_TIMEOUT)
+		return -ERESTARTSYS;
+	else {
+		struct restart_block *restart;
+		restart = &current_thread_info()->restart_block;
+		restart->fn = futex_wait_restart;
+		restart->arg0 = (unsigned long)uaddr;
+		restart->arg1 = (unsigned long)val;
+		restart->arg2 = time;
+		return -ERESTART_RESTARTBLOCK;
+	}
 
  out_unlock_release_sem:
 	queue_unlock(&q, hb);

@@ -1091,6 +1103,17 @@ static int futex_wait(u32 __user *uaddr,
 	return ret;
 }
 
+static long futex_wait_restart(struct restart_block *restart)
+{
+	u32 __user *uaddr = (u32 __user *)restart->arg0;
+	u32 val = (u32)restart->arg1;
+	unsigned long time = restart->arg2;
+
+	restart->fn = do_no_restart_syscall;
+	return (long)futex_wait(uaddr, val, time);
+}
+
+
 /*
  * Userspace tried a 0 -> TID atomic transition of the futex value
  * and failed. The kernel side here does the whole locking operation:

Patches currently in -mm which might be from [email protected] are

mm-fix-madvise-infinine-loop.patch futex-restartable-futex_wait.patch mm-remove-gcc-workaround.patch mm-more-rmap-checking.patch mm-make-read_cache_page-synchronous.patch fs-buffer-dont-pageuptodate-without-page-locked.patch mm-debug-check-for-the-fault-vs-invalidate-race.patch mm-debug-check-for-the-fault-vs-invalidate-race-tidy.patch mm-simplify-filemap_nopage.patch mm-fix-fault-vs-invalidate-race-for-linear-mappings.patch mm-merge-populate-and-nopage-into-fault-fixes-nonlinear.patch mm-merge-populate-and-nopage-into-fault-fixes-nonlinear-tidy.patch mm-merge-nopfn-into-fault.patch mm-remove-legacy-cruft.patch mm-fix-clear_page_dirty_for_io-vs-fault-race.patch exec-fix-remove_arg_zero.patch exec-fix-remove_arg_zero-add-comment.patch

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ingo Molnar

2007-03-12 09:10 UTC #2 lore raw

PATCH 1/1 change futex_wait() to hrtimers

Quoted 5 lines · Roland McGrath

I agree it should restart. But I don't think this is quite right in the timeout case. It will increase the total maximum real time spent arbitrarily by the amount of time elapsed in signal handlers. Other restartable, timed calls have to convert to an absolute timeout for the restart block (and convert back when doing the restart).

i dont think we should try to do this. We should not and cannot do anything about all of the artifacts that comes with the use of relative timeouts and schedule_timeout().

basically, using jiffies here (which schedule_timeout() does) is /fundamentally/ imprecise. If you get many interrupts, rounding errors sum up - and there's nothing we can do about it!

the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.

so i'd suggest for Nick to add restart processing ontop of this patch:
hrtimers use absolute timeouts and hence there are neither rounding nor signal processing delay artifacts.

Ingo

-------------->
From: S�bastien Dugu� <[email protected]>
Subject: [patch] change futex_wait() to hrtimers

This patch modifies futex_wait() to use an hrtimer + schedule() in place of schedule_timeout().

schedule_timeout() is tick based, therefore the timeout granularity is the tick (1 ms, 4 ms or 10 ms depending on HZ). By using a high resolution timer for timeout wakeup, we can attain a much finer timeout granularity (in the microsecond range). This parallels what is already done for futex_lock_pi().

The timeout passed to the syscall is no longer converted to jiffies and is therefore passed to do_futex() and futex_wait() as a timespec therefore keeping nanosecond resolution.

Also this removes the need to pass the nanoseconds timeout part to futex_lock_pi() in val2.

In futex_wait(), if the timeout is zero then a regular schedule() is performed. Otherwise, an hrtimer is fired before schedule() is called.

Signed-off-by S�bastien Dugu� <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

 include/linux/futex.h | 2 -
 kernel/futex.c        |   61 +++++++++++++++++++++++++++++++++-----------------
 kernel/futex_compat.c |   11 +--------
 3 files changed, 44 insertions(+), 30 deletions(-)

include/linux/futex.h +1 -1

Index: linux/include/linux/futex.h
===================================================================
@@ -94,7 +94,7 @@ struct robust_list_head {
 #define ROBUST_LIST_LIMIT	2048
 
 #ifdef __KERNEL__
-long do_futex(u32 __user *uaddr, int op, u32 val, unsigned long timeout,
+long do_futex(u32 __user *uaddr, int op, u32 val, struct timespec *timeout,
 	      u32 __user *uaddr2, u32 val2, u32 val3);
 
 extern int

kernel/futex.c +41 ~~-20~~

141 lines not on this page · raw message

kernel/futex_compat.c +2 -9

Index: linux/kernel/futex_compat.c
===================================================================
@@ -141,8 +141,7 @@ asmlinkage long compat_sys_futex(u32 __u
 		struct compat_timespec __user *utime, u32 __user *uaddr2,
 		u32 val3)
 {
-	struct timespec t;
-	unsigned long timeout = MAX_SCHEDULE_TIMEOUT;
+	struct timespec t = {.tv_sec = 0, .tv_nsec = 0};
 	int val2 = 0;
 
 	if (utime && (op == FUTEX_WAIT || op == FUTEX_LOCK_PI)) {

@@ -150,15 +149,9 @@ asmlinkage long compat_sys_futex(u32 __u
 			return -EFAULT;
 		if (!timespec_valid(&t))
 			return -EINVAL;
-		if (op == FUTEX_WAIT)
-			timeout = timespec_to_jiffies(&t) + 1;
-		else {
-			timeout = t.tv_sec;
-			val2 = t.tv_nsec;
-		}
 	}
 	if (op == FUTEX_REQUEUE || op == FUTEX_CMP_REQUEUE)
 		val2 = (int) (unsigned long) utime;
 
-	return do_futex(uaddr, op, val, timeout, uaddr2, val2, val3);
+	return do_futex(uaddr, op, val, &t, uaddr2, val2, val3);
 }

Nick Piggin

2007-03-12 09:16 UTC #3 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 9 lines · Ingo Molnar

Quoted 5 lines · Roland McGrath
I agree it should restart. But I don't think this is quite right in the timeout case. It will increase the total maximum real time spent arbitrarily by the amount of time elapsed in signal handlers. Other restartable, timed calls have to convert to an absolute timeout for the restart block (and convert back when doing the restart).

Quoted 7 lines

i dont think we should try to do this. We should not and cannot do anything about all of the artifacts that comes with the use of relative timeouts and schedule_timeout().
basically, using jiffies here (which schedule_timeout() does) is /fundamentally/ imprecise. If you get many interrupts, rounding errors sum up - and there's nothing we can do about it!

Well I did convert futex_wait to an absolute timeout based version in the subsequent incremental patch. I think that is OK?

Ingo Molnar

2007-03-12 11:03 UTC #4 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 10 lines · Nick Piggin

Quoted 7 lines
i dont think we should try to do this. We should not and cannot do anything about all of the artifacts that comes with the use of relative timeouts and schedule_timeout().
basically, using jiffies here (which schedule_timeout() does) is /fundamentally/ imprecise. If you get many interrupts, rounding errors sum up - and there's nothing we can do about it!
Well I did convert futex_wait to an absolute timeout based version in the subsequent incremental patch. I think that is OK?

it still has the rounding artifacts: using timer_list there is no way to do a precise long sleep based on many small sleeps.

even if this means more work for you (i'm sorry about that!) i'm quite sure we should take Sebastien's hrtimers based implementation of futex_wait(), and use the nanosleep method to restart it. There's no point in further tweaking the imprecise approach: whenever some timeout needs to be restarted, it's a candidate for hrtimers.

until then, glibc already handles timeouts and restarts it manually.

Ingo

Thomas Gleixner

2007-03-12 11:13 UTC #5 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 6 lines · Ingo Molnar

Quoted 2 lines
Well I did convert futex_wait to an absolute timeout based version in the subsequent incremental patch. I think that is OK?
it still has the rounding artifacts: using timer_list there is no way to do a precise long sleep based on many small sleeps.

Quoted 7 lines

even if this means more work for you (i'm sorry about that!) i'm quite sure we should take Sebastien's hrtimers based implementation of futex_wait(), and use the nanosleep method to restart it. There's no point in further tweaking the imprecise approach: whenever some timeout needs to be restarted, it's a candidate for hrtimers.
until then, glibc already handles timeouts and restarts it manually.

This also allows us to add a seperate absolute time bases futex op, which allows to remove the conversion of abstime to reltime in glibc.

tglx

Nick Piggin

2007-03-12 11:13 UTC #6 lore raw

Re: [patch] change futex_wait() to hrtimers

In reply to Ingo Molnar

Quoted 11 lines · Ingo Molnar

Quoted 8 lines · Nick Piggin
Quoted 7 lines
i dont think we should try to do this. We should not and cannot do anything about all of the artifacts that comes with the use of relative timeouts and schedule_timeout().
basically, using jiffies here (which schedule_timeout() does) is /fundamentally/ imprecise. If you get many interrupts, rounding errors sum up - and there's nothing we can do about it!

Quoted 5 lines

Quoted 2 lines
Well I did convert futex_wait to an absolute timeout based version in the subsequent incremental patch. I think that is OK?
it still has the rounding artifacts: using timer_list there is no way to do a precise long sleep based on many small sleeps.

OK but that is nothing to do with my patch, but the original futex_wait implementation.

Quoted 5 lines

even if this means more work for you (i'm sorry about that!) i'm quite sure we should take Sebastien's hrtimers based implementation of futex_wait(), and use the nanosleep method to restart it. There's no point in further tweaking the imprecise approach: whenever some timeout needs to be restarted, it's a candidate for hrtimers.

Absolute timeout is needed, sure. But once that is done, hrtimers does not fix a bug, does it?

Quoted 2 lines

until then, glibc already handles timeouts and restarts it manually.

It isn't timeout handling that is buggy, but EINTR behaviour. And glibc does not handle that here.

Ingo Molnar

2007-03-12 11:20 UTC #7 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 8 lines · Nick Piggin

Quoted 5 lines
even if this means more work for you (i'm sorry about that!) i'm quite sure we should take Sebastien's hrtimers based implementation of futex_wait(), and use the nanosleep method to restart it. There's no point in further tweaking the imprecise approach: whenever some timeout needs to be restarted, it's a candidate for hrtimers.
Absolute timeout is needed, sure. But once that is done, hrtimers does not fix a bug, does it?

the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So instead of trying to fix the bug the wrong way, please try to fix it the right way, ontop of an already existing and tested patch, ok? That also enables the other neat stuff Thomas talked about.

Quoted 4 lines

Quoted 1 line
until then, glibc already handles timeouts and restarts it manually.
It isn't timeout handling that is buggy, but EINTR behaviour. And glibc does not handle that here.

hm. I'm wondering how this wasnt noticed sooner - this futex_wait behavior has been there for like forever.

Ingo

Nick Piggin

2007-03-12 11:29 UTC #8 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 9 lines · Ingo Molnar

Quoted 6 lines · Nick Piggin
Quoted 5 lines
even if this means more work for you (i'm sorry about that!) i'm quite sure we should take Sebastien's hrtimers based implementation of futex_wait(), and use the nanosleep method to restart it. There's no point in further tweaking the imprecise approach: whenever some timeout needs to be restarted, it's a candidate for hrtimers.

Quoted 5 lines

Quoted 2 lines
Absolute timeout is needed, sure. But once that is done, hrtimers does not fix a bug, does it?
the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So

But using schedule_timeout is not a bug. Userspace timeouts are always defined to be "at least".

Quoted 3 lines

instead of trying to fix the bug the wrong way, please try to fix it the right way, ontop of an already existing and tested patch, ok? That also enables the other neat stuff Thomas talked about.

Well that's nice, but I have a bugfix here which probably needs to get backported to stable kernels and distro kernels.

It should be just as easy to rebase the hrtimer patch on top of my fix. Considering that you've had it for a year, I don't think it needs to be added right before my fix.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
until then, glibc already handles timeouts and restarts it manually.
It isn't timeout handling that is buggy, but EINTR behaviour. And glibc does not handle that here.
hm. I'm wondering how this wasnt noticed sooner - this futex_wait behavior has been there for like forever.

People ignore LTP test failures, and programs probably try to avoid exercising the nuances of the unix signal API, I guess.

Ingo Molnar

2007-03-12 11:38 UTC #9 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 5 lines · Nick Piggin

Quoted 2 lines
the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So
But using schedule_timeout is not a bug. Userspace timeouts are always defined to be "at least".

but what you are adding isnt a plain schedule_timeout(), it is a restart block handling loop. And for those restart blocks that relate to timeouts, we only use hrtimers. I am not making this up to annoy you:
take a look at all the current restart block handlers - they are hrtimer based, for exactly this reason.

Quoted 6 lines

Quoted 3 lines
instead of trying to fix the bug the wrong way, please try to fix it the right way, ontop of an already existing and tested patch, ok? That also enables the other neat stuff Thomas talked about.
Well that's nice, but I have a bugfix here which probably needs to get backported to stable kernels and distro kernels.

yes but your patch already exists for them which they can pick up.

really, this is a common Linux principle: fix it completely and fix it the right way. You are applying it yourself on a daily basis when having the maintainer hat on =B-)

Quoted 3 lines

It should be just as easy to rebase the hrtimer patch on top of my fix. Considering that you've had it for a year, I don't think it needs to be added right before my fix.

your latest patch looks quite kludgy, exactly due to the issues that were mentioned.

Quoted 5 lines

Quoted 2 lines
hm. I'm wondering how this wasnt noticed sooner - this futex_wait behavior has been there for like forever.
People ignore LTP test failures, and programs probably try to avoid exercising the nuances of the unix signal API, I guess.

then there's no rush and lets do this the right way, ok?

Ingo

Nick Piggin

2007-03-12 11:52 UTC #10 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 6 lines · Ingo Molnar

Quoted 3 lines · Nick Piggin
Quoted 2 lines
the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So

Quoted 8 lines

Quoted 2 lines
But using schedule_timeout is not a bug. Userspace timeouts are always defined to be "at least".
but what you are adding isnt a plain schedule_timeout(), it is a restart block handling loop. And for those restart blocks that relate to timeouts, we only use hrtimers. I am not making this up to annoy you:
take a look at all the current restart block handlers - they are hrtimer based, for exactly this reason.

So why do you say it is fundamentally incomplete?

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
instead of trying to fix the bug the wrong way, please try to fix it the right way, ontop of an already existing and tested patch, ok? That also enables the other neat stuff Thomas talked about.

Quoted 8 lines

Quoted 2 lines
Well that's nice, but I have a bugfix here which probably needs to get backported to stable kernels and distro kernels.
yes but your patch already exists for them which they can pick up.
really, this is a common Linux principle: fix it completely and fix it the right way. You are applying it yourself on a daily basis when having the maintainer hat on =B-)

I still didn't get anything wrong pointed out with the patch, though.

I'm not arguing against using hrtimers here to fix it the "right way".

Quoted 6 lines

Quoted 3 lines
It should be just as easy to rebase the hrtimer patch on top of my fix. Considering that you've had it for a year, I don't think it needs to be added right before my fix.
your latest patch looks quite kludgy, exactly due to the issues that were mentioned.

I don't see what is kludgy, unless you consider converting to and from absolute timeouts. But the userspace API is relative time based, so hrtimers doesn't change that.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
hm. I'm wondering how this wasnt noticed sooner - this futex_wait behavior has been there for like forever.
People ignore LTP test failures, and programs probably try to avoid exercising the nuances of the unix signal API, I guess.
then there's no rush and lets do this the right way, ok?

There is no rush to use hrtimers. I would have thought it fairly important to actually reach correctness, though. We're not talking about completely changing the design of something such that it will take a lot of work to "fix it properly". If that were the issue, then I would consider the hrtimer conversion as part of the fix.

And if you talk about doing it the right way, then I don't think it is strictly the right way to reimplement the function, including known bugs, to be slightly more efficient, and *then* fixing those bugs. I'd actually consider it better to fix the bugs first, not only because of the backport issue, but because it generally makes it easier to track the injection and removal points of bugs in the history.

Ingo Molnar

2007-03-12 12:21 UTC #11 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 7 lines · Nick Piggin

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So
But using schedule_timeout is not a bug. Userspace timeouts are always defined to be "at least".

Quoted 7 lines

Quoted 5 lines
but what you are adding isnt a plain schedule_timeout(), it is a restart block handling loop. And for those restart blocks that relate to timeouts, we only use hrtimers. I am not making this up to annoy you: take a look at all the current restart block handlers - they are hrtimer based, for exactly this reason.
So why do you say it is fundamentally incomplete?

because i misread your last patch :-) I thought it still has a window for inaccuracy, but you are right: it should be at most 1 jiffy inaccurate, no matter how many times we restart.

still ... the hrtimers patch has been submitted to lkml before yours, and has been tested extensively, so why go the extra side-jump prolonging the jiffies sleep method? The LTP failure has been there since the inception of the futex code i suspect. Going this way also enables the addressing of a more pressing need: the elimination of glibc's forced use of relative futex timeouts.

Ingo

Nick Piggin

2007-03-12 12:36 UTC #12 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 16 lines · Ingo Molnar

Quoted 13 lines · Nick Piggin
Quoted 12 lines
Quoted 6 lines
Quoted 3 lines
the issue is this: your fix reduces the effects of the bug but it is still fundamentally incomplete because of the use of timer_list. So
But using schedule_timeout is not a bug. Userspace timeouts are always defined to be "at least".
but what you are adding isnt a plain schedule_timeout(), it is a restart block handling loop. And for those restart blocks that relate to timeouts, we only use hrtimers. I am not making this up to annoy you: take a look at all the current restart block handlers - they are hrtimer based, for exactly this reason.

Quoted 5 lines

Quoted 1 line
So why do you say it is fundamentally incomplete?
because i misread your last patch :-) I thought it still has a window for inaccuracy, but you are right: it should be at most 1 jiffy inaccurate, no matter how many times we restart.

OK, no problem.

Quoted 6 lines

still ... the hrtimers patch has been submitted to lkml before yours, and has been tested extensively, so why go the extra side-jump prolonging the jiffies sleep method? The LTP failure has been there since the inception of the futex code i suspect. Going this way also enables the addressing of a more pressing need: the elimination of glibc's forced use of relative futex timeouts.

I guess my arguments are that my patch fixes a bug, which gives it a higher priority (being a userspace API bug, perhaps even 2.6.21); and that it will want to be backported while the hrtimer patch will not, so including the hrtimer patch first means 2 different patches to fix the same bug.

I'm not trying to make life harder for the hrtimer patch. I will even volunteer to forward port it on top of the restart fix, if that is an issue.

Andi Kleen

2007-03-12 10:28 UTC #13 lore raw

Re: [patch] change futex_wait() to hrtimers

In reply to Ingo Molnar

Quoted 3 lines · Ingo Molnar

the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.

But isn't that bad for power management? You'll likely get more idle wakeups, won't you?

-Andi

Thomas Gleixner

2007-03-12 10:53 UTC #14 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 7 lines · Andi Kleen

Quoted 3 lines · Ingo Molnar
the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.
But isn't that bad for power management? You'll likely get more idle wakeups, won't you?

Why so ? It comes more precise, but only once.

tglx

Andi Kleen

2007-03-12 10:58 UTC #15 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Andi Kleen
Quoted 3 lines · Ingo Molnar
the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.
But isn't that bad for power management? You'll likely get more idle wakeups, won't you?
Why so ? It comes more precise, but only once.

When it's clustered around the jiffies interval then wakeups from multiple processes will be somewhat batched. With a precise wakeup you'll get wakeups all over the jiffies period, won't you?

-Andi

Ingo Molnar

2007-03-12 11:04 UTC #16 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 7 lines · Andi Kleen

Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Andi Kleen
Quoted 3 lines · Ingo Molnar
the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
But isn't that bad for power management? You'll likely get more idle wakeups, won't you?
Why so ? It comes more precise, but only once.
When it's clustered around the jiffies interval then wakeups from multiple processes will be somewhat batched. With a precise wakeup you'll get wakeups all over the jiffies period, won't you?

if HIGH_RES_TIMERS is disabled then that is what happens. But frankly, most futex waits are without timeouts - if an application cares about micro-effects like that then you are much better off not using a per-futex timeout anyway.

Ingo

Andi Kleen

2007-03-12 11:20 UTC #17 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 1 line

if HIGH_RES_TIMERS is disabled then that is what happens. But frankly,

disabled? I would expect it (= more wakeups) when hrtimers are enabled.

Quoted 3 lines

most futex waits are without timeouts - if an application cares about micro-effects like that then you are much better off not using a per-futex timeout anywa

That sounds like you're arguing for not using hrtimers here because the applications shouldn't depend on precise timeouts here anyways?!?

Anyways when you convert more kernel timeouts to hrtimers you should probably add some kind of batching facility that can be globally configured with a sysctl or similar. Then at least laptops (and possibly servers) can opt for more power saving again. For the futexes alone it probably won't matter too much agreed, but I see a trend to more hr.

I also liked the idea (stolen from another popular OS) that a application can tell the OS how precise it wants its wakeups to be.

-Andi

Ingo Molnar

2007-03-12 11:29 UTC #18 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 5 lines · Andi Kleen

Quoted 2 lines
if HIGH_RES_TIMERS is disabled then that is what happens. But frankly,
disabled? I would expect it (= more wakeups) when hrtimers are enabled.

i mean the groupping of timer expiries happens automatically when high-res is disabled. When high-res is asked for then, duh, it's enabled and you get precise timeouts ;-)

Quoted 6 lines

Quoted 3 lines
most futex waits are without timeouts - if an application cares about micro-effects like that then you are much better off not using a per-futex timeout anywa
That sounds like you're arguing for not using hrtimers here because the applications shouldn't depend on precise timeouts here anyways?!?

I was talking about the "micro-effect" of grouping timer expiries.

Quoted 6 lines

Anyways when you convert more kernel timeouts to hrtimers you should probably add some kind of batching facility that can be globally configured with a sysctl or similar. Then at least laptops (and possibly servers) can opt for more power saving again. For the futexes alone it probably won't matter too much agreed, but I see a trend to more hr.

yeah, we had that in earlier versions, it's trivial - nobody used it. So i'll wait for the actual measurements and a patch (or i can do the patch too, if someone comes up with the measurements). I've added /proc/timer_stats and /proc/timer_info for exactly such reasons.

( note that we've added the facility for even more imprecise sleeps to
the timer_list APIs - but for hrtimers it's a lot less clear-cut case. )

Ingo

Theodore Tso

2007-03-12 14:13 UTC #19 lore raw

Re: [patch] change futex_wait() to hrtimers

In reply to Andi Kleen

Quoted 7 lines · Andi Kleen

Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Andi Kleen
Quoted 3 lines · Ingo Molnar
the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
But isn't that bad for power management? You'll likely get more idle wakeups, won't you?
Why so ? It comes more precise, but only once.
When it's clustered around the jiffies interval then wakeups from multiple processes will be somewhat batched. With a precise wakeup you'll get wakeups all over the jiffies period, won't you?

What we probably need in the long-term, and not just for high precision wakeups, is we need a way for waiters (either in the kernel or in userspace) to specify a desired precision in their timers. Is it, "wake me up in a second, exactly", or "wake me up in a second,
plus or minus 10ms"? (or 50ms? or 100ms?).

This becomes especially important if we want the tickless code to really shine as far as power management is concerned. Unfortunately, the POSIX timer abstraction doesn't give this kind of flexibility easily, so it's going to be a while before we see significant userspace adoption of such a kernel feature, but I think it's something that would be still worthwhile to add.

Regards,

- Ted

Andi Kleen

2007-03-12 14:22 UTC #20 lore raw

Re: [patch] change futex_wait() to hrtimers

Quoted 6 lines

This becomes especially important if we want the tickless code to really shine as far as power management is concerned. Unfortunately, the POSIX timer abstraction doesn't give this kind of flexibility easily, so it's going to be a while before we see significant userspace adoption of such a kernel feature, but I think it's something that would be still worthwhile to add.

I suspect it would be overkill to specify that on every sleep operation. 99% of applications won't care and for the 1% leftover a global per process setting should be fine.
I think a single prctl() and a global sysctl as default would be enough.

-Andi

Ingo Molnar

2007-03-12 14:32 UTC #21 lore raw

Re: [patch] change futex_wait() to hrtimers

In reply to Theodore Tso

Quoted 5 lines · Theodore Tso

What we probably need in the long-term, and not just for high precision wakeups, is we need a way for waiters (either in the kernel or in userspace) to specify a desired precision in their timers. Is it, "wake me up in a second, exactly", or "wake me up in a second, plus or minus 10ms"? (or 50ms? or 100ms?).

such a facility exists already, see round_jiffies() and round_jiffies_relative(). There's some short blurb about it at:

http://kernelnewbies.org/LinuxChanges#head-513ceda14f5d8cf5b8a7c81d7e3821543141ecb0

Quoted 2 lines

This becomes especially important if we want the tickless code to really shine as far as power management is concerned. [...]

yes. That's why we also implemented /proc/timer_stat, and this was measured and a few higher-frequency fuzzy waiters were converted to use round_jiffies(). Some other waiters were fixed in user-space. It's all dependent on actual measurements and circumstances.

Ingo

Nick Piggin

2007-03-12 14:32 UTC #22 lore raw

Re: [patch] change futex_wait() to hrtimers

In reply to Theodore Tso

Quoted 17 lines · Theodore Tso

Quoted 7 lines · Andi Kleen
Quoted 6 lines · Thomas Gleixner
Quoted 5 lines · Andi Kleen
Quoted 3 lines · Ingo Molnar
the only correct approach is the use of hrtimers, and a patch exists for that - see below. This has been included in -rt for quite some time.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
But isn't that bad for power management? You'll likely get more idle wakeups, won't you?
Why so ? It comes more precise, but only once.
When it's clustered around the jiffies interval then wakeups from multiple processes will be somewhat batched. With a precise wakeup you'll get wakeups all over the jiffies period, won't you?

Quoted 5 lines

What we probably need in the long-term, and not just for high precision wakeups, is we need a way for waiters (either in the kernel or in userspace) to specify a desired precision in their timers. Is it, "wake me up in a second, exactly", or "wake me up in a second,
plus or minus 10ms"? (or 50ms? or 100ms?).

Would this work, or will it just create more confusion for the API user? I mean, all sleeps can only guarantee "no less than".

Would it be enough for a binary (exact as possible / relaxed if needed) flag? Or perhaps ternary (exact/relaxed/batched), where relaxed could add an extra jiffy or so, and batched is really relaxed that may delay up to double the value of the timeout.

Quoted 6 lines

This becomes especially important if we want the tickless code to really shine as far as power management is concerned. Unfortunately, the POSIX timer abstraction doesn't give this kind of flexibility easily, so it's going to be a while before we see significant userspace adoption of such a kernel feature, but I think it's something that would be still worthwhile to add.

But given that we know most userspace API timeouts are broadly just an "equal to or greater", then we could add another timeout flag to specify it is a userspace timeout, and make that controllable by sysctl.

Sure it isn't ideal, but for those who really want power / hypervisor savings, it could be useful.

BTW. my futex man page says timeout's contents "describe the maximum duration of the wait". Surely that should be *minimum*? Michael cc'ed.