Skip to contentSkip to conversation

Linux 2.5.23-dj1

PATCH In review Dave Jones 6 participants 16 messages Jun 19, 2002 – Jun 21, 2002

Subsystems (3)
lore
Share & tools
lore

Dave Jones

author
#0 lore raw

Keeping up to date, and getting some of the more important bits out of the way from the pending folder.

As usual,..

Patch against 2.5.23 vanilla is available from:
ftp://ftp.kernel.org/pub/linux/kernel/people/davej/patches/2.5/

Merged patch archive: http://www.codemonkey.org.uk/patches/merged/

Check http://www.codemonkey.org.uk/Linux-2.5.html before reporting known bugs that are also in mainline.

-- Davej.

2.5.23-dj1

o   Small UP optimisation in the scheduler.		(James Bottomley)
o   Update x86 cpufreq scaling code.			(Dominik Brodowski)
o   Export ioremap_nocache() for modules.		(Andi Kleen)
o   Export default_wake_function() for modules.		(Benjamin LaHaise)
o   Compaq hotplug compile fixes.			(Felipe Contreras)
o   Fix migration thread for non linear numbered CPUs.	(Ingo Molnar)
o   Framebuffer updates.				(James Simmons)
o   Introduce CONFIG_ISA option for i386.		(Andi Kleen)
o   Fix bad locking in driver/ core.			(Arnd Bergmann)
Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#1 lore raw

PATCH 1/1 scheduler bits from 2.5.23-dj1

the scheduler optimisation in 2.5.23-dj1, from James Bottomley, looks fine to me. I did some modifications:

- use another form: task_cpu(p) & set_task_cpu(p, cpu), which is shorter for the common uses - and this way the optimisation also makes the source more compact.

- put task_cpu() and set_task_cpu() into sched.h, since these are task-level operations.

- there is no need for the #if CONFIG_SMP, gcc is good at optimizing away a branch if it has a (0 != 0) condition on UP :-)

- (i did not carry over all the comment changes, only the speling ones.)

another change in 2.5.23-dj1 is the initialization of the pidhash in sched_init(). It does not belong there - please create a new init function within fork.c if needed. The pidhash init used to be in sched_init(), but this doesnt make it right.

And i'm not quite sure whether it's needed to expose the pidhash to the rest of the kernel - it would be much simpler to have it in kernel/fork.c locally, and find_task_by_pid() would be a function instead of an inline. (it has a ~49 bytes footprint on x86, it's rather heavy i think.)

my current scheduler patchset (against 2.5.23, tested) is attached.

Ingo

include/linux/sched.h +28 -0
diff -Nru a/include/linux/sched.h b/include/linux/sched.h
@@ -863,6 +863,34 @@
 		clear_thread_flag(TIF_SIGPENDING);
 }
 
+/*
+ * Wrappers for p->thread_info->cpu access. No-op on UP.
+ */
+#ifdef CONFIG_SMP
+
+static inline unsigned int task_cpu(struct task_struct *p)
+{
+	return p->thread_info->cpu;
+}
+
+static inline unsigned int set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+	p->thread_info->cpu = cpu;
+}
+
+#else
+
+static inline unsigned int task_cpu(struct task_struct *p)
+{
+	return 0;
+}
+
+static inline unsigned int set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+}
+
+#endif /* CONFIG_SMP */
+
 #endif /* __KERNEL__ */
 
 #endif
kernel/sched.c +25 -23

151 lines not on this page · raw message

#2 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 2 lines · Ingo Molnar

the scheduler optimisation in 2.5.23-dj1, from James Bottomley, look fine to me. I did some modifications:

Nice.

Review

Quoted 2 lines
(quoted diff) +2 -0
+static inline unsigned int task_cpu(struct task_struct *p)
+static inline unsigned int set_task_cpu(struct task_struct *p, unsigned int cpu)

Technically, shouldn't we make these `unsigned long' ?

