LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2/2] rculist.h: use the rcu API
       [not found] <47BAC1E6.2050901@gmail.com>
@ 2008-02-19 11:51 ` Franck Bui-Huu
  0 siblings, 0 replies; 6+ messages in thread
From: Franck Bui-Huu @ 2008-02-19 11:51 UTC (permalink / raw)
  To: Andrew Morton; +Cc: paulmck, Linux Kernel Mailing List

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patch makes almost all list mutation primitives use
rcu_assign_pointer().

The main point of this being readability improvement.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 include/linux/rculist.h |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index aa9b3eb..8d2c81f 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -7,6 +7,7 @@
  * RCU-protected list version
  */
 #include <linux/list.h>
+#include <linux/rcupdate.h>
 
 /*
  * Insert a new entry between two known consecutive entries.
@@ -19,9 +20,8 @@ static inline void __list_add_rcu(struct list_head *new,
 {
 	new->next = next;
 	new->prev = prev;
-	smp_wmb();
+	rcu_assign_pointer(prev->next, new);
 	next->prev = new;
-	prev->next = new;
 }
 
 /**
@@ -110,9 +110,8 @@ static inline void list_replace_rcu(struct list_head *old,
 {
 	new->next = old->next;
 	new->prev = old->prev;
-	smp_wmb();
+	rcu_assign_pointer(new->prev->next, new);
 	new->next->prev = new;
-	new->prev->next = new;
 	old->prev = LIST_POISON2;
 }
 
@@ -166,8 +165,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
 	 */
 
 	last->next = at;
-	smp_wmb();
-	head->next = first;
+	rcu_assign_pointer(head->next, first);
 	first->prev = head;
 	at->prev = last;
 }
@@ -280,10 +278,9 @@ static inline void hlist_replace_rcu(struct hlist_node *old,
 
 	new->next = next;
 	new->pprev = old->pprev;
-	smp_wmb();
+	rcu_assign_pointer(*new->pprev, new);
 	if (next)
 		new->next->pprev = &new->next;
-	*new->pprev = new;
 	old->pprev = LIST_POISON2;
 }
 
@@ -310,12 +307,12 @@ static inline void hlist_add_head_rcu(struct hlist_node *n,
 					struct hlist_head *h)
 {
 	struct hlist_node *first = h->first;
+
 	n->next = first;
 	n->pprev = &h->first;
-	smp_wmb();
+	rcu_assign_pointer(h->first, n);
 	if (first)
 		first->pprev = &n->next;
-	h->first = n;
 }
 
 /**
@@ -341,9 +338,8 @@ static inline void hlist_add_before_rcu(struct hlist_node *n,
 {
 	n->pprev = next->pprev;
 	n->next = next;
-	smp_wmb();
+	rcu_assign_pointer(*(n->pprev), n);
 	next->pprev = &n->next;
-	*(n->pprev) = n;
 }
 
 /**
@@ -369,8 +365,7 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 {
 	n->next = prev->next;
 	n->pprev = &prev->next;
-	smp_wmb();
-	prev->next = n;
+	rcu_assign_pointer(prev->next, n);
 	if (n->next)
 		n->next->pprev = &n->next;
 }
-- 
1.5.4



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

* Re: [PATCH 2/2] rculist.h: use the rcu API
  2008-05-15  5:50   ` Alexey Dobriyan
@ 2008-05-15 14:48     ` Paul E. McKenney
  0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2008-05-15 14:48 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Franck Bui-Huu, Andrew Morton, Josh Triplett, Linux Kernel Mailing List

On Thu, May 15, 2008 at 09:50:11AM +0400, Alexey Dobriyan wrote:
> On Wed, May 14, 2008 at 11:26:18PM +0200, Franck Bui-Huu wrote:
> > This patch makes almost all list mutation primitives use
> > rcu_assign_pointer().
> >
> > The main point of this being readability improvement.
> 
> Which is not an improvement at all.
> 
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -17,9 +18,8 @@ static inline void __list_add_rcu(struct list_head *new,
> > {
> > 	new->next = next;
> > 	new->prev = prev;
> > -	smp_wmb();
> > +	rcu_assign_pointer(prev->next, new);
> > 	next->prev = new;
> > -	prev->next = new;
> > }
> 
> Nice chunk to demonstrate.
> 
> Before one could write this like:
> 
> 	smp_wmb();			smp_wmb();
> 	next->prev = new;	or	prev->next = new;
> 	prev->next = new;		next->prev = new;
> 
> And both examples aren't buggy.
> 
> After, you can't write:
> 
> 	next->prev = new;
> 	rcu_assign_pointer(prev->next, new);
> 
> Guess why?

Strangely enough, you actually can do this, because RCU readers are
not permitted to follow the prev pointers.  Any RCU readers who try to
follow the prev pointers are subject to failures due to list_del_rcu()'s
assignment:

	entry->prev = LIST_POISON2;

So this change is not in fact relying on undocumented rcu_assign_pointer()
side effects.

							Thanx, Paul

> This barrier is related not only to next assignment, but to the whole
> group of assignments.
> 
> > @@ -108,9 +108,8 @@ static inline void list_replace_rcu(struct list_head 
> > *old,
> > {
> > 	new->next = old->next;
> > 	new->prev = old->prev;
> > -	smp_wmb();
> > +	rcu_assign_pointer(new->prev->next, new);
> > 	new->next->prev = new;
> > -	new->prev->next = new;
> > 	old->prev = LIST_POISON2;
> > }
> 

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

* Re: [PATCH 2/2] rculist.h: use the rcu API
  2008-05-14 21:26 ` [PATCH 2/2] rculist.h: use the rcu API Franck Bui-Huu
