Strange code in cpu_idle()

Hello!

Strange code in i386, ia64, and x86-64 cpu_idle():

void cpu_idle (void)
{
/* endless idle loop with no priority at all */

  while (1) {
   while (!need_resched()) {
    void (*idle)(void);
    /*
     * Mark this as an RCU critical section so that
     * synchronize_kernel() in the unload path waits
     * for our completion.
     */
    rcu_read_lock();
    idle = pm_idle;
    if (!idle)
     idle = default_idle;
    idle();
    rcu_read_unlock();
   }
   schedule();

}
}

Unless idle_cpu() is busted, it seems like the above is, given the code in rcu_check_callbacks():

void rcu_check_callbacks(int cpu, int user)
{
if (user ||

      (idle_cpu(cpu) && !in_softirq() && 
     hardirq_count() <= (1 << HARDIRQ_SHIFT))) {
   rcu_qsctr_inc(cpu);
   rcu_bh_qsctr_inc(cpu);

} else if (!in_softirq())
rcu_bh_qsctr_inc(cpu);
tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
}

And idle_cpu() is pretty straightforward:

	int idle_cpu(int cpu)
	{
		return cpu_curr(cpu) == cpu_rq(cpu)->idle;
	}

So I would say that the rcu_read_lock() in cpu_idle() is having no effect, because any timer interrupt from cpu_idle() will mark a quiescent state notwithstanding. What am I missing here?

If I am not missing anything, then the attached patch would be in order here, though there might be some additional work required. (Though I thought that the try_stop_module() stuff took care of all of this these days...)

Note that we really, really do want the idle loop to be an extended quiescent state, otherwise one gets indefinite grace periods and runs out of memory...

Thanx, Paul

diff -urpN -X ../dontdiff linux-2.5/arch/i386/kernel/process.c linux-2.5-idle_rcu/arch/i386/kernel/process.c

arch/i386/kernel/process.c +4 -7

