RCU: clean up a few remaining synchronize_kernel() calls

2.6.12-rc6-mm1 has a few remaining synchronize_kernel()s, some (but not all) in comments. This patch changes these synchronize_kernel() calls (and comments) to synchronize_rcu() or synchronize_sched() as follows:

o arch/x86_64/kernel/mce.c mce_read(): change to synchronize_sched()
to handle races with machine-check exceptions (synchronize_rcu() would not cut it given RCU implementations intended for hardcore realtime use.

o drivers/input/serio/i8042.c i8042_stop(): change to
synchronize_sched() to handle races with i8042_interrupt() interrupt handler. Again, synchronize_rcu() would not cut it given RCU implementations intended for hardcore realtime use.

o include/*/kdebug.h comments: change to synchronize_sched()
to handle races with NMIs. As before, synchronize_rcu() would not cut it...

o include/linux/list.h comment: change to synchronize_rcu(),
since this comment is for list_del_rcu().

o security/keys/key.c unregister_key_type(): change to
synchronize_rcu(), since this is interacting with RCU read side.

o security/keys/process_keys.c install_session_keyring():
change to synchronize_rcu(), since this is interacting with RCU read side.

Please let me know if there are any problems with any of these changes.

---

 arch/x86_64/kernel/mce.c     |    2 +-
 drivers/input/serio/i8042.c  |    2 +-
 include/asm-i386/kdebug.h    |    2 +-
 include/asm-ppc64/kdebug.h   |    2 +-
 include/asm-sparc64/kdebug.h |    2 +-
 include/asm-x86_64/kdebug.h  |    2 +-
 include/linux/list.h         |    2 +-
 security/keys/key.c          |    2 +-
 security/keys/process_keys.c |    2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/arch/x86_64/kernel/mce.c linux-2.6.12-rc6-mm1-sksr/arch/x86_64/kernel/mce.c
@@ -411,7 +411,7 @@ static ssize_t mce_read(struct file *fil
 	memset(mcelog.entry, 0, next * sizeof(struct mce));
 	mcelog.next = 0;
 
-	synchronize_kernel();	
+	synchronize_sched();	
 
 	/* Collect entries that were still getting written before the synchronize. */
 

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/drivers/input/serio/i8042.c linux-2.6.12-rc6-mm1-sksr/drivers/input/serio/i8042.c
@@ -396,7 +396,7 @@ static void i8042_stop(struct serio *ser
 	struct i8042_port *port = serio->port_data;
 
 	port->exists = 0;
-	synchronize_kernel();
+	synchronize_sched();
 	port->serio = NULL;
 }
 

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/include/asm-i386/kdebug.h linux-2.6.12-rc6-mm1-sksr/include/asm-i386/kdebug.h
@@ -18,7 +18,7 @@ struct die_args {
 };
 
 /* Note - you should never unregister because that can race with NMIs.
-   If you really want to do it first unregister - then synchronize_kernel - then free.
+   If you really want to do it first unregister - then synchronize_sched - then free.
   */
 int register_die_notifier(struct notifier_block *nb);
 extern struct notifier_block *i386die_chain;

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/include/asm-ppc64/kdebug.h linux-2.6.12-rc6-mm1-sksr/include/asm-ppc64/kdebug.h
@@ -17,7 +17,7 @@ struct die_args {
 
 /*
    Note - you should never unregister because that can race with NMIs.
-   If you really want to do it first unregister - then synchronize_kernel -
+   If you really want to do it first unregister - then synchronize_sched -
    then free.
  */
 int register_die_notifier(struct notifier_block *nb);

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/include/asm-sparc64/kdebug.h linux-2.6.12-rc6-mm1-sksr/include/asm-sparc64/kdebug.h
@@ -16,7 +16,7 @@ struct die_args {
 };
 
 /* Note - you should never unregister because that can race with NMIs.
- * If you really want to do it first unregister - then synchronize_kernel
+ * If you really want to do it first unregister - then synchronize_sched
  * - then free.
  */
 int register_die_notifier(struct notifier_block *nb);

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/include/asm-x86_64/kdebug.h linux-2.6.12-rc6-mm1-sksr/include/asm-x86_64/kdebug.h
@@ -14,7 +14,7 @@ struct die_args { 
 }; 
 
 /* Note - you should never unregister because that can race with NMIs.
-   If you really want to do it first unregister - then synchronize_kernel - then free. 
+   If you really want to do it first unregister - then synchronize_sched - then free. 
   */
 int register_die_notifier(struct notifier_block *nb);
 extern struct notifier_block *die_chain;

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/include/linux/list.h linux-2.6.12-rc6-mm1-sksr/include/linux/list.h
@@ -189,7 +189,7 @@ static inline void list_del(struct list_
  * list_for_each_entry_rcu().
  *
  * Note that the caller is not permitted to immediately free
- * the newly deleted entry.  Instead, either synchronize_kernel()
+ * the newly deleted entry.  Instead, either synchronize_rcu()
  * or call_rcu() must be used to defer freeing until an RCU
  * grace period has elapsed.
  */

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/security/keys/key.c linux-2.6.12-rc6-mm1-sksr/security/keys/key.c
@@ -980,7 +980,7 @@ void unregister_key_type(struct key_type
 	spin_unlock(&key_serial_lock);
 
 	/* make sure everyone revalidates their keys */
-	synchronize_kernel();
+	synchronize_rcu();
 
 	/* we should now be able to destroy the payloads of all the keys of
 	 * this type with impunity */

diff -urpN -X dontdiff linux-2.6.12-rc6-mm1/security/keys/process_keys.c linux-2.6.12-rc6-mm1-sksr/security/keys/process_keys.c
@@ -234,7 +234,7 @@ static int install_session_keyring(struc
 	ret = 0;
 
 	/* we're using RCU on the pointer */
-	synchronize_kernel();
+	synchronize_rcu();
 	key_put(old);
  error:
 	return ret;

Hi Paul,
Do you have any pending patches to update Documentation/RCU/* too?
The best i can find explaining usage is from;

http://lwn.net/Articles/130341/

Thanks,
Zwane

Hello, Zwane,

I did update Documentation/RCU/* to change the uses of synchronize_kernel(), but was relying on the documentation-extraction stuff to build the API documentation. So I have the following header comments in mainline:

/**
* synchronize_rcu - wait until a grace period has elapsed. *
* Control will return to the caller some time after a full grace
* period has elapsed, in other words after all currently executing RCU
* read-side critical sections have completed. RCU read-side critical
* sections are delimited by rcu_read_lock() and rcu_read_unlock(),
* and may be nested.
*
* If your read-side code is not protected by rcu_read_lock(), do -not-
* use synchronize_rcu().
*/

/**
* synchronize_sched - block until all CPUs have exited any non-preemptive
* kernel code sequences.
*
* This means that all preempt_disable code sequences, including NMI and
* hardware-interrupt handlers, in progress on entry will have completed
* before this primitive returns. However, this does not guarantee that
* softirq handlers will have completed, since in some kernels *
* This primitive provides the guarantees made by the (deprecated)
* synchronize_kernel() API. In contrast, synchronize_rcu() only
* guarantees that rcu_read_lock() sections will have completed. */

Hmmm... Looks like some repair is needed in the synchronize_sched() header comment -- and I vaguely remember someone pointing this out. :-/ See patch below for a fix.

Glancing through the Documentation/RCU/* stuff again, it could definitely use some help. Would it help if I added an example use of synchronize_sched() that interacted with NMIs?

Thanx, Paul

---

 rcupdate.h |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff -urpN -X dontdiff linux-2.6.12-rc6/include/linux/rcupdate.h linux-2.6.12-rc6-ssdoc/include/linux/rcupdate.h
@@ -260,10 +260,15 @@ static inline int rcu_pending(int cpu)
  * synchronize_sched - block until all CPUs have exited any non-preemptive
  * kernel code sequences.
  *
- * This means that all preempt_disable code sequences, including NMI and
- * hardware-interrupt handlers, in progress on entry will have completed
- * before this primitive returns.  However, this does not guarantee that
- * softirq handlers will have completed, since in some kernels
+ * This means that all preempt_disable code sequences in progress on entry
+ * will have completed before this primitive returns.  It also means that
+ * all NMIs and hardware interrupts pending on entry will have completed
+ * their handlers before exit, where "pending" means that the interrupt
+ * assertion has reached the CPU.  Note however that some kernel
+ * configurations and some patches cause interrupt handlers and softirqs
+ * to run in process context, sometimes preemptably.  In such kernels,
+ * only the beginning portion of the handler that runs with preemption
+ * disabled will be guaranteed to have completed.
  *
  * This primitive provides the guarantees made by the (deprecated)
  * synchronize_kernel() API.  In contrast, synchronize_rcu() only

Hi Paul,

Thanks that looks like it covers it.

Zwane

Hello, Zwane,

How does the following look for NMI-RCU documentation?

Thanx, Paul

------------------------------------------------------------------------

Using RCU to Protect Dynamic NMI Handlers

Although RCU is usually used to protect read-mostly data structures, it is possible to use RCU to provide dynamic non-maskable interrupt handlers, as well as dynamic irq handlers. This document describes how to do this, drawing loosely from Zwane Mwaikambo's NMI-timer work in "arch/i386/oprofile/nmi_timer_int.c" and in "arch/i386/kernel/traps.c".

The relevant pieces of code are listed below each followed by a brief explanation.

	static int dummy_nmi_callback(struct pt_regs * regs, int cpu)
	{
		return 0;
	}

This is a "dummy" NMI handler that does nothing, but returns zero, thus saying that it did nothing, allowing the NMI handler to take the default machine-specific action.

	static nmi_callback_t nmi_callback = dummy_nmi_callback;

This is a global function pointer to the current NMI handler.

	fastcall void do_nmi(struct pt_regs * regs, long error_code)
	{
		int cpu;

		nmi_enter();

		cpu = smp_processor_id();
		++nmi_count(cpu);

		if (!nmi_callback(regs, cpu))
			default_do_nmi(regs);

		nmi_exit();
	}

The do_nmi() function processes each NMI. It first disables preemption in the same way that a hardware irq would, then increments the per-CPU count of NMIs. It then invokes the NMI handler stored in the nmi_callback function pointer. If this handler returns zero, do_nmi() invokes the default_do_nmi() function to handle a machine-specific NMI. Finally, preemption is restored.

Purists might argue that the invocation of nmi_callback() should use rcu_dereference as follows:

if (!rcu_dereference(nmi_callback)(regs, cpu))

Strictly speaking, rcu_dereference() is not needed, since this code runs only on i386, which does not need rcu_dereference() anyway. However, it may be helpful as a documentation aid, particularly for anyone attempting to do something similar on Alpha.

Quick Quiz: Why might the rcu_dereference() be necessary on Alpha,

	     given that the code reference by the pointer is read-only?

	void set_nmi_callback(nmi_callback_t callback)
	{
		nmi_callback = callback;
	}

This function registers an NMI handler.

	void unset_nmi_callback(void)
	{
		nmi_callback = dummy_nmi_callback;
	}

This function unregisters an NMI handler, restoring the original dummy_nmi_handler(). However, there may well be an NMI handler currently executing on some other CPU. We therefore cannot free up any data structures used by the old NMI handler until execution of it completes on all other CPUs.

One way to accomplish this is via synchronize_sched(), perhaps as follows:

	unset_nmi_callback();
	synchronize_sched();
	kfree(my_nmi_data);

This works because synchronize_sched() blocks until all CPUs complete any preemption-disabled segments of code that they were executing. Since NMI handlers disable preemption, synchronize_sched() is guaranteed no to return until all ongoing NMI handlers exit. It is therefore safe to free up the handler's data as soon as synchronize_sched() returns.

Answer to Quick Quiz

Why might the rcu_dereference() be necessary on Alpha, given that the code reference by the pointer is read-only?

Answer: The caller to set_nmi_callback() might well have
initialized some data that is to be used by the new NMI handler. In this case, the rcu_dereference() would be needed, because otherwise a CPU that received an NMI just after the new handler was set might see the pointer to the new NMI handler, but the old pre-initialized version of the handler's data.

Hello Paul,

That looks good, there is just one bit i'm not entirely sure about and i'd appreciate it if you could entertain me for a bit;

Answer to Quick Quiz

Why might the rcu_dereference() be necessary on Alpha, given that the code reference by the pointer is read-only?

Answer: The caller to set_nmi_callback() might well have
initialized some data that is to be used by the new NMI handler. In this case, the rcu_dereference() would be needed, because otherwise a CPU that received an NMI just after the new handler was set might see the pointer to the new NMI handler, but the old pre-initialized version of the handler's data.

Reading that i would think the general programming model for this would be;

setup data
write barrier
setup callback

Isn't that still required considering the following scenario;

CPU0 CPU1
setup data <NMI>
setup callback ...
... call callback

on i386, interrupts are data synchronising events, however if we happen to take an interrupt right when the data is being setup we won't synchronise with respect to that data. This could be achieved via the explicit write barrier after data setup or rcu_dereference in the NMI handler. Or perhaps i'm missing something?

Thanks!
Zwane

I will see what entertainment I can provide. ;-)

Agreed! I have updated the document to make this requirement clear.

On i386, writes are ordered. So if CPU1 sees the callback, it is guaranteed to also see the data.

However, you do have a good point -- weakly ordered CPUs would need to have an explicit memory barrier. This might well already be taken care of by the memory barriers in the locking primitives used by the up() operation invoked at the end of oprofile_start(), but I did not check all the possible ways that these functions can be called.

Given that set_nmi_callback isn't invoked all that often, seems like it might be preferable to insert an smp_wmb() at the beginning of set_nmi_callback(), so that it reads as follows:

	void set_nmi_callback(nmi_callback_t callback)
	{
		smp_wmb();
		nmi_callback = callback;
	}

Thoughts?

Thanx, Paul

Yes indeed, i was certain that i missed something ;)

I agree, that usage looks safe.

Andrew (rightly) tends to howls whenever someone adds a memory barrier without a comment ;) So if we were to make that change, how about the following accompanying comment;

"smp_wmb ensures that all data dependencies for the callback are posted and callback is ready for execution"

Thanks for elaborating, the examples certainly do help clarify usage.
Zwane

Cool!

Actually, I was guilty of posting email to LKML before being fully awake... How about the following instead?

	void set_nmi_callback(nmi_callback_t callback)
	{
		rcu_assign_pointer(nmi_callback, callback);
	}

Similarly:

	void unset_nmi_callback(void)
	{
		rcu_assign_pointer(nmi_callback, dummy_nmi_callback);
	}

This, combined with the rcu_dereference() in do_nmi() seem to me to make the usage a lot more clear. If you agree, would you like to submit the patches, or should I?

Glad they help, will clean them up (so that the examples use rcu_dereference() and rcu_assign_pointer()) and submit them!

Thanx, Paul

Great, more API coverage is always better.

You go ahead.

Thanks,
Zwane