Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <20120209183759.GC3174@joana>
References: <1328797057-26331-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
	<1328797057-26331-3-git-send-email-Andrei.Emeltchenko.news@gmail.com>
	<20120209183759.GC3174@joana>
Date: Thu, 9 Feb 2012 16:48:27 -0200
Message-ID: <CAA37ikbc0z49y+0iYd+z=+6Zs0vyPEKsF=dQsEasHibHPQFKDA@mail.gmail.com>
Subject: Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list
From: Ulisses Furquim <ulisses@profusion.mobi>
To: Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com>,
	linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Gustavo,

On Thu, Feb 9, 2012 at 4:37 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-09 16:17:23 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>
>> Usage of RCU list looks not reasonalbe for a number of reasons:
>> our code sleep and we have to use socket spinlocks, some parts
>> of code are updaters thus we need to use mutexes anyway.
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> ---
>> ?net/bluetooth/l2cap_core.c | ?108 ++++++++++++++++++++++----------------------
>> ?1 files changed, 54 insertions(+), 54 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 8dfccb3..ae08944 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
>>
>> ?static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
>> ?{
>> - ? ? struct l2cap_chan *c, *r = NULL;
>> -
>> - ? ? rcu_read_lock();
>> + ? ? struct l2cap_chan *c;
>>
>> - ? ? list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> - ? ? ? ? ? ? if (c->dcid == cid) {
>> - ? ? ? ? ? ? ? ? ? ? r = c;
>> - ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? }
>> + ? ? list_for_each_entry(c, &conn->chan_l, list) {
>> + ? ? ? ? ? ? if (c->dcid == cid)
>> + ? ? ? ? ? ? ? ? ? ? return c;
>> ? ? ? }
>> -
>> - ? ? rcu_read_unlock();
>> - ? ? return r;
>> + ? ? return NULL;
>> ?}
>>
>> ?static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
>> ?{
>> - ? ? struct l2cap_chan *c, *r = NULL;
>> -
>> - ? ? rcu_read_lock();
>> + ? ? struct l2cap_chan *c;
>>
>> - ? ? list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> - ? ? ? ? ? ? if (c->scid == cid) {
>> - ? ? ? ? ? ? ? ? ? ? r = c;
>> - ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? }
>> + ? ? list_for_each_entry(c, &conn->chan_l, list) {
>> + ? ? ? ? ? ? if (c->scid == cid)
>> + ? ? ? ? ? ? ? ? ? ? return c;
>> ? ? ? }
>> -
>> - ? ? rcu_read_unlock();
>> - ? ? return r;
>> + ? ? return NULL;
>> ?}
>>
>> ?/* Find channel with given SCID.
>> @@ -115,36 +103,34 @@ 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, *r = NULL;
>> -
>> - ? ? rcu_read_lock();
>> + ? ? struct l2cap_chan *c;
>>
>> - ? ? list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> - ? ? ? ? ? ? if (c->ident == ident) {
>> - ? ? ? ? ? ? ? ? ? ? r = c;
>> - ? ? ? ? ? ? ? ? ? ? break;
>> - ? ? ? ? ? ? }
>> + ? ? list_for_each_entry(c, &conn->chan_l, list) {
>> + ? ? ? ? ? ? if (c->ident == ident)
>> + ? ? ? ? ? ? ? ? ? ? return c;
>> ? ? ? }
>> -
>> - ? ? rcu_read_unlock();
>> - ? ? return r;
>> + ? ? return NULL;
>> ?}
>>
>> ?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;
>> ?}
>>
>> @@ -259,6 +245,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>>
>> ? ? ? BT_DBG("chan %p state %d", chan, chan->state);
>>
>> + ? ? mutex_lock(&chan->conn->chan_lock);
>> ? ? ? lock_sock(sk);
>>
>> ? ? ? if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
>> @@ -272,6 +259,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>> ? ? ? l2cap_chan_close(chan, reason);
>>
>> ? ? ? release_sock(sk);
>> + ? ? mutex_unlock(&chan->conn->chan_lock);
>>
>> ? ? ? chan->ops->close(chan->data);
>> ? ? ? l2cap_chan_put(chan);
>> @@ -357,7 +345,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>>
>> ? ? ? l2cap_chan_hold(chan);
>>
>> - ? ? list_add_rcu(&chan->list, &conn->chan_l);
>> + ? ? mutex_lock(&conn->chan_lock);
>> + ? ? list_add(&chan->list, &conn->chan_l);
>> + ? ? mutex_unlock(&conn->chan_lock);
>
> Check the commit that added RCU to our list, I think you are missing to revert
> some parts of it.

He's undoing it a little bit different. Check my comments in the
previous series he sent.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs