Hi Gustavo,
On Tue, Sep 20, 2011 at 7:59 PM, Gustavo Padovan <padovan@xxxxxxxxxxxxxx> wrote:
> Hi Luiz,
>
> * Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> [2011-09-20 19:06:30 +0300]:
>
>> Hi Marcel,
>>
>> On Tue, Sep 20, 2011 at 6:11 PM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>> > Hi Luiz,
>> >
>> >> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>> >> >> > ---
>> >> >> > 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.
>>
>> The order is kept, and Im not sure how you want the queue discipline
>> to work but afaik that how other implementation handle it, by setting
>> skb->priority. The other option is to have the hci_chan handling the
>> priority, but that way it will just work as you assume per channel and
>> that cannot support RFCOMM at all.
>
> But in this way we will prioritize all RFCOMM channel at once, and this is not
> the behaviour we want. It will be pointless if we all rfcomm based profiles
> are updated in the priority queue at the same time.
Not sure what to meant here, RFCOMM sockets should not support any
priority according to Marcel, it would be always 0, so there is no
updates or anything. The idea is just to make the code simpler since
RFCOMM would not be supported there is no clear benefit of doing the
prioritization per buffer, I still think that command latency for
RFCOMM can be a problem in some cases and that why Ive have the
priority per skb in the first place so we could properly prioritize
them, but as an option we can have it first per hci_chan and then
latter if needed have per skb.
--
Luiz Augusto von Dentz
--
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
