public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
@ 2005-06-18  0:20 Paul E. McKenney
  2005-06-19 17:53 ` Zwane Mwaikambo
  0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2005-06-18  0:20 UTC (permalink / raw)
  To: linux-kernel; +Cc: dhowells, dipankar, ak, akpm, maneesh

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.

Signed-off-by: paulmck@us.ibm.com

---

 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
--- linux-2.6.12-rc6-mm1/arch/x86_64/kernel/mce.c	Fri Jun 17 16:40:34 2005
+++ linux-2.6.12-rc6-mm1-sksr/arch/x86_64/kernel/mce.c	Fri Jun 17 16:47:51 2005
@@ -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
--- linux-2.6.12-rc6-mm1/drivers/input/serio/i8042.c	Fri Jun 17 16:34:36 2005
+++ linux-2.6.12-rc6-mm1-sksr/drivers/input/serio/i8042.c	Fri Jun 17 16:44:26 2005
@@ -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
--- linux-2.6.12-rc6-mm1/include/asm-i386/kdebug.h	Tue Mar  1 23:38:25 2005
+++ linux-2.6.12-rc6-mm1-sksr/include/asm-i386/kdebug.h	Fri Jun 17 16:50:10 2005
@@ -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
--- linux-2.6.12-rc6-mm1/include/asm-ppc64/kdebug.h	Tue Mar  1 23:38:01 2005
+++ linux-2.6.12-rc6-mm1-sksr/include/asm-ppc64/kdebug.h	Fri Jun 17 16:48:29 2005
@@ -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
--- linux-2.6.12-rc6-mm1/include/asm-sparc64/kdebug.h	Tue Mar  1 23:38:32 2005
+++ linux-2.6.12-rc6-mm1-sksr/include/asm-sparc64/kdebug.h	Fri Jun 17 16:55:26 2005
@@ -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
--- linux-2.6.12-rc6-mm1/include/asm-x86_64/kdebug.h	Fri Jun 17 16:35:11 2005
+++ linux-2.6.12-rc6-mm1-sksr/include/asm-x86_64/kdebug.h	Fri Jun 17 16:48:56 2005
@@ -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
--- linux-2.6.12-rc6-mm1/include/linux/list.h	Fri Jun 17 16:41:12 2005
+++ linux-2.6.12-rc6-mm1-sksr/include/linux/list.h	Fri Jun 17 16:49:47 2005
@@ -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
--- linux-2.6.12-rc6-mm1/security/keys/key.c	Fri Jun 17 16:41:16 2005
+++ linux-2.6.12-rc6-mm1-sksr/security/keys/key.c	Fri Jun 17 16:51:46 2005
@@ -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
--- linux-2.6.12-rc6-mm1/security/keys/process_keys.c	Fri Jun 17 16:41:16 2005
+++ linux-2.6.12-rc6-mm1-sksr/security/keys/process_keys.c	Fri Jun 17 16:51:26 2005
@@ -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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-18  0:20 [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls Paul E. McKenney
@ 2005-06-19 17:53 ` Zwane Mwaikambo
  2005-06-19 20:14   ` Paul E. McKenney
  2005-06-27  5:02   ` Paul E. McKenney
  0 siblings, 2 replies; 10+ messages in thread
From: Zwane Mwaikambo @ 2005-06-19 17:53 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Fri, 17 Jun 2005, Paul E. McKenney wrote:

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

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-19 17:53 ` Zwane Mwaikambo
@ 2005-06-19 20:14   ` Paul E. McKenney
  2005-06-20 11:58     ` Zwane Mwaikambo
  2005-06-27  5:02   ` Paul E. McKenney
  1 sibling, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2005-06-19 20:14 UTC (permalink / raw)
  To: Zwane Mwaikambo; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Sun, Jun 19, 2005 at 11:53:24AM -0600, Zwane Mwaikambo wrote:
> On Fri, 17 Jun 2005, Paul E. McKenney wrote:
> 
> > Please let me know if there are any problems with any of these changes.
> 
> 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/

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

---

Signed-off-by: paulmck@us.ibm.com

 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
--- linux-2.6.12-rc6/include/linux/rcupdate.h	Fri Jun 17 16:35:13 2005
+++ linux-2.6.12-rc6-ssdoc/include/linux/rcupdate.h	Sun Jun 19 12:53:51 2005
@@ -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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-19 20:14   ` Paul E. McKenney
@ 2005-06-20 11:58     ` Zwane Mwaikambo
  0 siblings, 0 replies; 10+ messages in thread
