[PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When sending an i-frame the first time unacked_frames is incremented
which means we are waiting an ack and there is no need to send an ack
since the i-frame itself already serve that purpose.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/l2cap_core.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 09cd860..e969677 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1448,8 +1448,10 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
 
 		chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);
 
-		if (bt_cb(skb)->retries == 1)
+		if (bt_cb(skb)->retries == 1) {
 			chan->unacked_frames++;
+			__clear_ack_timer(chan);
+		}
 
 		chan->frames_sent++;
 
-- 
1.7.7.6

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Ulisses Furquim]

Hi Luiz,

On Mon, Feb 6, 2012 at 12:38 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> When sending an i-frame the first time unacked_frames is incremented
> which means we are waiting an ack and there is no need to send an ack
> since the i-frame itself already serve that purpose.

I'm not sure this message is correct even though the patch makes
sense. We _might_ be acking received unacked frames by sending an
i-frame so it makes sense to clear the ack timer. However,
unacked_frames means our frames which weren't acked yet by the remote
side. Right?

> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 09cd860..e969677 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1448,8 +1448,10 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>
>                chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);
>
> -               if (bt_cb(skb)->retries == 1)
> +               if (bt_cb(skb)->retries == 1) {
>                        chan->unacked_frames++;
> +                       __clear_ack_timer(chan);
> +               }
>
>                chan->frames_sent++;

Hmm, maybe we can clear ack timer only once if we check nsent in the
end of l2cap_ertm_send() instead of potentially call it several times?
Not sure if it's worth it or not, though.

Regards,

-- Ulisses

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Szymon Janc]

Hi,

> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 09cd860..e969677 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1448,8 +1448,10 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
> >
> >                chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);
> >
> > -               if (bt_cb(skb)->retries == 1)
> > +               if (bt_cb(skb)->retries == 1) {
> >                        chan->unacked_frames++;
> > +                       __clear_ack_timer(chan);
> > +               }
> >
> >                chan->frames_sent++;
> 
> Hmm, maybe we can clear ack timer only once if we check nsent in the
> end of l2cap_ertm_send() instead of potentially call it several times?
> Not sure if it's worth it or not, though.

This is what __l2cap_send_ack is doing  i.e. sends RR frame only if
l2cap_ertm_send() returned 0 (or error). Would be good to have this consistent.

So maybe just clear ack timer when incrementing nsent for the 1st time?

if (!nsent++)
	__clear_ack_timer(chan);


-- 
BR
Szymon Janc

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Luiz Augusto von Dentz]

Hi Szymon,

On Tue, Feb 7, 2012 at 10:08 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>>
>> Hmm, maybe we can clear ack timer only once if we check nsent in the
>> end of l2cap_ertm_send() instead of potentially call it several times?
>> Not sure if it's worth it or not, though.
>
> This is what __l2cap_send_ack is doing  i.e. sends RR frame only if
> l2cap_ertm_send() returned 0 (or error). Would be good to have this consistent.
>
> So maybe just clear ack timer when incrementing nsent for the 1st time?
>
> if (!nsent++)
>        __clear_ack_timer(chan);
>

But that means we would be clearing ack timer even for retransmit
frames which Im not sure can be account as an ack?

-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Ulisses Furquim]

Hi Luiz,

On Tue, Feb 7, 2012 at 8:21 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Szymon,
>
> On Tue, Feb 7, 2012 at 10:08 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
>>>
>>> Hmm, maybe we can clear ack timer only once if we check nsent in the
>>> end of l2cap_ertm_send() instead of potentially call it several times?
>>> Not sure if it's worth it or not, though.
>>
>> This is what __l2cap_send_ack is doing  i.e. sends RR frame only if
>> l2cap_ertm_send() returned 0 (or error). Would be good to have this consistent.
>>
>> So maybe just clear ack timer when incrementing nsent for the 1st time?
>>
>> if (!nsent++)
>>        __clear_ack_timer(chan);
>>
>
> But that means we would be clearing ack timer even for retransmit
> frames which Im not sure can be account as an ack?

