From: Gustavo Padovan <padovan@profusion.mobi>
To: Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [RFCv6 02/14] Bluetooth: Revert to mutexes from RCU list
Date: Mon, 20 Feb 2012 16:33:57 -0200 [thread overview]
Message-ID: <20120220165334.GB28852@joana> (raw)
In-Reply-To: <1329747685-4163-3-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-20 16:21:13 +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 | 165 ++++++++++++++++++++++++++------------------
> net/bluetooth/l2cap_sock.c | 10 +++
> 2 files changed, 108 insertions(+), 67 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 63539f9..90e29ac 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,32 @@ 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;
> }
>
> @@ -228,11 +212,13 @@ static void l2cap_chan_timeout(struct work_struct *work)
> {
> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> chan_timer.work);
> + struct l2cap_conn *conn = chan->conn;
> struct sock *sk = chan->sk;
> int reason;
>
> BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(sk);
>
> if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> @@ -248,6 +234,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
> release_sock(sk);
>
> chan->ops->close(chan->data);
> + mutex_unlock(&conn->chan_lock);
> +
> l2cap_chan_put(chan);
> }
>
> @@ -331,7 +319,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);
> }
>
> /* Delete channel.
> @@ -348,8 +338,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>
> if (conn) {
> /* Delete from channel list */
> - list_del_rcu(&chan->list);
> - synchronize_rcu();
> + list_del(&chan->list);
>
> l2cap_chan_put(chan);
>
> @@ -400,10 +389,12 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
> /* Close not yet accepted channels */
> while ((sk = bt_accept_dequeue(parent, NULL))) {
> struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +
> __clear_chan_timer(chan);
> lock_sock(sk);
> l2cap_chan_close(chan, ECONNRESET);
> release_sock(sk);
> +
> chan->ops->close(chan->data);
> }
> }
> @@ -718,13 +709,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;
> + struct l2cap_chan *chan, *tmp;
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> bh_lock_sock(sk);
> @@ -804,7 +795,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
> bh_unlock_sock(sk);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* Find socket with cid and source bdaddr.
> @@ -916,9 +907,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> if (conn->hcon->out && conn->hcon->type == LE_LINK)
> smp_conn_security(conn, conn->hcon->pending_sec_level);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> bh_lock_sock(sk);
> @@ -938,7 +929,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> bh_unlock_sock(sk);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* Notify sockets that we cannot guaranty reliability anymore */
> @@ -948,16 +939,16 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
> sk->sk_err = err;
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> static void l2cap_info_timeout(struct work_struct *work)
> @@ -984,6 +975,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>
> kfree_skb(conn->rx_skb);
>
> + mutex_lock(&conn->chan_lock);
> +
> /* Kill channels */
> list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
> sk = chan->sk;
> @@ -993,6 +986,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> chan->ops->close(chan->data);
> }
>
> + mutex_unlock(&conn->chan_lock);
> +
> hci_chan_del(conn->hchan);
>
> if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> @@ -1050,6 +1045,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> conn->feat_mask = 0;
>
> spin_lock_init(&conn->lock);
> + mutex_init(&conn->chan_lock);
>
> INIT_LIST_HEAD(&conn->chan_l);
>
> @@ -1792,9 +1788,9 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
> if (chan->chan_type != L2CAP_CHAN_RAW)
> continue;
> @@ -1810,7 +1806,7 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
> kfree_skb(nskb);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* ---- L2CAP signalling commands ---- */
> @@ -2600,6 +2596,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>
> parent = pchan->sk;
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(parent);
>
> /* Check if the ACL is secure enough (if not SDP) */
> @@ -2673,6 +2670,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>
> response:
> release_sock(parent);
> + mutex_unlock(&conn->chan_lock);
>
> sendresp:
> rsp.scid = cpu_to_le16(scid);
> @@ -2714,6 +2712,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> struct l2cap_chan *chan;
> struct sock *sk;
> u8 req[128];
> + int err;
>
> scid = __le16_to_cpu(rsp->scid);
> dcid = __le16_to_cpu(rsp->dcid);
> @@ -2723,17 +2722,26 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
> dcid, scid, result, status);
>
> + mutex_lock(&conn->chan_lock);
> +
> if (scid) {
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> } else {
> - chan = l2cap_get_chan_by_ident(conn, cmd->ident);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> }
>
> + err = 0;
> +
> sk = chan->sk;
> + lock_sock(sk);
>
> switch (result) {
> case L2CAP_CR_SUCCESS:
> @@ -2760,7 +2768,10 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> }
>
> release_sock(sk);
> - return 0;
add an extra blank line here please.
> +unlock:
> + mutex_unlock(&conn->chan_lock);
> +
> + return err;
> }
>
> static inline void set_default_fcs(struct l2cap_chan *chan)
> @@ -2793,6 +2804,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> return -ENOENT;
>
> sk = chan->sk;
> + lock_sock(sk);
>
> if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
> struct l2cap_cmd_rej_cid rej;
> @@ -2905,6 +2917,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> return 0;
>
> sk = chan->sk;
> + lock_sock(sk);
>
> switch (result) {
> case L2CAP_CONF_SUCCESS:
> @@ -3006,11 +3019,16 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
>
> - chan = l2cap_get_chan_by_scid(conn, dcid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, dcid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
Seems to me we can use l2cap_get_chan_by_scid() here.
> + }
>
> sk = chan->sk;
> + lock_sock(sk);
>
> rsp.dcid = cpu_to_le16(chan->scid);
> rsp.scid = cpu_to_le16(chan->dcid);
> @@ -3022,6 +3040,9 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> release_sock(sk);
>
> chan->ops->close(chan->data);
> +
> + mutex_unlock(&conn->chan_lock);
> +
> return 0;
> }
>
> @@ -3037,16 +3058,24 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
>
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
Same here.
Gustavo
next prev parent reply other threads:[~2012-02-20 18:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-20 14:21 [RFCv6 00/14] Bluetooth: Change socket lock to l2cap_chan lock Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 01/14] Bluetooth: trivial: Fix long line Emeltchenko Andrei
2012-02-20 16:38 ` Gustavo Padovan
2012-02-20 14:21 ` [RFCv6 02/14] Bluetooth: Revert to mutexes from RCU list Emeltchenko Andrei
2012-02-20 14:44 ` Ulisses Furquim
2012-02-20 18:33 ` Gustavo Padovan [this message]
2012-02-20 18:57 ` Ulisses Furquim
2012-02-20 22:46 ` Gustavo Padovan
2012-02-20 14:21 ` [RFCv6 03/14] Bluetooth: Add l2cap_chan_lock Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 04/14] Bluetooth: Add locked and unlocked state_change Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 05/14] Bluetooth: Add socket error function Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 06/14] Bluetooth: Add unlocked __l2cap_chan_add function Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 07/14] Bluetooth: Use chan lock in timers Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 08/14] Bluetooth: Use chan lock in L2CAP sig commands Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 09/14] Bluetooth: Use chan lock in L2CAP conn start Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 10/14] Bluetooth: Use chan lock in receiving data Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 11/14] Bluetooth: Change locking logic for conn/chan ready Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 12/14] Bluetooth: Change locking logic in security_cfm Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 13/14] Bluetooth: Use l2cap chan lock in socket connect Emeltchenko Andrei
2012-02-20 14:21 ` [RFCv6 14/14] Bluetooth: Remove socket lock check Emeltchenko Andrei
2012-02-20 14:29 ` [RFCv6 00/14] Bluetooth: Change socket lock to l2cap_chan lock Marcel Holtmann
2012-02-20 14:44 ` Emeltchenko Andrei
2012-02-20 14:52 ` Ulisses Furquim
2012-02-20 14:53 ` Marcel Holtmann
2012-02-21 10:57 ` Emeltchenko Andrei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120220165334.GB28852@joana \
--to=padovan@profusion.mobi \
--cc=Andrei.Emeltchenko.news@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.