@ 2008-05-15  5:50   ` Alexey Dobriyan
  2008-05-15 14:48     ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Alexey Dobriyan @ 2008-05-15  5:50 UTC (permalink / raw)
  To: Franck Bui-Huu
  Cc: Andrew Morton, paulmck, Josh Triplett, Linux Kernel Mailing List

On Wed, May 14, 2008 at 11:26:18PM +0200, Franck Bui-Huu wrote:
> This patch makes almost all list mutation primitives use
> rcu_assign_pointer().
>
> The main point of this being readability improvement.

Which is not an improvement at all.

> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -17,9 +18,8 @@ static inline void __list_add_rcu(struct list_head *new,
> {
> 	new->next = next;
> 	new->prev = prev;
> -	smp_wmb();
> +	rcu_assign_pointer(prev->next, new);
> 	next->prev = new;
> -	prev->next = new;
> }

Nice chunk to demonstrate.

Before one could write this like:

	smp_wmb();			smp_wmb();
	next->prev = new;	or	prev->next = new;
	prev->next = new;		next->prev = new;

And both examples aren't buggy.

After, you can't write:

	next->prev = new;
	rcu_assign_pointer(prev->next, new);

Guess why?

This barrier is related not only to next assignment, but to the whole
group of assignments.

> @@ -108,9 +108,8 @@ static inline void list_replace_rcu(struct list_head 
> *old,
> {
> 	new->next = old->next;
> 	new->prev = old->prev;
> -	smp_wmb();
> +	rcu_assign_pointer(new->prev->next, new);
> 	new->next->prev = new;
> -	new->prev->next = new;
> 	old->prev = LIST_POISON2;
> }


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

* [PATCH 2/2] rculist.h: use the rcu API
  2008-05-14 21:24 [PATCH 1/2] Split list.h and move rcu-protected lists into rculist.h Franck Bui-Huu
@ 2008-05-14 21:26 ` Franck Bui-Huu
  2008-05-15  5:50   ` Alexey Dobriyan
  0 siblings, 1 reply; 6+ messages in thread
From: Franck Bui-Huu @ 2008-05-14 21:26 UTC (permalink / raw)
  To: Andrew Morton; +Cc: paulmck, Josh Triplett, Linux Kernel Mailing List

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patch makes almost all list mutation primitives use
rcu_assign_pointer().

The main point of this being readability improvement.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 include/linux/rculist.h |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e673c26..52cee71 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -5,6 +5,7 @@
  * RCU-protected list version
  */
 #include <linux/list.h>
+#include <linux/rcupdate.h>
 
 /*
  * Insert a new entry between two known consecutive entries.
@@ -17,9 +18,8 @@ static inline void __list_add_rcu(struct list_head *new,
 {
 	new->next = next;
 	new->prev = prev;
-	smp_wmb();
+	rcu_assign_pointer(prev->next, new);
 	next->prev = new;
-	prev->next = new;
 }
 
 /**
@@ -108,9 +108,8 @@ static inline void list_replace_rcu(struct list_head *old,
 {
 	new->next = old->next;
 	new->prev = old->prev;
-	smp_wmb();
+	rcu_assign_pointer(new->prev->next, new);
 	new->next->prev = new;
-	new->prev->next = new;
 	old->prev = LIST_POISON2;
 }
 
@@ -164,8 +163,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
 	 */
 
 	last->next = at;
-	smp_wmb();
-	head->next = first;
+	rcu_assign_pointer(head->next, first);
 	first->prev = head;
 	at->prev = last;
 }
@@ -260,10 +258,9 @@ static inline void hlist_replace_rcu(struct hlist_node *old,
 
 	new->next = next;
 	new->pprev = old->pprev;
-	smp_wmb();
+	rcu_assign_pointer(*new->pprev, new);
 	if (next)
 		new->next->pprev = &new->next;
-	*new->pprev = new;
 	old->pprev = LIST_POISON2;
 }
 
@@ -290,12 +287,12 @@ static inline void hlist_add_head_rcu(struct hlist_node *n,
 					struct hlist_head *h)
 {
 	struct hlist_node *first = h->first;
+
 	n->next = first;
 	n->pprev = &h->first;
-	smp_wmb();
+	rcu_assign_pointer(h->first, n);
 	if (first)
 		first->pprev = &n->next;
-	h->first = n;
 }
 
 /**
@@ -321,9 +318,8 @@ static inline void hlist_add_before_rcu(struct hlist_node *n,
 {
 	n->pprev = next->pprev;
 	n->next = next;
-	smp_wmb();
+	rcu_assign_pointer(*(n->pprev), n);
 	next->pprev = &n->next;
-	*(n->pprev) = n;
 }
 
 /**
@@ -349,8 +345,7 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 {
 	n->next = prev->next;
 	n->pprev = &prev->next;
-	smp_wmb();
-	prev->next = n;
+	rcu_assign_pointer(prev->next, n);
 	if (n->next)
 		n->next->pprev = &n->next;
 }
-- 
1.5.5.GIT


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

* Re: [PATCH 2/2] rculist.h: use the rcu API
  2008-01-17 20:48 ` [PATCH 2/2] rculist.h: use the rcu API Franck Bui-Huu
