[PATCH RT] convert RCU Preempt tasklet into softirq.

[PATCH RT] convert RCU Preempt tasklet into softirq.

[Steven Rostedt]

Following Dipankar's lead, I converted the tasklet in rcupreempt.c into
a softirq.

I've compiled and booted with this patch, but ran no other tests.

Paul,  I'm disappointed, this was so trivial I didn't get a chance to
learn anything ;-)


Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux-2.6.21-rt9/include/linux/rcupreempt.h
===================================================================
--- linux-2.6.21-rt9.orig/include/linux/rcupreempt.h
+++ linux-2.6.21-rt9/include/linux/rcupreempt.h
@@ -63,7 +63,9 @@ extern void rcu_check_callbacks(int cpu,
 extern void rcu_restart_cpu(int cpu);
 extern long rcu_batches_completed(void);
 
-extern void rcu_process_callbacks(unsigned long unused);
+struct softirq_action;
+
+extern void rcu_process_callbacks(struct softirq_action *unused);
 
 #endif /* __KERNEL__ */
 #endif /* __LINUX_RCUPREEMPT_H */
Index: linux-2.6.21-rt9/kernel/rcupreempt.c
===================================================================
--- linux-2.6.21-rt9.orig/kernel/rcupreempt.c
+++ linux-2.6.21-rt9/kernel/rcupreempt.c
@@ -57,7 +57,6 @@
 struct rcu_data {
 	raw_spinlock_t	lock;
 	long		completed;	/* Number of last completed batch. */
-	struct tasklet_struct rcu_tasklet;
 	struct rcu_head *nextlist;
 	struct rcu_head **nexttail;
 	struct rcu_head *waitlist;
@@ -255,7 +254,7 @@ void rcu_check_callbacks(int cpu, int us
 		spin_unlock_irqrestore(&rcu_data.lock, oldirq);
 	} else {
 		spin_unlock_irqrestore(&rcu_data.lock, oldirq);
-		tasklet_schedule(&rcu_data.rcu_tasklet);
+		raise_softirq(RCU_SOFTIRQ);
 	}
 }
 
@@ -279,7 +278,7 @@ void rcu_advance_callbacks(int cpu, int 
 	spin_unlock_irqrestore(&rcu_data.lock, oldirq);
 }
 
-void rcu_process_callbacks(unsigned long unused)
+void rcu_process_callbacks(struct softirq_action *unused)
 {
 	unsigned long flags;
 	struct rcu_head *next, *list;
@@ -367,7 +366,7 @@ void __init __rcu_init(void)
 	rcu_data.waittail = &rcu_data.waitlist;
 	rcu_data.donelist = NULL;
 	rcu_data.donetail = &rcu_data.donelist;
-	tasklet_init(&rcu_data.rcu_tasklet, rcu_process_callbacks, 0UL);
+	open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
 }
 
 /*

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Steven Rostedt]

On Thu, 2007-06-07 at 14:26 -0400, Steven Rostedt wrote:
> Following Dipankar's lead, I converted the tasklet in rcupreempt.c into
> a softirq.
> 
> I've compiled and booted with this patch, but ran no other tests.


There might still be an issue here. With the patch I'm getting a really
slow response time on networking. But that be because of other patches I
have applied.

I'll remove those other patches and see if this is still an issue.

/me booting with noapic to avoid the IO APIC issues.

-- Steve

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Steven Rostedt]

On Thu, 2007-06-07 at 14:51 -0400, Steven Rostedt wrote:

> There might still be an issue here. With the patch I'm getting a really
> slow response time on networking. But that be because of other patches I
> have applied.

I removed this patch and I can still get the network slowdown/hang. So
this patch is unrelated to this issue. RCU on the otherhand has not been
cleared of suspicion.

-- Steve

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Thu, Jun 07, 2007 at 02:26:59PM -0400, Steven Rostedt wrote:
> Following Dipankar's lead, I converted the tasklet in rcupreempt.c into
> a softirq.
> 
> I've compiled and booted with this patch, but ran no other tests.

I should be able to run rcutorture on it.

> Paul,  I'm disappointed, this was so trivial I didn't get a chance to
> learn anything ;-)

;-)

Some nits below.

						Thanx, Paul

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Index: linux-2.6.21-rt9/include/linux/rcupreempt.h
> ===================================================================
> --- linux-2.6.21-rt9.orig/include/linux/rcupreempt.h
> +++ linux-2.6.21-rt9/include/linux/rcupreempt.h
> @@ -63,7 +63,9 @@ extern void rcu_check_callbacks(int cpu,
>  extern void rcu_restart_cpu(int cpu);
>  extern long rcu_batches_completed(void);
> 
> -extern void rcu_process_callbacks(unsigned long unused);
> +struct softirq_action;
> +
> +extern void rcu_process_callbacks(struct softirq_action *unused);

I don't understand why the above is needed -- interrupt.h is included,
and the use of rcu_process_callbacks() follows the definition.

>  #endif /* __KERNEL__ */
>  #endif /* __LINUX_RCUPREEMPT_H */
> Index: linux-2.6.21-rt9/kernel/rcupreempt.c
> ===================================================================
> --- linux-2.6.21-rt9.orig/kernel/rcupreempt.c
> +++ linux-2.6.21-rt9/kernel/rcupreempt.c
> @@ -57,7 +57,6 @@
>  struct rcu_data {
>  	raw_spinlock_t	lock;
>  	long		completed;	/* Number of last completed batch. */
> -	struct tasklet_struct rcu_tasklet;
>  	struct rcu_head *nextlist;
>  	struct rcu_head **nexttail;
>  	struct rcu_head *waitlist;
> @@ -255,7 +254,7 @@ void rcu_check_callbacks(int cpu, int us
>  		spin_unlock_irqrestore(&rcu_data.lock, oldirq);
>  	} else {
>  		spin_unlock_irqrestore(&rcu_data.lock, oldirq);
> -		tasklet_schedule(&rcu_data.rcu_tasklet);
> +		raise_softirq(RCU_SOFTIRQ);
>  	}
>  }
> 
> @@ -279,7 +278,7 @@ void rcu_advance_callbacks(int cpu, int 
>  	spin_unlock_irqrestore(&rcu_data.lock, oldirq);
>  }
> 
> -void rcu_process_callbacks(unsigned long unused)
> +void rcu_process_callbacks(struct softirq_action *unused)
>  {
>  	unsigned long flags;
>  	struct rcu_head *next, *list;
> @@ -367,7 +366,7 @@ void __init __rcu_init(void)
>  	rcu_data.waittail = &rcu_data.waitlist;
>  	rcu_data.donelist = NULL;
>  	rcu_data.donetail = &rcu_data.donelist;
> -	tasklet_init(&rcu_data.rcu_tasklet, rcu_process_callbacks, 0UL);
> +	open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
>  }
> 
>  /*
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Thu, Jun 07, 2007 at 04:16:09PM -0400, Steven Rostedt wrote:
> On Thu, 2007-06-07 at 14:51 -0400, Steven Rostedt wrote:
> 
> > There might still be an issue here. With the patch I'm getting a really
> > slow response time on networking. But that be because of other patches I
> > have applied.
> 
> I removed this patch and I can still get the network slowdown/hang. So
> this patch is unrelated to this issue. RCU on the otherhand has not been
> cleared of suspicion.

Might the slowdown be due to different kernel threads running at different
priorities?  I could easily believe that changing the priority of the
kernel thread processing RCU callbacks could have a noticeable effect
on performance in some cases.

In other news, passed a set of kernbenches at this end, so starting
an rcutorture.

							Thanx, Paul

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Thu, Jun 07, 2007 at 09:16:51PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 07, 2007 at 04:16:09PM -0400, Steven Rostedt wrote:
> > On Thu, 2007-06-07 at 14:51 -0400, Steven Rostedt wrote:
> > 
> > > There might still be an issue here. With the patch I'm getting a really
> > > slow response time on networking. But that be because of other patches I
> > > have applied.
> > 
> > I removed this patch and I can still get the network slowdown/hang. So
> > this patch is unrelated to this issue. RCU on the otherhand has not been
> > cleared of suspicion.
> 
> Might the slowdown be due to different kernel threads running at different
> priorities?  I could easily believe that changing the priority of the
> kernel thread processing RCU callbacks could have a noticeable effect
> on performance in some cases.
> 
> In other news, passed a set of kernbenches at this end, so starting
> an rcutorture.

And rcutorture passed 8 hours on a 4-CPU Opteron box.

							Thanx, Paul

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Steven Rostedt]

On Thu, 2007-06-07 at 14:02 -0700, Paul E. McKenney wrote:

> Some nits below.
> 
> 						Thanx, Paul
> 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Index: linux-2.6.21-rt9/include/linux/rcupreempt.h
> > ===================================================================
> > --- linux-2.6.21-rt9.orig/include/linux/rcupreempt.h
> > +++ linux-2.6.21-rt9/include/linux/rcupreempt.h
> > @@ -63,7 +63,9 @@ extern void rcu_check_callbacks(int cpu,
> >  extern void rcu_restart_cpu(int cpu);
> >  extern long rcu_batches_completed(void);
> > 
> > -extern void rcu_process_callbacks(unsigned long unused);
> > +struct softirq_action;
> > +
> > +extern void rcu_process_callbacks(struct softirq_action *unused);
> 
> I don't understand why the above is needed -- interrupt.h is included,
> and the use of rcu_process_callbacks() follows the definition.
> 
> >  #endif /* __KERNEL__ */
> >  #endif /* __LINUX_RCUPREEMPT_H */

The first time I compiled it, I forgot the ';' and got a warning there.
But the warning also included "declaring structure softirq_action in
prototype", so I fixed both the ';' and added the struct. I can try
compile without it. But I also know that adding #include <interrupt.h>
in rcupreempt.h caused issues too.

-- Steve

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Fri, Jun 08, 2007 at 11:27:08AM -0400, Steven Rostedt wrote:
> On Thu, 2007-06-07 at 14:02 -0700, Paul E. McKenney wrote:
> 
> > Some nits below.
> > 
> > 						Thanx, Paul
> > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > Index: linux-2.6.21-rt9/include/linux/rcupreempt.h
> > > ===================================================================
> > > --- linux-2.6.21-rt9.orig/include/linux/rcupreempt.h
> > > +++ linux-2.6.21-rt9/include/linux/rcupreempt.h
> > > @@ -63,7 +63,9 @@ extern void rcu_check_callbacks(int cpu,
> > >  extern void rcu_restart_cpu(int cpu);
> > >  extern long rcu_batches_completed(void);
> > > 
> > > -extern void rcu_process_callbacks(unsigned long unused);
> > > +struct softirq_action;
> > > +
> > > +extern void rcu_process_callbacks(struct softirq_action *unused);
> > 
> > I don't understand why the above is needed -- interrupt.h is included,
> > and the use of rcu_process_callbacks() follows the definition.
> > 
> > >  #endif /* __KERNEL__ */
> > >  #endif /* __LINUX_RCUPREEMPT_H */
> 
> The first time I compiled it, I forgot the ';' and got a warning there.
> But the warning also included "declaring structure softirq_action in
> prototype", so I fixed both the ';' and added the struct. I can try
> compile without it. But I also know that adding #include <interrupt.h>
> in rcupreempt.h caused issues too.

If I leave out both the "struct softirq_action" and the
rcu_process_callbacks() declaration,, it compiles for me.

So I guess the rcu_process_callbacks() should be declared static...

						Thanx, Paul

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Steven Rostedt]

On Fri, 2007-06-08 at 12:36 -0700, Paul E. McKenney wrote:
> On Fri, Jun 08, 2007 at 11:27:08AM -0400, Steven Rostedt wrote:

> > The first time I compiled it, I forgot the ';' and got a warning there.
> > But the warning also included "declaring structure softirq_action in
> > prototype", so I fixed both the ';' and added the struct. I can try
> > compile without it. But I also know that adding #include <interrupt.h>
> > in rcupreempt.h caused issues too.
> 
> If I leave out both the "struct softirq_action" and the
> rcu_process_callbacks() declaration,, it compiles for me.
> 
> So I guess the rcu_process_callbacks() should be declared static...

OK, I can update the patch to reflect that. Remember, I didn't learn
anything from doing this patch, so I have no idea why
rcu_procell_callbacks was global. I was just keeping to the norm. :-)

