[v3,1/1] Bluetooth: Prioritize SCO traffic

When scheduling TX packets, send all SCO/eSCO packets first, check for
pending SCO/eSCO packets after every ACL/LE packet and send them if any
are pending.  This is done to make sure that we can meet SCO deadlines
on slow interfaces like UART.

If we were to queue up multiple ACL packets without checking for a SCO
packet, we might miss the SCO timing. For example:

The time it takes to send a maximum size ACL packet (1024 bytes):
t = 10/8 * 1024 bytes * 8 bits/byte * 1 packet / baudrate
        where 10/8 is uart overhead due to start/stop bits per byte

Replace t = 3.75ms (SCO deadline), which gives us a baudrate of 2730666.

At a baudrate of 3000000, if we didn't check for SCO packets within 1024
bytes, we would miss the 3.75ms timing window.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v3:
* Removed hci_sched_sync

Changes in v2:
* Refactor to check for SCO/eSCO after each ACL/LE packet sent
* Enabled SCO priority all the time and removed the sched_limit variable

 net/bluetooth/hci_core.c | 106 +++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

Hi Marcel,

Reminder to review this patch.

Thanks,
Abhishek

On Mon, Mar 23, 2020 at 12:45 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> When scheduling TX packets, send all SCO/eSCO packets first, check for
> pending SCO/eSCO packets after every ACL/LE packet and send them if any
> are pending.  This is done to make sure that we can meet SCO deadlines
> on slow interfaces like UART.
>
> If we were to queue up multiple ACL packets without checking for a SCO
> packet, we might miss the SCO timing. For example:
>
> The time it takes to send a maximum size ACL packet (1024 bytes):
> t = 10/8 * 1024 bytes * 8 bits/byte * 1 packet / baudrate
>         where 10/8 is uart overhead due to start/stop bits per byte
>
> Replace t = 3.75ms (SCO deadline), which gives us a baudrate of 2730666.
>
> At a baudrate of 3000000, if we didn't check for SCO packets within 1024
> bytes, we would miss the 3.75ms timing window.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in v3:
> * Removed hci_sched_sync
>
> Changes in v2:
> * Refactor to check for SCO/eSCO after each ACL/LE packet sent
> * Enabled SCO priority all the time and removed the sched_limit variable
>
>  net/bluetooth/hci_core.c | 106 +++++++++++++++++++++------------------
>  1 file changed, 57 insertions(+), 49 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index dbd2ad3a26ed..9e5d7662a047 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -4239,6 +4239,54 @@ static void __check_timeout(struct hci_dev *hdev, unsigned int cnt)
>         }
>  }
>
> +/* Schedule SCO */
> +static void hci_sched_sco(struct hci_dev *hdev)
> +{
> +       struct hci_conn *conn;
> +       struct sk_buff *skb;
> +       int quote;
> +
> +       BT_DBG("%s", hdev->name);
> +
> +       if (!hci_conn_num(hdev, SCO_LINK))
> +               return;
> +
> +       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
> +               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> +                       BT_DBG("skb %p len %d", skb, skb->len);
> +                       hci_send_frame(hdev, skb);
> +
> +                       conn->sent++;
> +                       if (conn->sent == ~0)
> +                               conn->sent = 0;
> +               }
> +       }
> +}
> +
> +static void hci_sched_esco(struct hci_dev *hdev)
> +{
> +       struct hci_conn *conn;
> +       struct sk_buff *skb;
> +       int quote;
> +
> +       BT_DBG("%s", hdev->name);
> +
> +       if (!hci_conn_num(hdev, ESCO_LINK))
> +               return;
> +
> +       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
> +                                                    &quote))) {
> +               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> +                       BT_DBG("skb %p len %d", skb, skb->len);
> +                       hci_send_frame(hdev, skb);
> +
> +                       conn->sent++;
> +                       if (conn->sent == ~0)
> +                               conn->sent = 0;
> +               }
> +       }
> +}
> +
>  static void hci_sched_acl_pkt(struct hci_dev *hdev)
>  {
>         unsigned int cnt = hdev->acl_cnt;
> @@ -4270,6 +4318,10 @@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
>                         hdev->acl_cnt--;
>                         chan->sent++;
>                         chan->conn->sent++;
> +
> +                       /* Send pending SCO packets right away */
> +                       hci_sched_sco(hdev);
> +                       hci_sched_esco(hdev);
>                 }
>         }
>
> @@ -4354,54 +4406,6 @@ static void hci_sched_acl(struct hci_dev *hdev)
>         }
>  }
>
> -/* Schedule SCO */
> -static void hci_sched_sco(struct hci_dev *hdev)
> -{
> -       struct hci_conn *conn;
> -       struct sk_buff *skb;
> -       int quote;
> -
> -       BT_DBG("%s", hdev->name);
> -
> -       if (!hci_conn_num(hdev, SCO_LINK))
> -               return;
> -
> -       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
> -               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -                       BT_DBG("skb %p len %d", skb, skb->len);
> -                       hci_send_frame(hdev, skb);
> -
> -                       conn->sent++;
> -                       if (conn->sent == ~0)
> -                               conn->sent = 0;
> -               }
> -       }
> -}
> -
> -static void hci_sched_esco(struct hci_dev *hdev)
> -{
> -       struct hci_conn *conn;
> -       struct sk_buff *skb;
> -       int quote;
> -
> -       BT_DBG("%s", hdev->name);
> -
> -       if (!hci_conn_num(hdev, ESCO_LINK))
> -               return;
> -
> -       while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK,
> -                                                    &quote))) {
> -               while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
> -                       BT_DBG("skb %p len %d", skb, skb->len);
> -                       hci_send_frame(hdev, skb);
> -
> -                       conn->sent++;
> -                       if (conn->sent == ~0)
> -                               conn->sent = 0;
> -               }
> -       }
> -}
> -
>  static void hci_sched_le(struct hci_dev *hdev)
>  {
>         struct hci_chan *chan;
> @@ -4436,6 +4440,10 @@ static void hci_sched_le(struct hci_dev *hdev)
>                         cnt--;
>                         chan->sent++;
>                         chan->conn->sent++;
> +
> +                       /* Send pending SCO packets right away */
> +                       hci_sched_sco(hdev);
> +                       hci_sched_esco(hdev);
>                 }
>         }
>
> @@ -4458,9 +4466,9 @@ static void hci_tx_work(struct work_struct *work)
>
>         if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
>                 /* Schedule queues and send stuff to HCI driver */
> -               hci_sched_acl(hdev);
>                 hci_sched_sco(hdev);
>                 hci_sched_esco(hdev);
> +               hci_sched_acl(hdev);
>                 hci_sched_le(hdev);
>         }
>
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Hi Abhishek,