@ 2008-01-18  5:55   ` Paul E. McKenney
  0 siblings, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2008-01-18  5:55 UTC (permalink / raw)
  To: Franck Bui-Huu; +Cc: Linux Kernel Mailing List

On Thu, Jan 17, 2008 at 09:48:25PM +0100, Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> This patch makes almost all list mutation primitives use
> rcu_assign_pointer().
> 
> The main point of this being readability improvement.

Looks better to me!

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  include/linux/rculist.h |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index 8056d38..038b64f 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -7,6 +7,7 @@
>   * RCU-protected list version
>   */
>  #include <linux/list.h>
> +#include <linux/rcupdate.h>
> 
>  /*
>   * Insert a new entry between two known consecutive entries.
> @@ -19,9 +20,8 @@ static inline void __list_add_rcu(struct list_head * new,
>  {
>  	new->next = next;
>  	new->prev = prev;
> -	smp_wmb();
> +	rcu_assign_pointer(prev->next, new);
>  	next->prev = new;
> -	prev->next = new;
>  }
> 
>  /**
> @@ -110,9 +110,8 @@ static inline void list_replace_rcu(struct list_head *old,
>  {
>  	new->next = old->next;
>  	new->prev = old->prev;
> -	smp_wmb();
> +	rcu_assign_pointer(new->prev->next, new);
>  	new->next->prev = new;
> -	new->prev->next = new;
>  	old->prev = LIST_POISON2;
>  }
> 
> @@ -166,8 +165,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
>  	 */
> 
>  	last->next = at;
> -	smp_wmb();
> -	head->next = first;
> +	rcu_assign_pointer(head->next, first);
>  	first->prev = head;
>  	at->prev = last;
>  }
> @@ -280,10 +278,9 @@ static inline void hlist_replace_rcu(struct hlist_node *old,
> 
>  	new->next = next;
>  	new->pprev = old->pprev;
> -	smp_wmb();
> +	rcu_assign_pointer(*new->pprev, new);
>  	if (next)
>  		new->next->pprev = &new->next;
> -	*new->pprev = new;
>  	old->pprev = LIST_POISON2;
>  }
> 
> @@ -310,12 +307,12 @@ static inline void hlist_add_head_rcu(struct hlist_node *n,
>  					struct hlist_head *h)
>  {
>  	struct hlist_node *first = h->first;
> +
>  	n->next = first;
>  	n->pprev = &h->first;
> -	smp_wmb();
> +	rcu_assign_pointer(h->first, n);
>  	if (first)
>  		first->pprev = &n->next;
> -	h->first = n;
>  }
> 
>  /**
> @@ -341,9 +338,8 @@ static inline void hlist_add_before_rcu(struct hlist_node *n,
>  {
>  	n->pprev = next->pprev;
>  	n->next = next;
> -	smp_wmb();
> +	rcu_assign_pointer(*(n->pprev), n);
>  	next->pprev = &n->next;
> -	*(n->pprev) = n;
>  }
> 
>  /**
> @@ -369,8 +365,7 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
>  {
>  	n->next = prev->next;
>  	n->pprev = &prev->next;
> -	smp_wmb();
> -	prev->next = n;
> +	rcu_assign_pointer(prev->next, n);
>  	if (n->next)
>  		n->next->pprev = &n->next;
>  }
> -- 
> 1.5.3.7
> 

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

* [PATCH 2/2] rculist.h: use the rcu API
  2008-01-17 20:46 [PATCH 0/2] Make RCU lists use the RCU API Franck Bui-Huu
@ 2008-01-17 20:48 ` Franck Bui-Huu
  2008-01-18  5:55   ` Paul E. McKenney
  0 siblings, 1 reply; 6+ messages in thread
From: Franck Bui-Huu @ 2008-01-17 20:48 UTC (permalink / raw)
  To: Linux Kernel Mailing List; +Cc: paulmck

From: Franck Bui-Huu <fbuihuu@gmail.com>

This patch makes almost all list mutation primitives use
rcu_assign_pointer().

The main point of this being readability improvement.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 include/linux/rculist.h |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 8056d38..038b64f 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -7,6 +7,7 @@
  * RCU-protected list version
  */
 #include <linux/list.h>
+#include <linux/rcupdate.h>
 
 /*
  * Insert a new entry between two known consecutive entries.
@@ -19,9 +20,8 @@ static inline void __list_add_rcu(struct list_head * new,
 {
 	new->next = next;
 	new->prev = prev;
-	smp_wmb();
+	rcu_assign_pointer(prev->next, new);
 	next->prev = new;
-	prev->next = new;
 }
 
 /**
@@ -110,9 +110,8 @@ static inline void list_replace_rcu(struct list_head *old,
 {
 	new->next = old->next;
 	new->prev = old->prev;
-	smp_wmb();
+	rcu_assign_pointer(new->prev->next, new);
 	new->next->prev = new;
-	new->prev->next = new;
 	old->prev = LIST_POISON2;
 }
 
@@ -166,8 +165,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
 	 */
 
 	last->next = at;
-	smp_wmb();
-	head->next = first;
+	rcu_assign_pointer(head->next, first);
 	first->prev = head;
 	at->prev = last;
 }
@@ -280,10 +278,9 @@ static inline void hlist_replace_rcu(struct hlist_node *old,
 
 	new->next = next;
 	new->pprev = old->pprev;
-	smp_wmb();
+	rcu_assign_pointer(*new->pprev, new);
 	if (next)
 		new->next->pprev = &new->next;
-	*new->pprev = new;
 	old->pprev = LIST_POISON2;
 }
 
@@ -310,12 +307,12 @@ static inline void hlist_add_head_rcu(struct hlist_node *n,
 					struct hlist_head *h)
 {
 	struct hlist_node *first = h->first;
+
 	n->next = first;
 	n->pprev = &h->first;
-	smp_wmb();
+	rcu_assign_pointer(h->first, n);
 	if (first)
 		first->pprev = &n->next;
-	h->first = n;
 }
 
 /**
@@ -341,9 +338,8 @@ static inline void hlist_add_before_rcu(struct hlist_node *n,
 {
 	n->pprev = next->pprev;
 	n->next = next;
-	smp_wmb();
+	rcu_assign_pointer(*(n->pprev), n);
 	next->pprev = &n->next;
-	*(n->pprev) = n;
 }
 
 /**
@@ -369,8 +365,7 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
 {
 	n->next = prev->next;
 	n->pprev = &prev->next;
-	smp_wmb();
-	prev->next = n;
+	rcu_assign_pointer(prev->next, n);
 	if (n->next)
 		n->next->pprev = &n->next;
 }
-- 
1.5.3.7


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

end of thread, other threads:[~2008-05-15 14:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <47BAC1E6.2050901@gmail.com>
2008-02-19 11:51 ` [PATCH 2/2] rculist.h: use the rcu API Franck Bui-Huu
2008-05-14 21:24 [PATCH 1/2] Split list.h and move rcu-protected lists into rculist.h Franck Bui-Huu
2008-05-14 21:26 ` [PATCH 2/2] rculist.h: use the rcu API Franck Bui-Huu
2008-05-15  5:50   ` Alexey Dobriyan
2008-05-15 14:48     ` Paul E. McKenney
  -- strict thread matches above, loose matches on Subject: below --
2008-01-17 20:46 [PATCH 0/2] Make RCU lists use the RCU API Franck Bui-Huu
2008-01-17 20:48 ` [PATCH 2/2] rculist.h: use the rcu API Franck Bui-Huu
2008-01-18  5:55   ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).