Actually, I'll make a separate patch for this change. This is a
different issue.

-- Steve

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Fri, Jun 08, 2007 at 03:43:48PM -0400, Steven Rostedt wrote:
> On Fri, 2007-06-08 at 12:36 -0700, Paul E. McKenney wrote:
> > On Fri, Jun 08, 2007 at 11:27:08AM -0400, Steven Rostedt wrote:
> 
> > > The first time I compiled it, I forgot the ';' and got a warning there.
> > > But the warning also included "declaring structure softirq_action in
> > > prototype", so I fixed both the ';' and added the struct. I can try
> > > compile without it. But I also know that adding #include <interrupt.h>
> > > in rcupreempt.h caused issues too.
> > 
> > If I leave out both the "struct softirq_action" and the
> > rcu_process_callbacks() declaration,, it compiles for me.
> > 
> > So I guess the rcu_process_callbacks() should be declared static...
> 
> OK, I can update the patch to reflect that. Remember, I didn't learn
> anything from doing this patch, so I have no idea why
> rcu_procell_callbacks was global. I was just keeping to the norm. :-)

Hey, -I- learned something from your doing the patch -- namely that
rcu_process_callbacks() was needlessly non-static.  ;-)

> Actually, I'll make a separate patch for this change. This is a
> different issue.

