[PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Daniel Walker]

Adds a check for cmpxchg in get_task_struct_rcu(), and implements the
case when it doesn't exist.

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

Index: linux-2.6.13/include/linux/sched.h
===================================================================
--- linux-2.6.13.orig/include/linux/sched.h
+++ linux-2.6.13/include/linux/sched.h
@@ -1026,13 +1026,21 @@ static inline int get_task_struct_rcu(st
 {
 	int oldusage;
 
+#ifdef __HAVE_ARCH_CMPXCHG
 	do {
 		oldusage = atomic_read(&t->usage);
 		if (oldusage == 0) {
 			return 0;
 		}
 	} while (cmpxchg(&t->usage.counter,
-		 oldusage, oldusage + 1) != oldusage);
+				oldusage, oldusage + 1) != oldusage);
+#else
+	oldusage = atomic_read(&t->usage);
+	if (oldusage == 0) {
+		return 0;
+	}
+	atomic_inc(&t->usage);
+#endif
 	return 1;
 }
 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Nick Piggin]

Daniel Walker wrote:
> Adds a check for cmpxchg in get_task_struct_rcu(), and implements the
> case when it doesn't exist.
> 

I believe this is racy.

> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
> 
> Index: linux-2.6.13/include/linux/sched.h
> ===================================================================
> --- linux-2.6.13.orig/include/linux/sched.h
> +++ linux-2.6.13/include/linux/sched.h
> @@ -1026,13 +1026,21 @@ static inline int get_task_struct_rcu(st
>  {
>  	int oldusage;
>  
> +#ifdef __HAVE_ARCH_CMPXCHG
>  	do {
>  		oldusage = atomic_read(&t->usage);
>  		if (oldusage == 0) {
>  			return 0;
>  		}
>  	} while (cmpxchg(&t->usage.counter,
> -		 oldusage, oldusage + 1) != oldusage);
> +				oldusage, oldusage + 1) != oldusage);
> +#else
> +	oldusage = atomic_read(&t->usage);
> +	if (oldusage == 0) {
> +		return 0;
> +	}

What if t->usage becomes 0 here?

> +	atomic_inc(&t->usage);
> +#endif
>  	return 1;
>  }
>  

You need my atomic_cmpxchg patches that provide an atomic_cmpxchg
(and atomic_inc_not_zero) for all architectures.

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Daniel Walker]

On Thu, 2005-09-22 at 10:59 +1000, Nick Piggin wrote:

> You need my atomic_cmpxchg patches that provide an atomic_cmpxchg
> (and atomic_inc_not_zero) for all architectures.
> 

It is racy, but why not just disable preemption ..

Daniel

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Nick Piggin]

Daniel Walker wrote:
> On Thu, 2005-09-22 at 10:59 +1000, Nick Piggin wrote:
> 
> 
>>You need my atomic_cmpxchg patches that provide an atomic_cmpxchg
>>(and atomic_inc_not_zero) for all architectures.
>>
> 
> 
> It is racy, but why not just disable preemption ..
> 

Well you haven't, that's the point. You *introduce* racy operation.

Anyway, no matter, atomic_cmpxchg will take care of this nicely.

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Daniel Walker]

Checks for cmpxchg in get_task_struct_rcu() . No race version.

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

Index: linux-2.6.13/include/linux/sched.h
===================================================================
--- linux-2.6.13.orig/include/linux/sched.h
+++ linux-2.6.13/include/linux/sched.h
@@ -1026,13 +1026,24 @@ static inline int get_task_struct_rcu(st
 {
 	int oldusage;
 
+#ifdef __HAVE_ARCH_CMPXCHG
 	do {
 		oldusage = atomic_read(&t->usage);
 		if (oldusage == 0) {
 			return 0;
 		}
 	} while (cmpxchg(&t->usage.counter,
-		 oldusage, oldusage + 1) != oldusage);
+				oldusage, oldusage + 1) != oldusage);
+#else
+	raw_local_irq_disable();
+	oldusage = atomic_read(&t->usage);
+	if (oldusage == 0) {
+		raw_local_irq_enable();
+		return 0;
+	}
+	atomic_inc(&t->usage);
+	raw_local_irq_enable();
+#endif
 	return 1;
 }
 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Nick Piggin]

Daniel Walker wrote:

> +#else
> +	raw_local_irq_disable();
> +	oldusage = atomic_read(&t->usage);
> +	if (oldusage == 0) {
> +		raw_local_irq_enable();
> +		return 0;
> +	}

Isn't this still racy if another cpu drops the last reference
to task struct here? Or can't that happen?

When you said "disable preempt", I had some silly idea that this
function was only used in PREEMPT_RT mode, and that you would
simply disallow architectures without cmpxchg from configuring
PREEMPT_RT.

> +	atomic_inc(&t->usage);
> +	raw_local_irq_enable();
> +#endif

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Ingo Molnar]

* Daniel Walker <dwalker@mvista.com> wrote:

> Checks for cmpxchg in get_task_struct_rcu() . No race version.

shouldnt we actually define a global, default cmpxchg() function, based 
on IRQ-disable - instead of open-coding one?

	Ingo

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Ingo Molnar]

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> You need my atomic_cmpxchg patches that provide an atomic_cmpxchg (and 
> atomic_inc_not_zero) for all architectures.

yeah. When will they be merged upstream?

	Ingo

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Nick Piggin]

On Mon, 2005-09-26 at 08:26 +0200, Ingo Molnar wrote:
> * Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> > You need my atomic_cmpxchg patches that provide an atomic_cmpxchg (and 
> > atomic_inc_not_zero) for all architectures.
> 
> yeah. When will they be merged upstream?
> 

Well they're in -mm now, you can put them in your RT tree until they're
in mainline... I guess realistically, 2.6.15. They should blow up fairly
quickly if there are any problems with them, but they simply need a bit
of testing on all architectures which I cannot do and I suspect even -mm
isn't tested on at least half of them.


-- 
SUSE Labs, Novell Inc.



Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Ingo Molnar]

* Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Mon, 2005-09-26 at 08:26 +0200, Ingo Molnar wrote:
> > * Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > 
> > > You need my atomic_cmpxchg patches that provide an atomic_cmpxchg (and 
> > > atomic_inc_not_zero) for all architectures.
> > 
> > yeah. When will they be merged upstream?
> > 
> 
> Well they're in -mm now, you can put them in your RT tree until 
> they're in mainline... I guess realistically, 2.6.15. They should blow 
> up fairly quickly if there are any problems with them, but they simply 
> need a bit of testing on all architectures which I cannot do and I 
> suspect even -mm isn't tested on at least half of them.

very good - i'll put them into -rt.

	Ingo

Re: [PATCH] RT: Checks for cmpxchg in get_task_struct_rcu()

[Daniel Walker]

On Mon, 2005-09-26 at 08:25 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > Checks for cmpxchg in get_task_struct_rcu() . No race version.
> 
> shouldnt we actually define a global, default cmpxchg() function, based 
> on IRQ-disable - instead of open-coding one?

I was thinking it should be restructures so it just needs an atomic_inc
in this case. Considering that without cmpxchg() you must be on a UP
machine .

Daniel