From: Zwane Mwaikambo @ 2005-06-20 11:58 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

Hi Paul,

On Sun, 19 Jun 2005, Paul E. McKenney wrote:

> 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:
> 
> 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?

Thanks that looks like it covers it.

	Zwane


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-19 17:53 ` Zwane Mwaikambo
  2005-06-19 20:14   ` Paul E. McKenney
@ 2005-06-27  5:02   ` Paul E. McKenney
  2005-06-28 14:22     ` Zwane Mwaikambo
  1 sibling, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2005-06-27  5:02 UTC (permalink / raw)
  To: Zwane Mwaikambo; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Sun, Jun 19, 2005 at 11:53:24AM -0600, Zwane Mwaikambo wrote:
> On Fri, 17 Jun 2005, Paul E. McKenney wrote:
> 
> > Please let me know if there are any problems with any of these changes.
> 
> 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,

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-27  5:02   ` Paul E. McKenney
@ 2005-06-28 14:22     ` Zwane Mwaikambo
  2005-06-28 15:32       ` Paul E. McKenney
  0 siblings, 1 reply; 10+ messages in thread
From: Zwane Mwaikambo @ 2005-06-28 14:22 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

Hello Paul,

On Sun, 26 Jun 2005, Paul E. McKenney wrote:

> How does the following look for NMI-RCU documentation?

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-28 14:22     ` Zwane Mwaikambo
@ 2005-06-28 15:32       ` Paul E. McKenney
  2005-06-28 17:15         ` Zwane Mwaikambo
  0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2005-06-28 15:32 UTC (permalink / raw)
  To: Zwane Mwaikambo; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Tue, Jun 28, 2005 at 08:22:24AM -0600, Zwane Mwaikambo wrote:
> Hello Paul,
> 
> On Sun, 26 Jun 2005, Paul E. McKenney wrote:
> 
> > How does the following look for NMI-RCU documentation?
> 
> 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;

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

> 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

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

> 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?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-28 15:32       ` Paul E. McKenney
@ 2005-06-28 17:15         ` Zwane Mwaikambo
  2005-06-28 17:40           ` Paul E. McKenney
  0 siblings, 1 reply; 10+ messages in thread
From: Zwane Mwaikambo @ 2005-06-28 17:15 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Tue, 28 Jun 2005, Paul E. McKenney wrote:

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

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

> 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.

I agree, that usage looks safe.

> 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?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-28 17:15         ` Zwane Mwaikambo
@ 2005-06-28 17:40           ` Paul E. McKenney
  2005-06-28 18:02             ` Zwane Mwaikambo
  0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2005-06-28 17:40 UTC (permalink / raw)
  To: Zwane Mwaikambo; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Tue, Jun 28, 2005 at 11:15:55AM -0600, Zwane Mwaikambo wrote:
> On Tue, 28 Jun 2005, Paul E. McKenney wrote:
> > 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.
> 
> I agree, that usage looks safe.

Cool!

> > 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?
> 
> 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"

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?

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

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

							Thanx, Paul

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls
  2005-06-28 17:40           ` Paul E. McKenney
@ 2005-06-28 18:02             ` Zwane Mwaikambo
  0 siblings, 0 replies; 10+ messages in thread
From: Zwane Mwaikambo @ 2005-06-28 18:02 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, dhowells, dipankar, ak, akpm, maneesh

On Tue, 28 Jun 2005, Paul E. McKenney wrote:

> 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);
> 	}

Great, more API coverage is always better.

> 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?

You go ahead.

> > Thanks for elaborating, the examples certainly do help clarify usage.
> 
> Glad they help, will clean them up (so that the examples use
> rcu_dereference() and rcu_assign_pointer()) and submit them!

Thanks,
	Zwane


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2005-06-28 17:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-06-18  0:20 [RFC,PATCH] RCU: clean up a few remaining synchronize_kernel() calls Paul E. McKenney
2005-06-19 17:53 ` Zwane Mwaikambo
2005-06-19 20:14   ` Paul E. McKenney
2005-06-20 11:58     ` Zwane Mwaikambo
2005-06-27  5:02   ` Paul E. McKenney
2005-06-28 14:22     ` Zwane Mwaikambo
2005-06-28 15:32       ` Paul E. McKenney
2005-06-28 17:15         ` Zwane Mwaikambo
2005-06-28 17:40           ` Paul E. McKenney
2005-06-28 18:02             ` Zwane Mwaikambo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox