possible bug in net/core/pktgen.c (2.6.10 kernel)

possible bug in net/core/pktgen.c (2.6.10 kernel)

[Dave Peterson]

Hi,

I found a piece of code that looks problematic in the 2.6.10 kernel.
The following code appears starting on line 746 in function inject()
of net/core/pktgen.c:

                if (netif_queue_stopped(odev) || need_resched()) {
                        u32 idle_start, idle;

                        idle_start = cycles();
                        do {
                                if (signal_pending(current)) {
                                        info->do_run_run = 0;
                                        break;
                                }
                                if (!netif_running(odev)) {
                                        info->do_run_run = 0;
                                        break;
                                }
                                if (need_resched())
                                        schedule();
                                else
                                        do_softirq();
                        } while (netif_queue_stopped(odev));
                        idle = cycles() - idle_start;
                        info->idle_acc += idle;
                }

Notice the call to do_softirq() above.  At this point it appears that
preemption is enabled.  However, do_softirq() is written under the
assumption that it will be called with preemption disabled.  It looks
like the code was originally written for the 2.4 kernel and this was
missed when kernel preemption was added in 2.5.  Is this in fact a bug
or am I missing something?  I haven't attempted to make a patch that
fixes this because I'm not too familiar with the networking code and
I'd rather let someone else determine what is the best way to fix it.


Dave Peterson
dsp@llnl.gov

possible bug in net/core/pktgen.c (2.6.10 kernel)

[Robert Olsson]

Dave Peterson writes:

 > I found a piece of code that looks problematic in the 2.6.10 kernel.
 > The following code appears starting on line 746 in function inject()
 > of net/core/pktgen.c:
 >                                         schedule();
 >                                 else
 >                                         do_softirq();


 Thanks! 
 So it should be?

--- net/core/pktgen.c.orig	2005-01-16 13:39:10.933427120 +0100
+++ net/core/pktgen.c	2005-01-16 13:40:41.926751672 +0100
@@ -753,8 +753,11 @@
 				}
 				if (need_resched())
 					schedule();
-				else
+				else {
+					preempt_disable();
 					do_softirq();
+					preempt_enable();
+				}
 			} while (netif_queue_stopped(odev));
 			idle = cycles() - idle_start;
 			info->idle_acc += idle;



						--ro

Re: possible bug in net/core/pktgen.c (2.6.10 kernel)

[Dave Peterson]

On Sunday 16 January 2005 04:46 am, Robert Olsson wrote:
> Dave Peterson writes:
>  > I found a piece of code that looks problematic in the 2.6.10 kernel.
>  > The following code appears starting on line 746 in function inject()
>  > of net/core/pktgen.c:
>  >                                         schedule();
>  >                                 else
>  >                                         do_softirq();
>
>  Thanks!
>  So it should be?

Cool!  Looks like a fix to me.

> --- net/core/pktgen.c.orig	2005-01-16 13:39:10.933427120 +0100
> +++ net/core/pktgen.c	2005-01-16 13:40:41.926751672 +0100
> @@ -753,8 +753,11 @@
>  				}
>  				if (need_resched())
>  					schedule();
> -				else
> +				else {
> +					preempt_disable();
>  					do_softirq();
> +					preempt_enable();
> +				}
>  			} while (netif_queue_stopped(odev));
>  			idle = cycles() - idle_start;
>  			info->idle_acc += idle;
>
>
>
> 						--ro

Re: possible bug in net/core/pktgen.c (2.6.10 kernel)

[David S. Miller]

On Tue, 18 Jan 2005 09:35:25 -0800
Dave Peterson <dsp@llnl.gov> wrote:

> On Sunday 16 January 2005 04:46 am, Robert Olsson wrote:
> > Dave Peterson writes:
> >  > I found a piece of code that looks problematic in the 2.6.10 kernel.
> >  > The following code appears starting on line 746 in function inject()
> >  > of net/core/pktgen.c:
> >  >                                         schedule();
> >  >                                 else
> >  >                                         do_softirq();
> >
> >  Thanks!
> >  So it should be?
> 
> Cool!  Looks like a fix to me.

I'm still a little bit confused on this one.

Since when does do_softirq() need preemption disabled
around calls to it?

do_softirq() disabled hard IRQs during the duration of it's
execution, thus effectively disabling preemption.

What is the problematic case again?

Re: possible bug in net/core/pktgen.c (2.6.10 kernel)

[Dave Peterson]

On Tuesday 18 January 2005 12:41 pm, David S. Miller wrote:
> I'm still a little bit confused on this one.
>
> Since when does do_softirq() need preemption disabled
> around calls to it?
>
> do_softirq() disabled hard IRQs during the duration of it's
> execution, thus effectively disabling preemption.
>
> What is the problematic case again?

Oops... My mistake.  Looking at __do_softirq() I noticed that it enables
interrupts before executing the softirqs.  However I didn't notice that
the call to local_bh_disable() in __do_softirq() disables preemption
before interrupts are enabled.  On second thought everything looks OK
and no bug fix is needed.


Dave

Re: possible bug in net/core/pktgen.c (2.6.10 kernel)

[David S. Miller]

On Tue, 18 Jan 2005 13:46:22 -0800
Dave Peterson <dsp@llnl.gov> wrote:

> > Since when does do_softirq() need preemption disabled
> > around calls to it?
> >
> > do_softirq() disabled hard IRQs during the duration of it's
> > execution, thus effectively disabling preemption.
> >
> > What is the problematic case again?
> 
> Oops... My mistake.  Looking at __do_softirq() I noticed that it enables
> interrupts before executing the softirqs.  However I didn't notice that
> the call to local_bh_disable() in __do_softirq() disables preemption
> before interrupts are enabled.  On second thought everything looks OK
> and no bug fix is needed.

Great, looks like things are OK then.