Re: [RFC 19/22] Bluetooth: Change l2cap chan_list to use RCU

Hi Andrei,

* Andrei Emeltchenko [off-list ref] [2011-12-19 12:42:57 +0200]:

Hi Gustavo,

On Sat, Dec 17, 2011 at 07:29:39PM -0200, Gustavo F. Padovan wrote:

quoted

From: "Gustavo F. Padovan" <redacted>

This list has much more reads than writes, so RCU makes senses here, also
it avoid deadlock against the socket lock.

Signed-off-by: Gustavo F. Padovan <redacted>
---
 net/bluetooth/l2cap_core.c |  117 +++++++++++++++++++++----------------------
 1 files changed, 57 insertions(+), 60 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d616519..a212295 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c

@@ -89,24 +89,36 @@ static inline void chan_put(struct l2cap_chan *c)
 
 static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
 {
-	struct l2cap_chan *c;
+	struct l2cap_chan *c, *r = NULL;
 
-	list_for_each_entry(c, &conn->chan_l, list) {
-		if (c->dcid == cid)
-			return c;
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(c, &conn->chan_l, list) {
+		if (c->dcid == cid) {
+			r = c;
+			break;
+		}
 	}
+
+	rcu_read_unlock();
 	return NULL;
 }
 
 static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
 {
-	struct l2cap_chan *c;
+	struct l2cap_chan *c, *r = NULL;
 
-	list_for_each_entry(c, &conn->chan_l, list) {
-		if (c->scid == cid)
-			return c;
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(c, &conn->chan_l, list) {
+		if (c->scid == cid) {
+			r = c;
+			break;
+		}
 	}
-	return NULL;
+
+	rcu_read_unlock();
+	return r;
 }
 
 /* Find channel with given SCID.

@@ -115,34 +127,36 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
 {
 	struct l2cap_chan *c;
 
-	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_scid(conn, cid);
 	if (c)
 		lock_sock(c->sk);
-	mutex_unlock(&conn->chan_lock);
 	return c;
 }
 
 static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
 {
-	struct l2cap_chan *c;
+	struct l2cap_chan *c, *r = NULL;
 
-	list_for_each_entry(c, &conn->chan_l, list) {
-		if (c->ident == ident)
-			return c;
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(c, &conn->chan_l, list) {
+		if (c->ident == ident) {
+			r = c;
+			break;
+		}
 	}
-	return NULL;
+
+	rcu_read_unlock();
+	return r;
 }
 
 static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
 {
 	struct l2cap_chan *c;
 
-	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_ident(conn, ident);
 	if (c)
 		lock_sock(c->sk);
-	mutex_unlock(&conn->chan_lock);
 	return c;
 }
 

@@ -323,7 +337,7 @@ void l2cap_chan_destroy(struct l2cap_chan *chan)
 	chan_put(chan);
 }
 
-static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 {
 	BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
 			chan->psm, chan->dcid);

@@ -364,7 +378,7 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	chan_hold(chan);
 
-	list_add(&chan->list, &conn->chan_l);
+	list_add_rcu(&chan->list, &conn->chan_l);
 }
 
 /* Delete channel.

@@ -381,9 +395,9 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 	if (conn) {
 		/* Delete from channel list */
-		mutex_lock(&conn->chan_lock);
-		list_del(&chan->list);
-		mutex_unlock(&conn->chan_lock);
+		list_del_rcu(&chan->list);
+		synchronize_rcu();
+
 		chan_put(chan);
 
 		chan->conn = NULL;

@@ -750,13 +764,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 /* ---- L2CAP connections ---- */
 static void l2cap_conn_start(struct l2cap_conn *conn)
 {
-	struct l2cap_chan *chan, *tmp;
+	struct l2cap_chan *chan;
 
 	BT_DBG("conn %p", conn);
 
-	mutex_lock(&conn->chan_lock);
+	rcu_read_lock();
 
-	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
+	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
 		bh_lock_sock(sk);

@@ -780,9 +794,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 					&chan->conf_state)) {
 				/* l2cap_chan_close() calls list_del(chan)
 				 * so release the lock */
-				mutex_unlock(&conn->chan_lock);
 				l2cap_chan_close(chan, ECONNRESET);
-				utex_lock(&conn->chan_lock);

OK, I see why this works. BTW: IMO the mutex and rcu patches shall be
amended, otherwise we do unneeded work of implementing mutexes and then
changing them to RCU.

Indeed, but unfortunately I pushed the patches already and forgot to ammend
both path patches. I was trying to push this ASAP so you guys could rebase
your work on top of it.

	Gustavo