Sounds good!

						Thanx, Paul

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Clark Williams]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Paul E. McKenney wrote:
> On Fri, Jun 08, 2007 at 03:43:48PM -0400, Steven Rostedt wrote:
>> On Fri, 2007-06-08 at 12:36 -0700, Paul E. McKenney wrote:
>>> On Fri, Jun 08, 2007 at 11:27:08AM -0400, Steven Rostedt wrote:
>>>> The first time I compiled it, I forgot the ';' and got a warning there.
>>>> But the warning also included "declaring structure softirq_action in
>>>> prototype", so I fixed both the ';' and added the struct. I can try
>>>> compile without it. But I also know that adding #include <interrupt.h>
>>>> in rcupreempt.h caused issues too.
>>> If I leave out both the "struct softirq_action" and the
>>> rcu_process_callbacks() declaration,, it compiles for me.
>>>
>>> So I guess the rcu_process_callbacks() should be declared static...
>> OK, I can update the patch to reflect that. Remember, I didn't learn
>> anything from doing this patch, so I have no idea why
>> rcu_procell_callbacks was global. I was just keeping to the norm. :-)
> 
> Hey, -I- learned something from your doing the patch -- namely that
> rcu_process_callbacks() was needlessly non-static.  ;-)
> 
>> Actually, I'll make a separate patch for this change. This is a
>> different issue.
> 
> Sounds good!
> 

