N1int for interactivity

PATCH In review Con Kolivas 5 participants 9 messages Jul 15, 2003 – Jul 16, 2003 Scheduler

Share & tools

Con Kolivas

author

2003-07-15 03:52 UTC #0 lore raw

PATCH 1/1 N1int for interactivity

I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches. This patch applies to any kernel patched up to the latest patch-O5int-0307150857 which applies on top of 2.5.75-mm1.

Please test and comment, and advise what further changes you think are appropriate or necessary, including other archs. I've preserved Mike's code unchanged wherever possible. It works well for me, but n=1 does not a good sample population make.

The patch-N1int-0307151249 is available here:
http://kernel.kolivas.org/2.5

and here:

include/linux/sched.h +3 -1

diff -Naurp linux-2.5.75-mm1/include/linux/sched.h linux-2.5.75-test/include/linux/sched.h
@@ -341,7 +341,9 @@ struct task_struct {
 
 	unsigned long sleep_avg;
 	unsigned long avg_start;
-	unsigned long last_run;
+	unsigned long long last_run;
+	unsigned int run_nsecs;
+	unsigned int sleep_nsecs;
 
 	unsigned long policy;
 	cpumask_t cpus_allowed;

kernel/sched.c +88 ~~-58~~

233 lines not on this page · raw message

arch/i386/kernel/timers/timer_tsc.c +4 -3

diff -Naurp linux-2.5.75-mm1/arch/i386/kernel/timers/timer_tsc.c linux-2.5.75-test/arch/i386/kernel/timers/timer_tsc.c
@@ -102,12 +102,13 @@ static unsigned long get_offset_tsc(void
 static unsigned long long monotonic_clock_tsc(void)
 {
 	unsigned long long last_offset, this_offset, base;
-	
+	unsigned long flags;
+
 	/* atomically read monotonic base & last_offset */
-	read_lock_irq(&monotonic_lock);
+	read_lock_irqsave(&monotonic_lock, flags);
 	last_offset = ((unsigned long long)last_tsc_high<<32)|last_tsc_low;
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);
 
 	/* Read the Time Stamp Counter */
 	rdtscll(this_offset);

Andrew Morton

2003-07-15 03:52 UTC #1 lore raw

Quoted 5 lines · Con Kolivas

I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches.
+inline void __scheduler_tick(runqueue_t *rq, task_t *p)

Two callsites, this guy shouldn't be inlined.

Should it have static scope? The code as-is generates a third copy...

Quoted 5 lines

static unsigned long long monotonic_clock_tsc(void) {
arch/i386/kernel/timers/timer_tsc.c +1 -1
 	unsigned long long last_offset, this_offset, base;
-	
+	unsigned long flags;

arch/i386/kernel/timers/timer_tsc.c static unsigned long get_offset_tsc(void · line 113

Quoted 10 lines

arch/i386/kernel/timers/timer_tsc.c +3 -2

+
 	/* atomically read monotonic base & last_offset */
-	read_lock_irq(&monotonic_lock);
+	read_lock_irqsave(&monotonic_lock, flags);
 	last_offset = ((unsigned long long)last_tsc_high<<32)|last_tsc_low;
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);
 
 	/* Read the Time Stamp Counter */

Why do we need to take a global lock here? Can't we use get_cycles() or something?

Have all the other architectures been reviewed to see if they need this change?

Con Kolivas

author

2003-07-15 03:46 UTC #2 lore raw

Quoted 24 lines · Andrew Morton

Quoted 4 lines · Con Kolivas
I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches.
+inline void __scheduler_tick(runqueue_t *rq, task_t *p)
Two callsites, this guy shouldn't be inlined.
Should it have static scope? The code as-is generates a third copy...
Quoted 14 lines
static unsigned long long monotonic_clock_tsc(void) {
arch/i386/kernel/timers/timer_tsc.c +4 -3
 	unsigned long long last_offset, this_offset, base;
-
+	unsigned long flags;
+
 	/* atomically read monotonic base & last_offset */
-	read_lock_irq(&monotonic_lock);
+	read_lock_irqsave(&monotonic_lock, flags);
 	last_offset = ((unsigned long long)last_tsc_high<<32)|last_tsc_low;
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);

Quoted 8 lines

Quoted 1 line
/* Read the Time Stamp Counter */
Why do we need to take a global lock here? Can't we use get_cycles() or something?
Have all the other architectures been reviewed to see if they need this change?

I'm calling for help here. This is getting way out of my depth; I've simply applied Mike's patch.

Con

Mike Galbraith

2003-07-15 06:43 UTC #3 lore raw

Quoted 4 lines · Con Kolivas

Quoted 3 lines · Andrew Morton
Quoted 2 lines · Con Kolivas
I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches.

(I think a repeat of an earlier public warning wrt this diff may be in order: run irman/contest before you proceed... here there be giant economy sized dragons)

Quoted 4 lines

Quoted 4 lines
Quoted 2 lines
+inline void __scheduler_tick(runqueue_t *rq, task_t *p)
Two callsites, this guy shouldn't be inlined.

(I just wild-guessed that it'd be a tiny bit faster while i was coding... _the_ fast-path and all)

Quoted 17 lines

Should it have static scope? The code as-is generates a third copy...

Quoted 14 lines

static unsigned long long monotonic_clock_tsc(void)

 {
    unsigned long long last_offset, this_offset, base;

arch/i386/kernel/timers/timer_tsc.c +4 -3

-
+   unsigned long flags;
+
    /* atomically read monotonic base & last_offset */
-   read_lock_irq(&monotonic_lock);
+   read_lock_irqsave(&monotonic_lock, flags);
    last_offset = ((unsigned long long)last_tsc_high<<32)|last_tsc_low;
    base = monotonic_base;
-   read_unlock_irq(&monotonic_lock);
+   read_unlock_irqrestore(&monotonic_lock, flags);

Quoted 4 lines

Quoted 4 lines
Quoted 1 line
    /* Read the Time Stamp Counter */
Why do we need to take a global lock here? Can't we use get_cycles() or something?

The scalability issue is gone I'm told.

Quoted 5 lines

Have all the other architectures been reviewed to see if they need this
Quoted 1 line
change?
I'm calling for help here. This is getting way out of my depth; I've simply applied Mike's patch.

Everyone will likely need that, or there will be major explosions. You'll also have to deal with the no-tsc case... either return jiffies * 1000000 or make it a config option.

(I'd go with a config option if I were you...dragons aren't all that likely to torch a desktop load, but if you do this globally, I'm quite certain that something is going to get char-broiled;)

-Mike

Zwane Mwaikambo

2003-07-15 06:57 UTC #4 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

arch/i386/kernel/timers/timer_tsc.c static unsigned long get_offset_tsc(void · line 113
Quoted 5 lines
arch/i386/kernel/timers/timer_tsc.c +1 -1
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);
 
 	/* Read the Time Stamp Counter */
Why do we need to take a global lock here? Can't we use get_cycles() or something?

I think that'll break even on some x86 boxes if we used get_cycles. I do wonder however why we need that lock, i see x86/64 uses seqlock at least. Although i can't vouch for whether that would have an adverse affect here. I presume Stultz would know.

Quoted 2 lines

Have all the other architectures been reviewed to see if they need this change?

No one else appears to have monotonic_clock, this would break every other arch out there.

Signature · 1 line

function.linuxpower.ca

Con Kolivas

author

2003-07-15 10:07 UTC #5 lore raw

Quoted 15 lines · Zwane Mwaikambo

Quoted 8 lines · Andrew Morton
Review
Quoted 5 lines
(quoted diff) +1 -1
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);
 
 	/* Read the Time Stamp Counter */
Why do we need to take a global lock here? Can't we use get_cycles() or something?
I think that'll break even on some x86 boxes if we used get_cycles. I do wonder however why we need that lock, i see x86/64 uses seqlock at least. Although i can't vouch for whether that would have an adverse affect here. I presume Stultz would know.

Quoted 5 lines

Quoted 2 lines
Have all the other architectures been reviewed to see if they need this change?
No one else appears to have monotonic_clock, this would break every other arch out there.

Ok what this patch is for is to see whether we really need to go down this path. It may be untidy and have dragons but it can be cleaned up and work with other archs but I just need some feedback by testers to see if it adds _significant_ benefit that warrants it's addition to the direction I'm already taking. Unfortunately all the incremental patches I've done seem to have scared people off :(

Here's a slightly tidier version (also at kernel.kolivas.org/2.5) which patches against 2.5.75-mm1 that has been patched with O5int.

akpm removed from cc list as it's not interesting for -mm atm

Con

arch/i386/kernel/timers/timer_tsc.c +4 -3

diff -Naurp linux-2.5.75-mm1/arch/i386/kernel/timers/timer_tsc.c linux-2.5.75-test/arch/i386/kernel/timers/timer_tsc.c
@@ -102,12 +102,13 @@ static unsigned long get_offset_tsc(void
 static unsigned long long monotonic_clock_tsc(void)
 {
 	unsigned long long last_offset, this_offset, base;
-	
+	unsigned long flags;
+
 	/* atomically read monotonic base & last_offset */
-	read_lock_irq(&monotonic_lock);
+	read_lock_irqsave(&monotonic_lock, flags);
 	last_offset = ((unsigned long long)last_tsc_high<<32)|last_tsc_low;
 	base = monotonic_base;
-	read_unlock_irq(&monotonic_lock);
+	read_unlock_irqrestore(&monotonic_lock, flags);
 
 	/* Read the Time Stamp Counter */
 	rdtscll(this_offset);

include/linux/sched.h +3 -1

diff -Naurp linux-2.5.75-mm1/include/linux/sched.h linux-2.5.75-test/include/linux/sched.h
@@ -341,7 +341,9 @@ struct task_struct {
 
 	unsigned long sleep_avg;
 	unsigned long avg_start;
-	unsigned long last_run;
+	unsigned long long last_run;
+	unsigned long run_nsecs;
+	unsigned long sleep_nsecs;
 
 	unsigned long policy;
 	cpumask_t cpus_allowed;

kernel/sched.c +84 ~~-58~~

229 lines not on this page · raw message

Felipe Alfaro Solana

2003-07-15 15:15 UTC #6 lore raw

In reply to Con Kolivas

Quoted 5 lines · Con Kolivas

I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches. This patch applies to any kernel patched up to the latest patch-O5int-0307150857 which applies on top of 2.5.75-mm1.

Quoted 7 lines

Please test and comment, and advise what further changes you think are appropriate or necessary, including other archs. I've preserved Mike's code unchanged wherever possible. It works well for me, but n=1 does not a good sample population make.
The patch-N1int-0307151249 is available here:
http://kernel.kolivas.org/2.5

I can still starve XMMS on 2.5.75-mm1 + patch-O5int-0307150857 + patch-N1int-0307152010:

1. Log on to KDE
2. Launch Konqueror
3. Launch XMMS and make it play
4. Move Konqueror window all over the desktop

Step 4 will make XMMS starve for a few seconds. Also, under heavy load (while true; do a=2; done), moving the Konqueror window like crazy makes X go jerky after a few seconds. If I quit moving windows around, after a few other seconds, X returns to normal/smooth behavior.

I can fix the starvation/smoothness by setting:

#define PRIO_BONUS_RATIO        45
#define INTERACTIVE_DELTA       4
#define MAX_SLEEP_AVG           (HZ)
#define STARVATION_LIMIT        (HZ)

For me, 2.6.0-test1 stock scheduler plus above changes makes the most user-friendly desktop I've ever seen.

Thanks!

Con Kolivas

author

2003-07-15 22:54 UTC #7 lore raw

Quoted 22 lines · Felipe Alfaro Solana

Quoted 5 lines · Con Kolivas
I've modified Mike Galbraith's nanosleep work for greater resolution to help the interactivity estimator work I've done in the O*int patches. This patch applies to any kernel patched up to the latest patch-O5int-0307150857 which applies on top of 2.5.75-mm1.
Quoted 7 lines
Please test and comment, and advise what further changes you think are appropriate or necessary, including other archs. I've preserved Mike's code unchanged wherever possible. It works well for me, but n=1 does not a good sample population make.
The patch-N1int-0307151249 is available here:
http://kernel.kolivas.org/2.5
I can still starve XMMS on 2.5.75-mm1 + patch-O5int-0307150857 + patch-N1int-0307152010:
1. Log on to KDE
2. Launch Konqueror
3. Launch XMMS and make it play
4. Move Konqueror window all over the desktop

Quoted 4 lines

Step 4 will make XMMS starve for a few seconds. Also, under heavy load (while true; do a=2; done), moving the Konqueror window like crazy makes X go jerky after a few seconds. If I quit moving windows around, after a few other seconds, X returns to normal/smooth behavior.

I was more concerned to see if the N1 patch added anything to the current behaviour, but that is abandoned work now so don't worry. I can address these further issues incrementally if you're willing to test them. My test boxes have been tamed but I need and appreciate greatly your testing.

Quoted 9 lines

I can fix the starvation/smoothness by setting:
#define PRIO_BONUS_RATIO        45
#define INTERACTIVE_DELTA       4
#define MAX_SLEEP_AVG           (HZ)
#define STARVATION_LIMIT        (HZ)
For me, 2.6.0-test1 stock scheduler plus above changes makes the most user-friendly desktop I've ever seen.

With a max sleep avg of just one second you'll never starve X or xmms for sustained periods; but under load X will not be smooth getting jerky even with small bursts of heavy X use; that's why Ingo increased the max sleep avg in the first place... but that led to other issues...

Con

Felipe Alfaro Solana

2003-07-16 06:57 UTC #8 lore raw

Quoted 13 lines · Con Kolivas

Quoted 8 lines
I can still starve XMMS on 2.5.75-mm1 + patch-O5int-0307150857 + patch-N1int-0307152010:
1. Log on to KDE
2. Launch Konqueror
3. Launch XMMS and make it play
4. Move Konqueror window all over the desktop
Quoted 4 lines
Step 4 will make XMMS starve for a few seconds. Also, under heavy load (while true; do a=2; done), moving the Konqueror window like crazy makes X go jerky after a few seconds. If I quit moving windows around, after a few other seconds, X returns to normal/smooth behavior.

Quoted 4 lines

I was more concerned to see if the N1 patch added anything to the current behaviour, but that is abandoned work now so don't worry. I can address these further issues incrementally if you're willing to test them. My test boxes have been tamed but I need and appreciate greatly your testing.

Just throw anything to me and I will test it gladly :-)

Quoted 7 lines

Quoted 7 lines
I can fix the starvation/smoothness by setting:
#define PRIO_BONUS_RATIO        45
#define INTERACTIVE_DELTA       4
#define MAX_SLEEP_AVG           (HZ)
#define STARVATION_LIMIT        (HZ)

Quoted 7 lines

Quoted 2 lines
For me, 2.6.0-test1 stock scheduler plus above changes makes the most user-friendly desktop I've ever seen.
With a max sleep avg of just one second you'll never starve X or xmms for sustained periods; but under load X will not be smooth getting jerky even with small bursts of heavy X use; that's why Ingo increased the max sleep avg in the first place... but that led to other issues...

Well, under heavy load (while true ...) X still feels very smooth with 2.6.0-test1 and the above changes. In fact, I was inclined to make those changes as X didn't feel smooth under load on my system without them.