> When scheduling TX packets, send all SCO/eSCO packets first, check for
> pending SCO/eSCO packets after every ACL/LE packet and send them if any
> are pending.  This is done to make sure that we can meet SCO deadlines
> on slow interfaces like UART.
> 
> If we were to queue up multiple ACL packets without checking for a SCO
> packet, we might miss the SCO timing. For example:
> 
> The time it takes to send a maximum size ACL packet (1024 bytes):
> t = 10/8 * 1024 bytes * 8 bits/byte * 1 packet / baudrate
>        where 10/8 is uart overhead due to start/stop bits per byte
> 
> Replace t = 3.75ms (SCO deadline), which gives us a baudrate of 2730666.
> 
> At a baudrate of 3000000, if we didn't check for SCO packets within 1024
> bytes, we would miss the 3.75ms timing window.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v3:
> * Removed hci_sched_sync
> 
> Changes in v2:
> * Refactor to check for SCO/eSCO after each ACL/LE packet sent
> * Enabled SCO priority all the time and removed the sched_limit variable
> 
> net/bluetooth/hci_core.c | 106 +++++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 49 deletions(-)

patch has been applied to bluetooth-next tree.

However I have been a bit reluctant to apply this right away. I think when this code was originally written, we only had ACL and SCO packets. The world was pretty simple. And right now we also only have two packets types (ignoring ISO packets for now), but we added LE and eSCO as separate scheduling and thus “fake” packet types.

I have the feeling that this serialized packet processing will get us into trouble since we prioritize BR/EDR packets over LE packets and SCO over eSCO. I think we should have looked at all packets based on SO_PRIORITY and with ISO packets we have to most likely re-design this. Anyway, just something to think about.

Regards

Marcel

Hi Marcel,

Thanks for merging.

I agree that the distinction between SCO/eSCO and ACL/LE is a bit
concerning for scheduling. I will make some time to revisit this as
part of Audio improvements we are making.

Thanks
Abhishek

Abhishek