Paul,

I had a test run of a kernel using Steven's patch (RCU using tasklets) going over the
weekend. It looks like it made it through running racer for 24hrs without a panic,
but I'm not entirely convinced (since my reservation of the test system expired on
Saturday and I didn't look at it until Wednesday; bad Clark, no doughnut).

I've got another test running now and I'll be able to poke around on the system
tomorrow morning to see if in fact there were no RCU related Oops'en. I'll let you
know what we find.

Clark


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFG4D5AHyuj/+TTEp0RAkfxAKDmGHLgTEaVrclCPQfytJZbF9hf3QCfZdct
Hjr1PgUP6U8X4dLAdd3vXQk=
=T7lw
-----END PGP SIGNATURE-----

Re: [PATCH RT] convert RCU Preempt tasklet into softirq.

[Paul E. McKenney]

On Thu, Sep 06, 2007 at 12:52:00PM -0500, Clark Williams wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Paul E. McKenney wrote:
> > On Fri, Jun 08, 2007 at 03:43:48PM -0400, Steven Rostedt wrote:
> >> On Fri, 2007-06-08 at 12:36 -0700, Paul E. McKenney wrote:
> >>> On Fri, Jun 08, 2007 at 11:27:08AM -0400, Steven Rostedt wrote:
> >>>> The first time I compiled it, I forgot the ';' and got a warning there.
> >>>> But the warning also included "declaring structure softirq_action in
> >>>> prototype", so I fixed both the ';' and added the struct. I can try
> >>>> compile without it. But I also know that adding #include <interrupt.h>
> >>>> in rcupreempt.h caused issues too.
> >>> If I leave out both the "struct softirq_action" and the
> >>> rcu_process_callbacks() declaration,, it compiles for me.
> >>>
> >>> So I guess the rcu_process_callbacks() should be declared static...
> >> OK, I can update the patch to reflect that. Remember, I didn't learn
> >> anything from doing this patch, so I have no idea why
> >> rcu_procell_callbacks was global. I was just keeping to the norm. :-)
> > 
> > Hey, -I- learned something from your doing the patch -- namely that
> > rcu_process_callbacks() was needlessly non-static.  ;-)
> > 
> >> Actually, I'll make a separate patch for this change. This is a
> >> different issue.
> > 
> > Sounds good!
> > 
> 
> Paul,
> 
> I had a test run of a kernel using Steven's patch (RCU using tasklets) going over the
> weekend. It looks like it made it through running racer for 24hrs without a panic,
> but I'm not entirely convinced (since my reservation of the test system expired on
> Saturday and I didn't look at it until Wednesday; bad Clark, no doughnut).
> 
> I've got another test running now and I'll be able to poke around on the system
> tomorrow morning to see if in fact there were no RCU related Oops'en. I'll let you
> know what we find.

Thank you for the info, Clark!

Of course, this all begs the question of why the heck switching from
tasklets to softirqs should make any difference at all...

							Thanx, Paul