[PATCH] Bluetooth: Simplify L2CAP timer logic

From: Andrei Emeltchenko <redacted>

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

Signed-off-by: Andrei Emeltchenko <redacted>
---
 include/net/bluetooth/l2cap.h |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9b242c6..e4b2fe7 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h

@@ -621,29 +621,30 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
 	mutex_unlock(&chan->lock);
 }
 
-static inline void l2cap_set_timer(struct l2cap_chan *chan,
-					struct delayed_work *work, long timeout)
-{
-	BT_DBG("chan %p state %s timeout %ld", chan,
-					state_to_string(chan->state), timeout);
-
-	if (!cancel_delayed_work(work))
-		l2cap_chan_hold(chan);
-	schedule_delayed_work(work, timeout);
-}
-
 static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
-					struct delayed_work *work)
+				     struct delayed_work *work)
 {
 	bool ret;
 
-	ret = cancel_delayed_work(work);
+	ret = (delayed_work_pending(work) && cancel_delayed_work(work));
 	if (ret)
 		l2cap_chan_put(chan);
 
 	return ret;
 }
 
+static inline void l2cap_set_timer(struct l2cap_chan *chan,
+				   struct delayed_work *work, long timeout)
+{
+	BT_DBG("chan %p state %s timeout %ld", chan,
+	       state_to_string(chan->state), timeout);
+
+	l2cap_clear_timer(chan, work);
+
+	l2cap_chan_hold(chan);
+	schedule_delayed_work(work, timeout);
+}
+
 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
 #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
 #define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \

-- 
1.7.9.1

Hi Andrei,

* Andrei Emeltchenko [off-list ref] [2012-03-20 14:51:08 +0200]:

From: Andrei Emeltchenko <redacted>

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

Signed-off-by: Andrei Emeltchenko <redacted>
---
 include/net/bluetooth/l2cap.h |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

Applied, thanks.

	Gustavo

On Tue, 20 Mar 2012, Andrei Emeltchenko wrote:

Why change this?  cancel_delayed_work() can handle a delayed_work that 
is not pending, and has the proper locking to guard against race 
conditions.

	if (ret)
		l2cap_chan_put(chan);

	return ret;
}

+static inline void l2cap_set_timer(struct l2cap_chan *chan,
+				   struct delayed_work *work, long timeout)
+{
+	BT_DBG("chan %p state %s timeout %ld", chan,
+	       state_to_string(chan->state), timeout);
+
+	l2cap_clear_timer(chan, work);
+
+	l2cap_chan_hold(chan);
+	schedule_delayed_work(work, timeout);
+}
+
#define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
#define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
#define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Hi Andrei,

On Tue, Mar 20, 2012 at 9:51 AM, Andrei Emeltchenko
[off-list ref] wrote:

From: Andrei Emeltchenko <redacted>

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

The description is a lot better, thanks. However, I don't see why this
change is an improvement. The old code could be hard to read but then
we need probably some comments to clarify it, just that IMHO. If you
are solving a real bug I'd very much like to see an oops with a stack
trace or a very good description or test case.

Here the old code just checked if we could cancel the delayed work. If
it returns 0 it means the work wasn't pending at all and then we need
to hold(chan). If it was pending somehow we don't need to hold(chan)
once again.

 static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
-                                       struct delayed_work *work)
+                                    struct delayed_work *work)
 {
       bool ret;

-       ret = cancel_delayed_work(work);
+       ret = (delayed_work_pending(work) && cancel_delayed_work(work));
       if (ret)
               l2cap_chan_put(chan);

       return ret;
 }

The semantic here is the same as the old code, if I'm not missing
anything. If we had a pending work and we could cancel it then we
put(chan). Otherwise either the work wasn't pending and we don't need
to put(chan) or the work was running and it'll put(chan) itself later.
Moreover, just like Mat said we might be introducing a race here, we'd
better check that.

+static inline void l2cap_set_timer(struct l2cap_chan *chan,
+                                  struct delayed_work *work, long timeout)
+{
+       BT_DBG("chan %p state %s timeout %ld", chan,
+              state_to_string(chan->state), timeout);
+
+       l2cap_clear_timer(chan, work);
+
+       l2cap_chan_hold(chan);
+       schedule_delayed_work(work, timeout);
+}
+
 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
 #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
 #define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \
--
1.7.9.1

These are just my thoughts but I don't have the final word if we merge
this code or not. I just think we need to discuss more this kind of
change or even document better why we are changing or solving a bug.
In particular, code that is core to the stack needs more discussion
and care as it may impact several things we don't anticipate when
making a change.

Regards,

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

Hi Ulisses,

On Tue, Mar 20, 2012 at 04:56:11PM -0300, Ulisses Furquim wrote:

Hi Andrei,

On Tue, Mar 20, 2012 at 9:51 AM, Andrei Emeltchenko
[off-list ref] wrote:

quoted

From: Andrei Emeltchenko <redacted>

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

The description is a lot better, thanks. However, I don't see why this
change is an improvement. The old code could be hard to read but then
we need probably some comments to clarify it, just that IMHO. 

Agree with you here.

