[PATCH] Bluetooth: Simplify L2CAP timer logic

[PATCH] Bluetooth: Simplify L2CAP timer logic

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

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 <andrei.emeltchenko@intel.com>
---
 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

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-03-20 14:51:08 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> 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 <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/l2cap.h |   27 ++++++++++++++-------------
>  1 files changed, 14 insertions(+), 13 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Mat Martineau]

On Tue, 20 Mar 2012, Andrei Emeltchenko wrote:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> 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 <andrei.emeltchenko@intel.com>
> ---
> 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));

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

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Ulisses Furquim]

Hi Andrei,

On Tue, Mar 20, 2012 at 9:51 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> 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.

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  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.

>  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

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Andrei Emeltchenko]

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
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > 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.
> 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  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.
> 
> >  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

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Ulisses Furquim]

Hi Andrei,

On Wed, Mar 21, 2012 at 5:15 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> >
>> > 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

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Andrei Emeltchenko]

Hi Ulisses,

On Thu, Mar 22, 2012 at 04:00:38PM -0300, Ulisses Furquim wrote:
> >> > 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 

Re: [PATCH] Bluetooth: Simplify L2CAP timer logic

[Ulisses Furquim]

Hi Andrei,

On Fri, Mar 23, 2012 at 11:33 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Thu, Mar 22, 2012 at 04:00:38PM -0300, Ulisses Furquim wrote:
>> >> > 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