Re: [PATCH] de-xchg fork.c

Re: [PATCH] de-xchg fork.c

[Pavel Machek]

Hi!

> Don't know who put this in for 2.5.38.
> 
> I realize that using xchg() makes you 'leet.  But doing an atomic op
> where none is required is suboptimal and confusing.

Well, atomic op is also more expensive. i think ingo did this. Ingo, is
patch below safe? It is faster *and* it works on 386.
								Pavel 

> diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.38/kernel/fork.c working-2.5.38-unxchg/kernel/fork.c
> --- linux-2.5.38/kernel/fork.c	2002-09-21 13:55:19.000000000 +1000
> +++ working-2.5.38-unxchg/kernel/fork.c	2002-09-23 11:00:31.000000000 +1000
> @@ -64,11 +64,12 @@ void __put_task_struct(struct task_struc
>  	} else {
>  		int cpu = smp_processor_id();
>  
> -		tsk = xchg(task_cache + cpu, tsk);
> +		tsk = task_cache[cpu];
>  		if (tsk) {
>  			free_thread_info(tsk->thread_info);
>  			kmem_cache_free(task_struct_cachep,tsk);
>  		}
> +		task_cache[cpu] = current;
>  	}
>  }

-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

[PATCH] de-xchg fork.c

[Rusty Russell]

Don't know who put this in for 2.5.38.

I realize that using xchg() makes you 'leet.  But doing an atomic op
where none is required is suboptimal and confusing.

Hey, at least it wasn't:
	if (likely(tsk = xchg(task_cache + cpu, tsk))) {

Cheers,
Rusty.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.38/kernel/fork.c working-2.5.38-unxchg/kernel/fork.c
--- linux-2.5.38/kernel/fork.c	2002-09-21 13:55:19.000000000 +1000
+++ working-2.5.38-unxchg/kernel/fork.c	2002-09-23 11:00:31.000000000 +1000
@@ -64,11 +64,12 @@ void __put_task_struct(struct task_struc
 	} else {
 		int cpu = smp_processor_id();
 
-		tsk = xchg(task_cache + cpu, tsk);
+		tsk = task_cache[cpu];
 		if (tsk) {
 			free_thread_info(tsk->thread_info);
 			kmem_cache_free(task_struct_cachep,tsk);
 		}
+		task_cache[cpu] = current;
 	}
 }
 

--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] de-xchg fork.c

[Robert Love]

On Mon, 2002-09-23 at 14:31, Ingo Molnar wrote:

> the attached patch (against BK-curr) fixes all xchg()'s and a preemption
> bug.

Thanks, Ingo.

> --- linux/kernel/fork.c.orig	Mon Sep 23 20:28:36 2002
> +++ linux/kernel/fork.c	Mon Sep 23 20:30:02 2002
> @@ -64,11 +64,12 @@
>  	} else {
>  		int cpu = smp_processor_id();
>  
> -		tsk = xchg(task_cache + cpu, tsk);
> +		tsk = task_cache[cpu];
>  		if (tsk) {
>  			free_thread_info(tsk->thread_info);
>  			kmem_cache_free(task_struct_cachep,tsk);
>  		}
> +		task_cache[cpu] = current;
>  	}
>  }

I think you need get/put_cpu() here, too?

	Robert Love

Re: [PATCH] de-xchg fork.c

[Ingo Molnar]

On Mon, 23 Sep 2002, Rusty Russell wrote:

> -		tsk = xchg(task_cache + cpu, tsk);
> +		tsk = task_cache[cpu];

this was me - i forgot that this are per-CPU fields. (hey and i wrote the
original task_cache code so there's no excuse :-)

the attached patch (against BK-curr) fixes all xchg()'s and a preemption
bug.

	Ingo

--- linux/kernel/fork.c.orig	Mon Sep 23 20:28:36 2002
+++ linux/kernel/fork.c	Mon Sep 23 20:30:02 2002
@@ -64,11 +64,12 @@
 	} else {
 		int cpu = smp_processor_id();
 
-		tsk = xchg(task_cache + cpu, tsk);
+		tsk = task_cache[cpu];
 		if (tsk) {
 			free_thread_info(tsk->thread_info);
 			kmem_cache_free(task_struct_cachep,tsk);
 		}
+		task_cache[cpu] = current;
 	}
 }
 
@@ -126,8 +127,11 @@
 {
 	struct task_struct *tsk;
 	struct thread_info *ti;
+	int cpu = get_cpu();
 
-	tsk = xchg(task_cache + smp_processor_id(), NULL);
+	tsk = task_cache[cpu];
+	task_cache[cpu] = NULL;
+	put_cpu();
 	if (!tsk) {
 		ti = alloc_thread_info();
 		if (!ti)

Re: [PATCH] de-xchg fork.c

[Ingo Molnar]

On 23 Sep 2002, Robert Love wrote:

> > +		task_cache[cpu] = current;
> >  	}
> >  }
> 
> I think you need get/put_cpu() here, too?

you are right - new patch attached.

	Ingo

--- linux/kernel/fork.c.orig	Mon Sep 23 20:28:36 2002
+++ linux/kernel/fork.c	Mon Sep 23 20:55:08 2002
@@ -62,13 +62,15 @@
 		free_thread_info(tsk->thread_info);
 		kmem_cache_free(task_struct_cachep,tsk);
 	} else {
-		int cpu = smp_processor_id();
+		int cpu = get_cpu();
 
-		tsk = xchg(task_cache + cpu, tsk);
+		tsk = task_cache[cpu];
 		if (tsk) {
 			free_thread_info(tsk->thread_info);
 			kmem_cache_free(task_struct_cachep,tsk);
 		}
+		task_cache[cpu] = current;
+		put_cpu();
 	}
 }
 
@@ -126,8 +128,11 @@
 {
 	struct task_struct *tsk;
 	struct thread_info *ti;
+	int cpu = get_cpu();
 
-	tsk = xchg(task_cache + smp_processor_id(), NULL);
+	tsk = task_cache[cpu];
+	task_cache[cpu] = NULL;
+	put_cpu();
 	if (!tsk) {
 		ti = alloc_thread_info();
 		if (!ti)