[PATCH 2.6 PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place

[PATCH 2.6 PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

The patch "RCU: use rcu_assign_pointer()" 
http://linux.bkbits.net:8080/linux-2.6/diffs/net/sched/sch_api.c@1.39?nav=index.html|src/net/|src/net|src/net/sched|related/net/sched/sch_api.c|cset@1.2287
changed a list_add_tail to list_add_tail_rcu in qdisc_create. It's
dev->qdisc not dev->qdisc_list that is protected by RCU, this patch
reverts that change. It also removes a misleading comment and replaces
the smp_wmb in qdisc_create_dflt by rcu_assign_pointer in dev_activate
to document more clearly what is protected.

Regards
Patrick







[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2045 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/11/05 22:14:18+01:00 kaber@coreworks.de 
#   [PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/sched/sch_generic.c
#   2004/11/05 22:14:08+01:00 kaber@coreworks.de +5 -7
#   [PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/sched/sch_api.c
#   2004/11/05 22:14:08+01:00 kaber@coreworks.de +2 -4
#   [PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/sched/sch_api.c b/net/sched/sch_api.c
--- a/net/sched/sch_api.c	2004-11-05 22:29:06 +01:00
+++ b/net/sched/sch_api.c	2004-11-05 22:29:06 +01:00
@@ -451,11 +451,9 @@
         else
                 sch->handle = handle;
 
-	/* enqueue is accessed locklessly - make sure it's visible
-	 * before we set a netdevice's qdisc pointer to sch */
 	if (!ops->init || (err = ops->init(sch, tca[TCA_OPTIONS-1])) == 0) {
 		qdisc_lock_tree(dev);
-		list_add_tail_rcu(&sch->list, &dev->qdisc_list);
+		list_add_tail(&sch->list, &dev->qdisc_list);
 		qdisc_unlock_tree(dev);
 
 #ifdef CONFIG_NET_ESTIMATOR
diff -Nru a/net/sched/sch_generic.c b/net/sched/sch_generic.c
--- a/net/sched/sch_generic.c	2004-11-05 22:29:06 +01:00
+++ b/net/sched/sch_generic.c	2004-11-05 22:29:06 +01:00
@@ -436,9 +436,6 @@
 	dev_hold(dev);
 	sch->stats_lock = &dev->queue_lock;
 	atomic_set(&sch->refcnt, 1);
-	/* enqueue is accessed locklessly - make sure it's visible
-	 * before we set a netdevice's qdisc pointer to sch */
-	smp_wmb();
 	if (!ops->init || ops->init(sch, NULL) == 0)
 		return sch;
 
@@ -519,7 +516,8 @@
 	}
 
 	spin_lock_bh(&dev->queue_lock);
-	if ((dev->qdisc = dev->qdisc_sleeping) != &noqueue_qdisc) {
+	rcu_assign_pointer(dev->qdisc, dev->qdisc_sleeping);
+	if (dev->qdisc != &noqueue_qdisc) {
 		dev->trans_start = jiffies;
 		dev_watchdog_up(dev);
 	}

Re: [PATCH 2.6 PKT_SCHED]: Fix rcu_assign_pointer fallout, use it in the right place

[David S. Miller]

On Fri, 05 Nov 2004 22:45:46 +0100
Patrick McHardy <kaber@trash.net> wrote:

> The patch "RCU: use rcu_assign_pointer()" 
> http://linux.bkbits.net:8080/linux-2.6/diffs/net/sched/sch_api.c@1.39?nav=index.html|src/net/|src/net|src/net/sched|related/net/sched/sch_api.c|cset@1.2287
> changed a list_add_tail to list_add_tail_rcu in qdisc_create. It's
> dev->qdisc not dev->qdisc_list that is protected by RCU, this patch
> reverts that change. It also removes a misleading comment and replaces
> the smp_wmb in qdisc_create_dflt by rcu_assign_pointer in dev_activate
> to document more clearly what is protected.

Good catch Patrick, patch applied.

Thanks a lot.