Re: [RFC 3/5 v2] Bluetooth: make use sk_priority to priritize RFCOMM packets

[Marcel Holtmann <marcel@holtmann.org>]

Hi Luiz,

> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >> > ---
> >> >  net/bluetooth/rfcomm/core.c |   51 +++++++++++++++++++++++++++++-------------
> >> >  net/bluetooth/rfcomm/sock.c |    2 +
> >> >  2 files changed, 37 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> >> > index 85580f2..bfc6bce 100644
> >> > --- a/net/bluetooth/rfcomm/core.c
> >> > +++ b/net/bluetooth/rfcomm/core.c
> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
> >> >
> >> >  static LIST_HEAD(session_list);
> >> >
> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > +                                                   u32 priority);
> >> >  static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci);
> >> >  static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci);
> >> >  static int rfcomm_queue_disc(struct rfcomm_dlc *d);
> >> > @@ -747,19 +748,34 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d
> >> >  }
> >> >
> >> >  /* ---- RFCOMM frame sending ---- */
> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len)
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > +                                                   u32 priority)
> >> >  {
> >> >     struct socket *sock = s->sock;
> >> > +   struct sock *sk = sock->sk;
> >> >     struct kvec iv = { data, len };
> >> >     struct msghdr msg;
> >> >
> >> > -   BT_DBG("session %p len %d", s, len);
> >> > +   BT_DBG("session %p len %d priority %u", s, len, priority);
> >> > +
> >> > +   if (sk->sk_priority != priority) {
> >> > +           lock_sock(sk);
> >> > +           sk->sk_priority = priority;
> >> > +           release_sock(sk);
> >> > +   }
> >> >
> >> >     memset(&msg, 0, sizeof(msg));
> >> >
> >> >     return kernel_sendmsg(sock, &msg, &iv, 1, len);
> >> >  }
> >> >
> >> > +static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd)
> >> > +{
> >> > +   BT_DBG("%p cmd %u", s, cmd->ctrl);
> >> > +
> >> > +   return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX);
> >>
> >>
> >> What's the idea here? Prioritize commands over data? But does this really
> >> happen? Because we have only one queue to receive the data in L2CAP. There
> >> is no separation between data and cmd there.
> >>
> >> Also, did you check if we can send RFCOMM commands and data out of order?
> >>
> >> I really would like to rewrite l2cap-rfcomm iteraction before adding new
> >> features here.
> >
> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
> > return an error code. Priority on RFCOMM links is also rather pointless
> > since they all go via the same L2CAP PSM anyway. You would end up
> > prioritizing all RFCOMM connections over any other L2CAP connection.
> 
> Currently the priority is set per skb not per channel, so it is not as
> broken as you may think it is. Other than that you can't really ignore
> the priority for RFCOMM because as the priority will be not set in its
> skb it will be left 0 so it is low priority which for RFCOMM commands
> may cause problems due latency being increased (remember it not a
> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
> specific and currently no error is return.

and we have to super careful with any sort of potential re-ordering
here. I rather not touch SKBs coming in from RFCOMM. They should stay as
they are.

> > So if you try to prioritize HFP then you also prioritize PBAP in the end
> > and we are back where we started. We could implement the 27.007 priority
> > support inside RFCOMM, but that seems even more pointless endeavor.
> 
> That is not the way it work, take a look at rfcomm_send_frame:
> 
> +       if (sk->sk_priority != priority) {
> +               lock_sock(sk);
> +               sk->sk_priority = priority;
> +               release_sock(sk);
> +       }
> 
> This is suppose to dynamically updates the priority if it changes.

As I said above, I rather not mess with this. Keep the RFCOMM stream as
it is. L2CAP priority is essentially something different than RFCOMM
priority. And I do not wanna go there right now.

> > What we really want is prioritized L2CAP links and hopefully in the
> > future everything moves over to use L2CAP directly anyway and RFCOMM
> > will be slowly dying out.
> 
> The problem here is not really RFCOMM as for L2CAP we also give
> maximum priority to commands to avoid latency problems, this is
> specially important when connecting because it will page but if either
> L2CAP or RFCOMM commands are not properly prioritized they may timeout
> so all the paging is wasted, btw this was very easy to emulate with
> hid+a2dp then try to connect anything else.

As pointed our earlier, we have to be really careful to not reorder
command wrongly. If that happens we have a serious problem with the
overall system.

Regards

Marcel