On Thu, Apr 2, 2020 at 11:56 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > When scheduling TX packets, send all SCO/eSCO packets first, check for
> > pending SCO/eSCO packets after every ACL/LE packet and send them if any
> > are pending.  This is done to make sure that we can meet SCO deadlines
> > on slow interfaces like UART.
> >
> > If we were to queue up multiple ACL packets without checking for a SCO
> > packet, we might miss the SCO timing. For example:
> >
> > The time it takes to send a maximum size ACL packet (1024 bytes):
> > t = 10/8 * 1024 bytes * 8 bits/byte * 1 packet / baudrate
> >        where 10/8 is uart overhead due to start/stop bits per byte
> >
> > Replace t = 3.75ms (SCO deadline), which gives us a baudrate of 2730666.
> >
> > At a baudrate of 3000000, if we didn't check for SCO packets within 1024
> > bytes, we would miss the 3.75ms timing window.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v3:
> > * Removed hci_sched_sync
> >
> > Changes in v2:
> > * Refactor to check for SCO/eSCO after each ACL/LE packet sent
> > * Enabled SCO priority all the time and removed the sched_limit variable
> >
> > net/bluetooth/hci_core.c | 106 +++++++++++++++++++++------------------
> > 1 file changed, 57 insertions(+), 49 deletions(-)
>
> patch has been applied to bluetooth-next tree.
>
> However I have been a bit reluctant to apply this right away. I think when this code was originally written, we only had ACL and SCO packets. The world was pretty simple. And right now we also only have two packets types (ignoring ISO packets for now), but we added LE and eSCO as separate scheduling and thus “fake” packet types.
>
> I have the feeling that this serialized packet processing will get us into trouble since we prioritize BR/EDR packets over LE packets and SCO over eSCO. I think we should have looked at all packets based on SO_PRIORITY and with ISO packets we have to most likely re-design this. Anyway, just something to think about.
>
> Regards
>
> Marcel
>

On Fri, Apr 3, 2020 at 11:11 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Marcel,
>
> Thanks for merging.
>
> I agree that the distinction between SCO/eSCO and ACL/LE is a bit
> concerning for scheduling. I will make some time to revisit this as
> part of Audio improvements we are making.

A) I know nothing of bluetooth.
B) I am unfond of strict priority queues, as they can cause
starvation. My immediate instinct is to reach for a drr++ derived
solution
to give fairness to all flows, and a bit of priority to the ones that
matter most.


>
> Thanks
> Abhishek
>
> Abhishek
>
> On Thu, Apr 2, 2020 at 11:56 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Abhishek,
> >
> > > When scheduling TX packets, send all SCO/eSCO packets first, check for
> > > pending SCO/eSCO packets after every ACL/LE packet and send them if any
> > > are pending.  This is done to make sure that we can meet SCO deadlines
> > > on slow interfaces like UART.
> > >
> > > If we were to queue up multiple ACL packets without checking for a SCO
> > > packet, we might miss the SCO timing. For example:
> > >
> > > The time it takes to send a maximum size ACL packet (1024 bytes):
> > > t = 10/8 * 1024 bytes * 8 bits/byte * 1 packet / baudrate
> > >        where 10/8 is uart overhead due to start/stop bits per byte
> > >
> > > Replace t = 3.75ms (SCO deadline), which gives us a baudrate of 2730666.
> > >
> > > At a baudrate of 3000000, if we didn't check for SCO packets within 1024
> > > bytes, we would miss the 3.75ms timing window.
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > > Changes in v3:
> > > * Removed hci_sched_sync
> > >
> > > Changes in v2:
> > > * Refactor to check for SCO/eSCO after each ACL/LE packet sent
> > > * Enabled SCO priority all the time and removed the sched_limit variable
> > >
> > > net/bluetooth/hci_core.c | 106 +++++++++++++++++++++------------------
> > > 1 file changed, 57 insertions(+), 49 deletions(-)
> >
> > patch has been applied to bluetooth-next tree.
> >
> > However I have been a bit reluctant to apply this right away. I think when this code was originally written, we only had ACL and SCO packets. The world was pretty simple. And right now we also only have two packets types (ignoring ISO packets for now), but we added LE and eSCO as separate scheduling and thus “fake” packet types.
> >
> > I have the feeling that this serialized packet processing will get us into trouble since we prioritize BR/EDR packets over LE packets and SCO over eSCO. I think we should have looked at all packets based on SO_PRIORITY and with ISO packets we have to most likely re-design this. Anyway, just something to think about.
> >
> > Regards
> >
> > Marcel
> >