@@ -144,14 +144,12 @@ void cpu_idle (void)
 {
 	/* endless idle loop with no priority at all */
 	while (1) {
+		/*
+		 * Note that it is illegal to use RCU read-side
+		 * critical sections within the idle loop.
+		 */
 		while (!need_resched()) {
 			void (*idle)(void);
-			/*
-			 * Mark this as an RCU critical section so that
-			 * synchronize_kernel() in the unload path waits
-			 * for our completion.
-			 */
-			rcu_read_lock();
 			idle = pm_idle;
 
 			if (!idle)

@@ -159,7 +157,6 @@ void cpu_idle (void)
 
 			irq_stat[smp_processor_id()].idle_timestamp = jiffies;
 			idle();
-			rcu_read_unlock();
 		}
 		schedule();
 	}

diff -urpN -X ../dontdiff linux-2.5/arch/ia64/kernel/process.c linux-2.5-idle_rcu/arch/ia64/kernel/process.c

arch/ia64/kernel/process.c +4 -7

@@ -230,6 +230,10 @@ cpu_idle (void *unused)
 
 	/* endless idle loop with no priority at all */
 	while (1) {
+		/*
+		 * Note that it is illegal to use RCU read-side
+		 * critical sections within the idle loop.
+		 */
 #ifdef CONFIG_SMP
 		if (!need_resched())
 			min_xtp();

@@ -239,17 +243,10 @@ cpu_idle (void *unused)
 
 			if (mark_idle)
 				(*mark_idle)(1);
-			/*
-			 * Mark this as an RCU critical section so that
-			 * synchronize_kernel() in the unload path waits
-			 * for our completion.
-			 */
-			rcu_read_lock();
 			idle = pm_idle;
 			if (!idle)
 				idle = default_idle;
 			(*idle)();
-			rcu_read_unlock();
 		}
 
 		if (mark_idle)

diff -urpN -X ../dontdiff linux-2.5/arch/x86_64/kernel/process.c linux-2.5-idle_rcu/arch/x86_64/kernel/process.c

arch/x86_64/kernel/process.c +4 -8

@@ -133,19 +133,16 @@ void cpu_idle (void)
 {
 	/* endless idle loop with no priority at all */
 	while (1) {
+		/*
+		 * Note that it is illegal to use RCU read-side
+		 * critical sections within the idle loop.
+		 */
 		while (!need_resched()) {
 			void (*idle)(void);
-			/*
-			 * Mark this as an RCU critical section so that
-			 * synchronize_kernel() in the unload path waits
-			 * for our completion.
-			 */
-			rcu_read_lock();
 			idle = pm_idle;
 			if (!idle)
 				idle = default_idle;
 			idle();
-			rcu_read_unlock();
 		}
 		schedule();
 	}
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Paul,

Well, your mail is _very_ interresting for me.

I get this oops for weeks with several kernel versions now:

Unable to handle kernel paging request at virtual address 00099108
 printing eip:
b01010c0
*pde = 00000000
Oops: 0000 [#1]
SMP 
Modules linked in: nfs lockd sunrpc nvidia ipv6 ohci_hcd tuner tvaudio msp3400 bttv video_buf firmware_class btcx_risc ehci_hcd uhci_hcd usbcore intel_agp agpgart evdev
CPU:    0
EIP:    0060:[<b01010c0>]    Tainted: P      VLI
EFLAGS: 00010286   (2.6.10-rc2-ck2-nozeroram-findvmastat)

The above oops shows a crash when we try to access the thread_info->flags field. This is done by need_resched() to check the TIF_NEED_RESCHED flag.
I don't know how to interpret the oops correctly to find the source of the crash.

Quoted 6 lines

Hi Paul,

Well, your mail is _very_ interresting for me.

I get this oops for weeks with several kernel versions now:

There shouldn't be any oopses with the changes in question, the original bugfix was for crashes whilst unloading ACPI processor or APM module.

Quoted 10 lines

Unable to handle kernel paging request at virtual address 00099108
 printing eip:
b01010c0
*pde = 00000000
Oops: 0000 [#1]
SMP 
Modules linked in: nfs lockd sunrpc nvidia ipv6 ohci_hcd tuner tvaudio msp3400 bttv video_buf firmware_class btcx_risc ehci_hcd uhci_hcd usbcore intel_agp agpgart evdev
CPU:    0
EIP:    0060:[<b01010c0>]    Tainted: P      VLI
EFLAGS: 00010286   (2.6.10-rc2-ck2-nozeroram-findvmastat)

It would be a lot easier to debug with a vanilla kernel and no nvidia loaded.

Quoted 5 lines

The above oops shows a crash when we try to access the thread_info->flags field. This is done by need_resched() to check the TIF_NEED_RESCHED flag.
I don't know how to interpret the oops correctly to find the source of the crash.

Looks like thread_info was. Wow i think you're in a different world of hurt. If you can reproduce with less patches and no nvidia please send over the bugreport.

Thanks,
Zwane

Unless idle_cpu() is busted, it seems like the above is, given the code in rcu_check_callbacks():

	void rcu_check_callbacks(int cpu, int user)
	{
		if (user || 
		    (idle_cpu(cpu) && !in_softirq() && 
					hardirq_count() <= (1 << HARDIRQ_SHIFT))) {
			rcu_qsctr_inc(cpu);
			rcu_bh_qsctr_inc(cpu);
		} else if (!in_softirq())
			rcu_bh_qsctr_inc(cpu);
		tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
	}

And idle_cpu() is pretty straightforward:

	int idle_cpu(int cpu)
	{
		return cpu_curr(cpu) == cpu_rq(cpu)->idle;
	}

So I would say that the rcu_read_lock() in cpu_idle() is having no effect, because any timer interrupt from cpu_idle() will mark a quiescent state notwithstanding. What am I missing here?

Note that we really, really do want the idle loop to be an extended quiescent state, otherwise one gets indefinite grace periods and runs out of memory...

Hello Paul,

Quoted 15 lines · Paul E. McKenney

Unless idle_cpu() is busted, it seems like the above is, given the code in rcu_check_callbacks():

	void rcu_check_callbacks(int cpu, int user)
	{
		if (user || 
		    (idle_cpu(cpu) && !in_softirq() && 
					hardirq_count() <= (1 << HARDIRQ_SHIFT))) {
			rcu_qsctr_inc(cpu);
			rcu_bh_qsctr_inc(cpu);
		} else if (!in_softirq())
			rcu_bh_qsctr_inc(cpu);
		tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
	}

Quoted 3 lines

So I would say that the rcu_read_lock() in cpu_idle() is having no effect, because any timer interrupt from cpu_idle() will mark a quiescent state notwithstanding. What am I missing here?

What about the hardirq_count check since we're coming in from the timer interrupt?

Quoted 6 lines

Quoted 3 lines

So I would say that the rcu_read_lock() in cpu_idle() is having no effect, because any timer interrupt from cpu_idle() will mark a quiescent state notwithstanding. What am I missing here?

What about the hardirq_count check since we're coming in from the timer interrupt?

Look at the hardirq_count check closely - it only checks for reentrant hardirqs. If the idle task gets interrupted by a timer interrupt, the RCU quiscent state counter for the cpu will get incremented. So, rcu_read_lock() in cpu_idle() is bogus.