Hi Mat,
* Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2012-02-24 17:08:07 -0800]:
> On 2/24/2012 12:13 PM, Ulisses Furquim wrote:
> >Hi Mat,
> >
> >On Thu, Feb 23, 2012 at 6:37 PM, Mat Martineau<mathewm@xxxxxxxxxxxxxx> wrote:
> >>The previous ERTM implementation had a handler for each frame type,
> >>and each handler had to figure out what the current state was and
> >>handle each frame accordingly.
> >>
> >>This implementation has a state machine that chooses an execution path
> >>first based on the receive or transmit state, then each state has
> >>handlers for the frame types. This is easier to match up with the
> >>spec, which is defined similarly.
> >>
> >>Signed-off-by: Mat Martineau<mathewm@xxxxxxxxxxxxxx>
> >
> ><snip>
> >
> >@@ -1245,7 +1457,8 @@ int __l2cap_wait_ack(struct sock *sk)
> >
> > add_wait_queue(sk_sleep(sk),&wait);
> > set_current_state(TASK_INTERRUPTIBLE);
> >- while (chan->unacked_frames> 0&& chan->conn) {
> >+ while (chan->unacked_frames> 0&& chan->conn&&
> >+ atomic_read(&chan->ertm_queued)) {
> > if (!timeo)
> > timeo = HZ/5;
> >
> >Can we have unacked_frames> 0 and nothing queued? Or have I misinterpreted?
>
> In normal operation, there should be unacked frames when ertm_queued
> is non-zero. I think I ran in to a corner case with AMP, where
> unacked_frames can be forced to 0 during a channel move. There are
> AMP components to the state machine that are not included in this
> patch.
>
> >
> ><snip>
> >
> >+ BT_DBG("Sent txseq %d", (int)control->txseq);
> >
> >- skb = skb_queue_next(&chan->tx_q, skb);
> >+ chan->next_tx_seq = __next_seq(chan->next_tx_seq, chan);
> >+ chan->frames_sent += 1;
> >+ sent += 1;
> >
> >Nitpick here. frames_sent++ and sent++ ? Happens in other places as
> >well so I won't copy all of them here.
>
> Ok, will fix.
>
> >
> ><snip>
> >
> >- if (bt_cb(skb)->retries == 1) {
> >- chan->unacked_frames++;
> >+ l2cap_chan_hold(chan);
> >+ sock_hold(chan->sk);
> >+ tx_skb->sk = chan->sk;
> >
> >Do we really need these? Do we always have chan->sk? (We have that in
> >l2cap_ertm_send() and l2cap_ertm_resend()).
>
> The upstream ERTM code still relies on having chan->sk, so I didn't
> try to finish splitting channels & sockets within this patch. skb
> destructors expect to have an sk pointer, so it is critical to
> modify these reference counts so the socket and chan are around when
> the skb leaves the hci tx queue.
>
> >
> ><snip>
> >
> >+ keep_sk = schedule_work(&chan->tx_work);
> >
> >Would make sense to schedule this in hdev->workqueue?
>
> It's a tradeoff. If this is scheduled on hdev->workqueue, then that
> workqueue can get blocked waiting for the socket lock. If these are
> scheduled on the system workqueue, there is potential for more
> latency (but it hasn't been a problem in practice, even with AMP
> data rates).
>
> >
> ><snip>
> >
> >+static void l2cap_ertm_tx_worker(struct work_struct *work)
> > {
> >
> >Why do we need this worker?
>
> To control the depth of the hci tx queue. Without this, you end up
> with an entire tx window of iframes queued up at the hci layer.
> When an sframe shows up from the remote device and you have to
> retransmit, or when an sframe needs to be sent, then retransmissions
> and sframes have to get queued behind that full tx window of
> iframes. It adds a HUGE amount of latency in those situations,
> which leads to ERTM timeouts and disconnects that are not necessary.
> ERTM works much, much better with lossy connections (like AMP) if it
> does not flood the hci tx queue.
>
> We had a discussion on the list about how to solve this. The idea
> is to push most queuing up to the L2CAP layer, and have the hci
> scheduler call up to L2CAP to fetch frames. However, this hasn't
> been implemented yet.
Ok, I can see why you added tx_work now. We really need to move to the new
proposal here.
Also I think the most important thing here is not point out wrong stuff in
this huge patch but find ways to split it in many patches. If we could get a
cleaner and rebased patch fixing the unnecessary renames and functions move,
etc. that would be good for us. :)
Gustavo
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