That's a good point. The retransmitted frames might have an ack, I
think, but only ckecking nsent we won't know that for sure. Thus I do
think the patch from Luiz is better but it's not the right fix IMO.
We'd need to track the last ack sent and then we can check if we have
pending acks.

Seriously, I was wrong suggesting to Luiz that maybe a simple change
in l2cap_ertm_send() to clear ack timer would solve the problem. This
is not true and shows that our code is somewhat messy right now. Let's
try to be more explicit and not rely on implicit conditions or
behavior, please.

Szymon, I've seen you sent patches to handle this before so if you (or
Luiz) could send a patch to introduce tracking of last ack sent it'd
be good. Then we can compute correctly if we have pending acks or not.

Regards,

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

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Szymon Janc]

Hi,
 
> >> Hmm, maybe we can clear ack timer only once if we check nsent in the
> >> end of l2cap_ertm_send() instead of potentially call it several times?
> >> Not sure if it's worth it or not, though.
> >
> > This is what __l2cap_send_ack is doing  i.e. sends RR frame only if
> > l2cap_ertm_send() returned 0 (or error). Would be good to have this consistent.
> >
> > So maybe just clear ack timer when incrementing nsent for the 1st time?
> >
> > if (!nsent++)
> >        __clear_ack_timer(chan);
> >
> 
> But that means we would be clearing ack timer even for retransmit
> frames which Im not sure can be account as an ack?

Right, only pending I-frame(s) is an acknowledgement. But that means
__l2cap_send_ack() is broken.

To fix that, I think l2cap_ertm_send should return number of pending I-frames
transmitted, not a total number of I-frames sent.
I've quickly looked over the code and that change should be OK.

What do you think?

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 09cd860..8dece4e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1448,17 +1448,19 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
 
                chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);
 
-               if (bt_cb(skb)->retries == 1)
+               if (bt_cb(skb)->retries == 1) {
                        chan->unacked_frames++;
 
+                       if (!nsent++)
+                               __clear_ack_timer(chan);
+               }
+
                chan->frames_sent++;
 
                if (skb_queue_is_last(&chan->tx_q, skb))
                        chan->tx_send_head = NULL;
                else
                        chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
-
-               nsent++;
        }
 
        return nsent;


-- 
BR
Szymon Janc

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Ulisses Furquim]

Hi Szymon,

On Tue, Feb 7, 2012 at 9:21 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi,
>
>> >> Hmm, maybe we can clear ack timer only once if we check nsent in the
>> >> end of l2cap_ertm_send() instead of potentially call it several times?
>> >> Not sure if it's worth it or not, though.
>> >
>> > This is what __l2cap_send_ack is doing  i.e. sends RR frame only if
>> > l2cap_ertm_send() returned 0 (or error). Would be good to have this consistent.
>> >
>> > So maybe just clear ack timer when incrementing nsent for the 1st time?
>> >
>> > if (!nsent++)
>> >        __clear_ack_timer(chan);
>> >
>>
>> But that means we would be clearing ack timer even for retransmit
>> frames which Im not sure can be account as an ack?
>
> Right, only pending I-frame(s) is an acknowledgement. But that means
> __l2cap_send_ack() is broken.

Yes, it is broken because it was relying on l2cap_ertm_send()
returning if it sent any i-frame with an ack. And that wasn't
documented anywhere AFAIK and it's very easy for someone else to add a
change that breaks this assumption.

> To fix that, I think l2cap_ertm_send should return number of pending I-frames
> transmitted, not a total number of I-frames sent.

Why? Have you checked if other part of the code relies on
l2cap_ertm_send() returning number of I-frames sent?

> I've quickly looked over the code and that change should be OK.
>
> What do you think?

I've already answered in my other e-mail. I'd like to see us tracking
what we already acked and then easily check if we need to send an ack
or not.

> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 09cd860..8dece4e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1448,17 +1448,19 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>
>                chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);
>
> -               if (bt_cb(skb)->retries == 1)
> +               if (bt_cb(skb)->retries == 1) {
>                        chan->unacked_frames++;
>
> +                       if (!nsent++)
> +                               __clear_ack_timer(chan);
> +               }
> +
>                chan->frames_sent++;
>
>                if (skb_queue_is_last(&chan->tx_q, skb))
>                        chan->tx_send_head = NULL;
>                else
>                        chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
> -
> -               nsent++;
>        }
>
>        return nsent;

Regards,

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

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Szymon Janc]

Hi,

> >> But that means we would be clearing ack timer even for retransmit
> >> frames which Im not sure can be account as an ack?
> >
> > Right, only pending I-frame(s) is an acknowledgement. But that means
> > __l2cap_send_ack() is broken.
> 
> Yes, it is broken because it was relying on l2cap_ertm_send()
> returning if it sent any i-frame with an ack. And that wasn't
> documented anywhere AFAIK and it's very easy for someone else to add a
> change that breaks this assumption.

We can add comment for that. Yet, this is what I assumed when looked at that
code in first place, that it returns number of pending frames sent..

> 
> > To fix that, I think l2cap_ertm_send should return number of pending I-frames
> > transmitted, not a total number of I-frames sent.
> 
> Why? Have you checked if other part of the code relies on
> l2cap_ertm_send() returning number of I-frames sent?

Yes, it looks like only __l2cap_send_ack() is interested if any frames were sent.
l2cap_chan_send is only interested if error occurred or not. Others just ignore
return value.

> 
> > I've quickly looked over the code and that change should be OK.
> >
> > What do you think?
> 
> I've already answered in my other e-mail. I'd like to see us tracking
> what we already acked and then easily check if we need to send an ack
> or not.

Well, if ack timer is running we have sth to ack. Isn't that enough?

-- 
BR
Szymon Janc

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Ulisses Furquim]

Hi,

On Tue, Feb 7, 2012 at 9:02 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi,
>
>> >> But that means we would be clearing ack timer even for retransmit
>> >> frames which Im not sure can be account as an ack?
>> >
>> > Right, only pending I-frame(s) is an acknowledgement. But that means
>> > __l2cap_send_ack() is broken.
>>
>> Yes, it is broken because it was relying on l2cap_ertm_send()
>> returning if it sent any i-frame with an ack. And that wasn't
>> documented anywhere AFAIK and it's very easy for someone else to add a
>> change that breaks this assumption.
>
> We can add comment for that. Yet, this is what I assumed when looked at that
> code in first place, that it returns number of pending frames sent..
>
>>
>> > To fix that, I think l2cap_ertm_send should return number of pending I-frames
>> > transmitted, not a total number of I-frames sent.
>>
>> Why? Have you checked if other part of the code relies on
>> l2cap_ertm_send() returning number of I-frames sent?
>
> Yes, it looks like only __l2cap_send_ack() is interested if any frames were sent.
> l2cap_chan_send is only interested if error occurred or not. Others just ignore
> return value.
>
>>
>> > I've quickly looked over the code and that change should be OK.
>> >
>> > What do you think?
>>
>> I've already answered in my other e-mail. I'd like to see us tracking
>> what we already acked and then easily check if we need to send an ack
>> or not.
>
> Well, if ack timer is running we have sth to ack. Isn't that enough?

That should be the case. When ack timer is running we have something
to ack. The point is answering if we still have pending acks to send.
And that's being answered by the return of calling l2cap_ertm_send()
which was wrong until now and I don't think it's a good idea. Marcel,
any opinions on that? Luiz?

Best regards,

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

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Luiz Augusto von Dentz]

Hi Ulisses,

On Tue, Feb 7, 2012 at 2:37 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>> Well, if ack timer is running we have sth to ack. Isn't that enough?
>
> That should be the case. When ack timer is running we have something
> to ack. The point is answering if we still have pending acks to send.
> And that's being answered by the return of calling l2cap_ertm_send()
> which was wrong until now and I don't think it's a good idea. Marcel,
> any opinions on that? Luiz?