I know on x86 we store cpu as a `u32' so it does not matter per se, but in reality it is an unsigned long and other architectures may export it as such.

Further, we compare and set it against the various CPU bitmaps and they are all `unsigned long' and we do shifts against 1UL ...

Robert Love

#3 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 11 lines · Robert Love
Quoted 2 lines · Ingo Molnar

the scheduler optimisation in 2.5.23-dj1, from James Bottomley, look fine to me. I did some modifications:

Nice.

Review

Quoted 2 lines
(quoted diff) +2 -0
+static inline unsigned int task_cpu(struct task_struct *p)
+static inline unsigned int set_task_cpu(struct task_struct *p, unsigned int cpu)

Technically, shouldn't we make these `unsigned long' ?

obviously not. Supporting 4G cpus is enough for this century, so the other 32bit would be just wasted space. the 1 in the shiftleft needs the UL anyways to be correct with >32 cpus (it's not strictly a bug right now to forget the UL but if we get it right we'll be able to go 64-way on 64bit systems with no change other than NR_TASKS). So the bitmasks must be all unsigned longs, the cpu numbers are definitely fine as unsigned ints.

Even after we break at some point the 64CPU limit growing the bitmask ala sigset_t, still the cpu numbers will remain unsigned int for a very long time (probably we'll never have a chance to see the need of unsigned long cpu numbers in our lifes).

Andrea

#4 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 7 lines · Andrea Arcangeli

obviously not. Supporting 4G cpus is enough for this century, so the other 32bit would be just wasted space. the 1 in the shiftleft needs the UL anyways to be correct with >32 cpus (it's not strictly a bug right now to forget the UL but if we get it right we'll be able to go 64-way on 64bit systems with no change other than NR_TASKS). So the bitmasks must be all unsigned longs, the cpu numbers are definitely fine as unsigned ints.

Eh, very true. I was confusing the bitmasks and the counts.

Sorry, Ingo - ignore me.

Robert Love

author
#5 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

In reply to Ingo Molnar

On Thu, Jun 20, 2002 at 01:35:35AM +0200, Ingo Molnar wrote:

Hi Ingo,

Quoted 2 lines

the scheduler optimisation in 2.5.23-dj1, from James Bottomley, look fine to me. I did some modifications:

Thanks for taking time out to look them over..
The bulk of the UP optimisation was by Mikael Petterson, James just fixed up something I goofed in an earlier patchset.

Quoted 2 lines

- there is no need for the #if CONFIG_SMP, gcc is good at optimizing away a branch if it has a (0 != 0) condition on UP :-)

*nod*, I had intended to clean this up, but there's only so many hours in a day.. 8-)

Quoted 4 lines

another change in 2.5.23-dj1 is the initialization of the pidhash in sched_init(). It does not belong there - please create a new init function within fork.c if needed. The pidhash init used to be in sched_init(), but this doesnt make it right.

Agreed.

Quoted 4 lines

And i'm not quite sure whether it's needed to expose the pidhash to the rest of the kernel - it would be much simpler to have it in kernel/fork.c locally, and find_task_by_pid() would be a function instead of an inline. (it has a ~49 bytes footprint on x86, it's rather heavy i think.)

I'll take a look at this tomorrow, unless William "no sleep `til 2.6" Irwin beats me to it 8-) (he did this part of the patch iirc).

Quoted 1 line

my current scheduler patchset (against 2.5.23, tested) is attached.

Thanks again.

Dave.

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#6 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 2 lines · Dave Jones

I'll take a look at this tomorrow, unless William "no sleep `til 2.6" Irwin beats me to it 8-) (he did this part of the patch iirc).

How does this look? (compiles, boots, & runs on UP i386)

Cheers,
Bill

include/linux/sched.h +1 -0
diff -urN linux-2.5.23-virgin/include/linux/sched.h linux-2.5.23-wli/include/linux/sched.h
@@ -144,6 +144,7 @@
 typedef struct task_struct task_t;
 
 extern void sched_init(void);
+extern void pidhash_init(void);
 extern void init_idle(task_t *idle, int cpu);
 extern void show_state(void);
 extern void cpu_init (void);
init/main.c +1 -0
diff -urN linux-2.5.23-virgin/init/main.c linux-2.5.23-wli/init/main.c
@@ -347,6 +347,7 @@
 	trap_init();
 	init_IRQ();
 	sched_init();
+	pidhash_init();
 	softirq_init();
 	time_init();
 
kernel/fork.c +20 -1
diff -urN linux-2.5.23-virgin/kernel/fork.c linux-2.5.23-wli/kernel/fork.c
@@ -27,7 +27,7 @@
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/jiffies.h>
-
+#include <linux/bootmem.h>
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>
@@ -49,6 +49,25 @@
 
 list_t *pidhash;
 unsigned long pidhash_size;
+
+void __init pidhash_init(void)
+{
+	int i, size = PAGE_SIZE*sizeof(list_t);
+
+	do {
+		pidhash = (list_t *)alloc_bootmem(size);
+		if (!pidhash)
+			size >>= 1;
+	} while (!pidhash && size >= sizeof(list_t));
+
+	if (!pidhash)
+		panic("Failed to allocated pid hash table!\n");
+
+	pidhash_size = size/sizeof(list_t);
+
+	for (i = 0; i < pidhash_size; ++i)
+		INIT_LIST_HEAD(&pidhash[i]);
+}
 
 rwlock_t tasklist_lock __cacheline_aligned = RW_LOCK_UNLOCKED;  /* outer */
 
kernel/sched.c +1 -15
diff -urN linux-2.5.23-virgin/kernel/sched.c linux-2.5.23-wli/kernel/sched.c
@@ -1662,21 +1662,7 @@
 void __init sched_init(void)
 {
 	runqueue_t *rq;
-	int i, j, k, size = PAGE_SIZE*sizeof(list_t);
-
-	do {
-		pidhash = (list_t *)alloc_bootmem(size);
-		if (!pidhash)
-			size >>= 1;
-	} while (!pidhash && size >= sizeof(list_t));
-
-	if (!pidhash)
-		panic("Failed to allocated pid hash table!\n");
-
-	pidhash_size = size/sizeof(list_t);
-
-	for (i = 0; i < pidhash_size; ++i)
-		INIT_LIST_HEAD(&pidhash[i]);
+	int i, j, k;
 
 	for (i = 0; i < NR_CPUS; i++) {
 		prio_array_t *array;
#7 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 5 lines · William Lee Irwin III
Quoted 2 lines · Dave Jones

I'll take a look at this tomorrow, unless William "no sleep `til 2.6" Irwin beats me to it 8-) (he did this part of the patch iirc).

How does this look? (compiles, boots, & runs on UP i386)

looks good to me - what do you think about my other pidhash suggestion:

Quoted 5 lines

And i'm not quite sure whether it's needed to expose the pidhash to the rest of the kernel - it would be much simpler to have it in kernel/fork.c locally, and find_task_by_pid() would be a function instead of an inline. (it has a ~49 bytes footprint on x86, it's rather heavy i think.)

Ingo

#8 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 7 lines · Ingo Molnar

looks good to me - what do you think about my other pidhash suggestion:

Quoted 5 lines

And i'm not quite sure whether it's needed to expose the pidhash to the rest of the kernel - it would be much simpler to have it in kernel/fork.c locally, and find_task_by_pid() would be a function instead of an inline. (it has a ~49 bytes footprint on x86, it's rather heavy i think.)

It's an excellent idea, I think I only forgot that was in the queue of things to write. I'll follow up with that as well.

Thanks,
Bill

#9 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

In reply to Ingo Molnar
Quoted 1 line · Ingo Molnar

looks good to me - what do you think about my other pidhash suggestion:

An excellent idea. I didn't go all the way and make the pidhash entirely private to fork.c but taking find_task_by_pid() out-of-line is implemented in the following, built atop the prior patch. I can also privatize the pidhash entirely if that's wanted.

Cheers,
Bill

include/linux/sched.h +1 -13
diff -urN linux-2.5.23-virgin/include/linux/sched.h linux-2.5.23-wli/include/linux/sched.h
@@ -459,19 +459,7 @@
 	list_del(&p->pidhash_list);
 }
 
-static inline task_t *find_task_by_pid(int pid)
-{
-	list_t *p, *pid_list = &pidhash[pid_hashfn(pid)];
-
-	list_for_each(p, pid_list) {
-		task_t *t = list_entry(p, task_t, pidhash_list);
-
-		if(t->pid == pid)
-			return t;
-	}
-
-	return NULL;
-}
+extern task_t *find_task_by_pid(int pid);
 
 /* per-UID process charging. */
 extern struct user_struct * alloc_uid(uid_t);
kernel/fork.c +15 -0
diff -urN linux-2.5.23-virgin/kernel/fork.c linux-2.5.23-wli/kernel/fork.c
@@ -69,6 +69,21 @@
 		INIT_LIST_HEAD(&pidhash[i]);
 }
 
+task_t *find_task_by_pid(int pid)
+{
+	list_t *p, *pid_list = &pidhash[pid_hashfn(pid)];
+
+	list_for_each(p, pid_list) {
+		task_t *t = list_entry(p, task_t, pidhash_list);
+
+		if(t->pid == pid)
+			return t;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(find_task_by_pid);
+
 rwlock_t tasklist_lock __cacheline_aligned = RW_LOCK_UNLOCKED;  /* outer */
 
 void add_wait_queue(wait_queue_head_t *q, wait_queue_t * wait)
#10 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 1 line · Ingo Molnar
Quoted 1 line

looks good to me - what do you think about my other pidhash suggestion:

Quoted 4 lines · William Lee Irwin III

An excellent idea. I didn't go all the way and make the pidhash entirely private to fork.c but taking find_task_by_pid() out-of-line is implemented in the following, built atop the prior patch. I can also privatize the pidhash entirely if that's wanted.

The final piece, built on top of the previous two. Please comment and/or use your discretion. Although I test compiled and booted it on UP i386, this may not be the desired cleanup. The original purpose of this was to enforce the usage of Linux' standard list data type for the pidhash.

Cheers,
Bill

include/linux/sched.h +2 -18
diff -urN linux-2.5.23-virgin/include/linux/sched.h linux-2.5.23-wli/include/linux/sched.h
@@ -441,24 +441,8 @@
 extern struct task_struct *init_tasks[NR_CPUS];
 
 /* PID hashing. */
-extern list_t *pidhash;
-extern unsigned long pidhash_size;
-
-static inline unsigned pid_hashfn(pid_t pid)
-{
-	return ((pid >> 8) ^ pid) & (pidhash_size - 1);
-}
-
-static inline void hash_pid(struct task_struct *p)
-{
-	list_add(&p->pidhash_list, &pidhash[pid_hashfn(p->pid)]);
-}
-
-static inline void unhash_pid(struct task_struct *p)
-{
-	list_del(&p->pidhash_list);
-}
-
+extern void hash_pid(task_t *);
+extern void unhash_pid(task_t *);
 extern task_t *find_task_by_pid(int pid);
 
 /* per-UID process charging. */
kernel/fork.c +18 -0
diff -urN linux-2.5.23-virgin/kernel/fork.c linux-2.5.23-wli/kernel/fork.c
@@ -69,6 +69,11 @@
 		INIT_LIST_HEAD(&pidhash[i]);
 }
 
+static inline unsigned pid_hashfn(pid_t pid)
+{
+	return ((pid >> 8) ^ pid) & (pidhash_size - 1);
+}
+
 task_t *find_task_by_pid(int pid)
 {
 	list_t *p, *pid_list = &pidhash[pid_hashfn(pid)];
@@ -82,7 +87,20 @@
 
 	return NULL;
 }
+
+void hash_pid(task_t *p)
+{
+	list_add(&p->pidhash_list, &pidhash[pid_hashfn(p->pid)]);
+}
+
+void unhash_pid(task_t *p)
+{
+	list_del(&p->pidhash_list);
+}
+
 EXPORT_SYMBOL(find_task_by_pid);
+EXPORT_SYMBOL(hash_pid);
+EXPORT_SYMBOL(unhash_pid);
 
 rwlock_t tasklist_lock __cacheline_aligned = RW_LOCK_UNLOCKED;  /* outer */
 
kernel/ksyms.c +0 -2
diff -urN linux-2.5.23-virgin/kernel/ksyms.c linux-2.5.23-wli/kernel/ksyms.c
@@ -602,5 +602,3 @@
 EXPORT_SYMBOL(init_thread_union);
 
 EXPORT_SYMBOL(tasklist_lock);
-EXPORT_SYMBOL(pidhash);
-EXPORT_SYMBOL(pidhash_size);
#11 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 4 lines · William Lee Irwin III

The final piece, built on top of the previous two. Please comment and/or use your discretion. Although I test compiled and booted it on UP i386, this may not be the desired cleanup. The original purpose of this was to enforce the usage of Linux' standard list data type for the pidhash.

Okay, one more piece, thanks to Brad Heilbrun <[email protected]>:

Cheers,
Bill

kernel/fork.c +1 -1
diff -urN linux-2.5.23-virgin/kernel/fork.c linux-2.5.23-wli/kernel/fork.c
@@ -61,7 +61,7 @@
 	} while (!pidhash && size >= sizeof(list_t));
 
 	if (!pidhash)
-		panic("Failed to allocated pid hash table!\n");
+		panic("Failed to allocate pid hash table!\n");
 
 	pidhash_size = size/sizeof(list_t);
 
#12 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

In reply to Ingo Molnar
Quoted 4 lines · Ingo Molnar

another change in 2.5.23-dj1 is the initialization of the pidhash in sched_init(). It does not belong there - please create a new init function within fork.c if needed. The pidhash init used to be in sched_init(), but this doesnt make it right.

On its way shortly.

Cheers,
Bill

#13 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

In reply to Ingo Molnar

Ingo,
A small doubt,
Should'nt this function below return something ? set_task_cpu() should return unsigned int but it seems to do nothing ....

Quoted 9 lines · Ingo Molnar
include/linux/sched.h +7 -0
+
+static inline unsigned int set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+}
+
+#endif /* CONFIG_SMP */
+
 #endif /* __KERNEL__ */
#14 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

Quoted 5 lines · Manik Raina

Ingo,
A small doubt,
Should'nt this function below return something ? set_task_cpu() should return unsigned int but it seems to do nothing ....

it's a NOP on UP. On SMP it changes p->thread_info->cpu.

Ingo

#15 lore raw

Re: [patch] scheduler bits from 2.5.23-dj1

In reply to Manik Raina
Quoted 4 lines · Manik Raina

A small doubt,
Should'nt this function below return something ? set_task_cpu() should return unsigned int but it seems to do nothing ....

now i understand. We shouldnt return anything, it should be a 'void', like Mikael Pettersson suggests. New scheduler patch attached, with this fixed, against 2.5.23.

Ingo

include/linux/sched.h +28 -0
diff -Nru a/include/linux/sched.h b/include/linux/sched.h
@@ -863,6 +863,34 @@
 		clear_thread_flag(TIF_SIGPENDING);
 }
 
+/*
+ * Wrappers for p->thread_info->cpu access. No-op on UP.
+ */
+#ifdef CONFIG_SMP
+
+static inline unsigned int task_cpu(struct task_struct *p)
+{
+	return p->thread_info->cpu;
+}
+
+static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+	p->thread_info->cpu = cpu;
+}
+
+#else
+
+static inline unsigned int task_cpu(struct task_struct *p)
+{
+	return 0;
+}
+
+static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
+{
+}
+
+#endif /* CONFIG_SMP */
+
 #endif /* __KERNEL__ */
 
 #endif

diff -Nru a/include/linux/smp.h b/include/linux/smp.h

kernel/sched.c +25 -23

151 lines not on this page · raw message