After further investigation I think that current code is OK, Gustavo could
you revert the patch.

Best regards 
Andrei Emeltchenko 

If you
are solving a real bug I'd very much like to see an oops with a stack
trace or a very good description or test case.

quoted

Signed-off-by: Andrei Emeltchenko <redacted>
---
 include/net/bluetooth/l2cap.h |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9b242c6..e4b2fe7 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h

@@ -621,29 +621,30 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)

       mutex_unlock(&chan->lock);
 }

-static inline void l2cap_set_timer(struct l2cap_chan *chan,
-                                       struct delayed_work *work, long timeout)
-{
-       BT_DBG("chan %p state %s timeout %ld", chan,
-                                       state_to_string(chan->state), timeout);
-
-       if (!cancel_delayed_work(work))
-               l2cap_chan_hold(chan);
-       schedule_delayed_work(work, timeout);
-}

Here the old code just checked if we could cancel the delayed work. If
it returns 0 it means the work wasn't pending at all and then we need
to hold(chan). If it was pending somehow we don't need to hold(chan)
once again.

quoted

 static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
-                                       struct delayed_work *work)
+                                    struct delayed_work *work)
 {
       bool ret;

-       ret = cancel_delayed_work(work);
+       ret = (delayed_work_pending(work) && cancel_delayed_work(work));
       if (ret)
               l2cap_chan_put(chan);

       return ret;
 }

The semantic here is the same as the old code, if I'm not missing
anything. If we had a pending work and we could cancel it then we
put(chan). Otherwise either the work wasn't pending and we don't need
to put(chan) or the work was running and it'll put(chan) itself later.
Moreover, just like Mat said we might be introducing a race here, we'd
better check that.

quoted

+static inline void l2cap_set_timer(struct l2cap_chan *chan,
+                                  struct delayed_work *work, long timeout)
+{
+       BT_DBG("chan %p state %s timeout %ld", chan,
+              state_to_string(chan->state), timeout);
+
+       l2cap_clear_timer(chan, work);
+
+       l2cap_chan_hold(chan);
+       schedule_delayed_work(work, timeout);
+}
+
 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
 #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
 #define __set_retrans_timer(c) l2cap_set_timer(c, &c->retrans_timer, \
--
1.7.9.1

These are just my thoughts but I don't have the final word if we merge
this code or not. I just think we need to discuss more this kind of
change or even document better why we are changing or solving a bug.
In particular, code that is core to the stack needs more discussion
and care as it may impact several things we don't anticipate when
making a change.

Regards,

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

Hi Andrei,

On Wed, Mar 21, 2012 at 5:15 AM, Andrei Emeltchenko
[off-list ref] wrote:

Hi Ulisses,

On Tue, Mar 20, 2012 at 04:56:11PM -0300, Ulisses Furquim wrote:

quoted

Hi Andrei,

On Tue, Mar 20, 2012 at 9:51 AM, Andrei Emeltchenko
[off-list ref] wrote:

quoted

From: Andrei Emeltchenko <redacted>

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

The description is a lot better, thanks. However, I don't see why this
change is an improvement. The old code could be hard to read but then
we need probably some comments to clarify it, just that IMHO.

Agree with you here.

After further investigation I think that current code is OK, Gustavo could
you revert the patch.

Thank you for checking this. What about a patch from you documenting
this? I already saw your commit with the missing _put(chan) in the
workers, which was great, thanks.

Regards,

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

Hi Ulisses,

On Thu, Mar 22, 2012 at 04:00:38PM -0300, Ulisses Furquim wrote:

quoted

quoted

quoted

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

The description is a lot better, thanks. However, I don't see why this
change is an improvement. The old code could be hard to read but then
we need probably some comments to clarify it, just that IMHO.

Agree with you here.

After further investigation I think that current code is OK, Gustavo could
you revert the patch.

Thank you for checking this. What about a patch from you documenting
this? I already saw your commit with the missing _put(chan) in the
workers, which was great, thanks.

I've just sent a patch with comments how timers work.

Best regards 
Andrei Emeltchenko 

Hi Andrei,

On Fri, Mar 23, 2012 at 11:33 AM, Andrei Emeltchenko
[off-list ref] wrote:

Hi Ulisses,

On Thu, Mar 22, 2012 at 04:00:38PM -0300, Ulisses Furquim wrote:

quoted

quoted

quoted

quoted

Simplify L2CAP timers logic. Previous logic was hard to understand.
Now we always hold(chan) when setting up timer and put(chan) only
if work pending and we successfully cancel delayed work.

If delayed work is executing it will put(chan) itself.

The description is a lot better, thanks. However, I don't see why this
change is an improvement. The old code could be hard to read but then
we need probably some comments to clarify it, just that IMHO.

Agree with you here.

After further investigation I think that current code is OK, Gustavo could
you revert the patch.

Thank you for checking this. What about a patch from you documenting
this? I already saw your commit with the missing _put(chan) in the
workers, which was great, thanks.

I've just sent a patch with comments how timers work.

Yes, it looks good, thanks. Marcel was faster and already acked it.

Best regards,

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