For now I would just try to fix current code upstream without changing
too much the logic and risk more regressions. Im afraid we gonna have
to change quite a bit ertm code for next releases but we have
obexd/obex-client to test regressions so it should be easier to test
this code, so perhaps Szymon fix should be applied before its too late
to do a pull request.

Btw, here is what Ive been using to test this code:

> obexd/tools/test-server -b -c 4097 -p -i 32767 -o 32767

> obexd/tools/test-client -b -s <address of source adapter> -d <address of destination adapter> -c 4097 -p -i 32767 -o 32767

Im using 32767 as MTU because that is what we use by default in OBEX,
but currently it doesn't work due to some bug in ERTM that apparently
doesn't handle MTU being bigger than mts * tx_win, so the transfer
just stall at some point, using something like 16384 works though.

-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Ulisses Furquim]

Hi Luiz,

On Tue, Feb 7, 2012 at 11:28 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Ulisses,
>
> On Tue, Feb 7, 2012 at 2:37 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>>> Well, if ack timer is running we have sth to ack. Isn't that enough?
>>
>> That should be the case. When ack timer is running we have something
>> to ack. The point is answering if we still have pending acks to send.
>> And that's being answered by the return of calling l2cap_ertm_send()
>> which was wrong until now and I don't think it's a good idea. Marcel,
>> any opinions on that? Luiz?
>
> For now I would just try to fix current code upstream without changing
> too much the logic and risk more regressions. Im afraid we gonna have
> to change quite a bit ertm code for next releases but we have
> obexd/obex-client to test regressions so it should be easier to test
> this code, so perhaps Szymon fix should be applied before its too late
> to do a pull request.

Another valid point. I'm ok with Szymon fix for now if we revisit this
when changing ERTM code for next releases. It does need some love.

> Btw, here is what Ive been using to test this code:
>
>> obexd/tools/test-server -b -c 4097 -p -i 32767 -o 32767
>
>> obexd/tools/test-client -b -s <address of source adapter> -d <address of destination adapter> -c 4097 -p -i 32767 -o 32767

Ok. Are you running them on the same machine with 2 dongles?

> Im using 32767 as MTU because that is what we use by default in OBEX,
> but currently it doesn't work due to some bug in ERTM that apparently
> doesn't handle MTU being bigger than mts * tx_win, so the transfer
> just stall at some point, using something like 16384 works though.

Does it stall forever? I have no idea what might be this one.

Regards,

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

Re: [PATCH] Bluetooth: Fix not clearing ack timer when sending an i-frame

[Luiz Augusto von Dentz]

Hi Ulisses,

On Tue, Feb 7, 2012 at 3:45 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>> Btw, here is what Ive been using to test this code:
>>
>>> obexd/tools/test-server -b -c 4097 -p -i 32767 -o 32767
>>
>>> obexd/tools/test-client -b -s <address of source adapter> -d <address of destination adapter> -c 4097 -p -i 32767 -o 32767
>
> Ok. Are you running them on the same machine with 2 dongles?

Yep

>> Im using 32767 as MTU because that is what we use by default in OBEX,
>> but currently it doesn't work due to some bug in ERTM that apparently
>> doesn't handle MTU being bigger than mts * tx_win, so the transfer
>> just stall at some point, using something like 16384 works though.
>
> Does it stall forever? I have no idea what might be this one.

It timeout after 10 seconds than the client disconnects, but I don't
think it would recover even after that since basically each side does
ack with s-frame RR and nothing else happen. I suspect we are going
past what a window could carry (tx_win (63) * mts (300)) because the
MTU is bigger than that the sdu_len can be bigger too, afaik this
would have to be fragmented in different window or perhaps the mts
size should be adjusted too, but lets figure out this in another
thread to avoid too much noise here.

-- 
Luiz Augusto von Dentz