Re: RFC: Allow Bluez to select flushable or non-flushable ACL packets with L2CAP_LM_RELIABLE

[Nick Pelly <npelly@google.com>]

On Fri, Dec 18, 2009 at 6:05 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Nick,
>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> Right now Bluez always requests flusha=
ble ACL
>> >> =A0 =A0 =A0 =A0 packets (but does not
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> set a flush timeout, so effectively th=
ey are
>> >> =A0 =A0 =A0 =A0 non-flushable):
>> >> =A0 =A0 =A0 =A0 > > > >> >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> However it is desirable to use an ACL =
flush
>> >> =A0 =A0 =A0 =A0 timeout on A2DP packets so
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> that if the ACL packets block for some=
 reason
>> >> =A0 =A0 =A0 =A0 then the LM can flush
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> them to make room for newer packets.
>> >> =A0 =A0 =A0 =A0 > > > >> >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> Is it reasonable for Bluez to use the =
0x00 ACL
>> >> =A0 =A0 =A0 =A0 packet boundary flag by
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> default (non-flushable packet), and le=
t
>> >> =A0 =A0 =A0 =A0 userspace request flushable
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> packets on A2DP L2CAP sockets with the=
 socket
>> >> =A0 =A0 =A0 =A0 option
>> >> =A0 =A0 =A0 =A0 > > > >> >> >> L2CAP_LM_RELIABLE.
>> >> =A0 =A0 =A0 =A0 > > > >> >> >
>> >> =A0 =A0 =A0 =A0 > > > >> >> > the reliable option has a different mea=
ning. It
>> >> =A0 =A0 =A0 =A0 comes back from the old
>> >> =A0 =A0 =A0 =A0 > > > >> >> > Bluetooth 1.1 qualification days where =
we had to
>> >> =A0 =A0 =A0 =A0 tests on L2CAP that had
>> >> =A0 =A0 =A0 =A0 > > > >> >> > to confirm that we can detect malformed=
 packets
>> >> =A0 =A0 =A0 =A0 and report them. These
>> >> =A0 =A0 =A0 =A0 > > > >> >> > days it is just fine to drop them.
>> >> =A0 =A0 =A0 =A0 > > > >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> Got it, how about introducing
>> >> =A0 =A0 =A0 =A0 > > > >> >>
>> >> =A0 =A0 =A0 =A0 > > > >> >> #define L2CAP_LM_FLUSHABLE 0x0040
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > that l2cap_sock_setsockopt_old() sets this=
 didn't
>> >> =A0 =A0 =A0 =A0 give you a hint that
>> >> =A0 =A0 =A0 =A0 > > > >> > we might wanna deprecate this socket optio=
ns ;)
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > I need to read up on the flushable stuff, =
but in
>> >> =A0 =A0 =A0 =A0 the end it deserves its
>> >> =A0 =A0 =A0 =A0 > > > >> > own socket option. Also an ioctl() to actu=
ally
>> >> =A0 =A0 =A0 =A0 trigger Enhanced flush
>> >> =A0 =A0 =A0 =A0 > > > >> > might be needed.
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> >> struct l2cap_pinfo {
>> >> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0...
>> >> =A0 =A0 =A0 =A0 > > > >> >> =A0 =A0__u8 flushable;
>> >> =A0 =A0 =A0 =A0 > > > >> >> }
>> >> =A0 =A0 =A0 =A0 > > > >> >
>> >> =A0 =A0 =A0 =A0 > > > >> > Sure. In the long run we need to turn this=
 into a
>> >> =A0 =A0 =A0 =A0 bitmask. We are just
>> >> =A0 =A0 =A0 =A0 > > > >> > wasting memory here.
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> Attached is an updated patch, that checks th=
e LMP
>> >> =A0 =A0 =A0 =A0 features bitmask
>> >> =A0 =A0 =A0 =A0 > > > >> before using the new non-flushable packet ty=
pe.
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> I am still using L2CAP_LM_FLUSHABLE socket o=
ption in
>> >> =A0 =A0 =A0 =A0 > > > >> l2cap_sock_setsockopt_old(), which I don't t=
hink you
>> >> =A0 =A0 =A0 =A0 are happy with.
>> >> =A0 =A0 =A0 =A0 > > > >> So how about a new option:
>> >> =A0 =A0 =A0 =A0 > > > >>
>> >> =A0 =A0 =A0 =A0 > > > >> SOL_L2CAP, L2CAP_ACL_FLUSH
>> >> =A0 =A0 =A0 =A0 > > > >> which has a default value of 0, and can be s=
et to 1
>> >> =A0 =A0 =A0 =A0 to make the ACL
>> >> =A0 =A0 =A0 =A0 > > > >> data sent by this L2CAP socket flushable.
>> >> =A0 =A0 =A0 =A0 > > >
>> >> =A0 =A0 =A0 =A0 > > > Was this proposal ok?
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for t=
his.
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > > >> In a later commit we would then add
>> >> =A0 =A0 =A0 =A0 > > > >> SOL_ACL, ACL_FLUSH_TIMEOUT
>> >> =A0 =A0 =A0 =A0 > > > >> That is used to set an automatic flush timeo=
ut for
>> >> =A0 =A0 =A0 =A0 the ACL link on a
>> >> =A0 =A0 =A0 =A0 > > > >> L2CAP socket. Note that SOL_ACL is new.
>> >> =A0 =A0 =A0 =A0 > > > >
>> >> =A0 =A0 =A0 =A0 > > > > can I stop you right here (without even looki=
ng at the
>> >> =A0 =A0 =A0 =A0 patch). We do
>> >> =A0 =A0 =A0 =A0 > > > > have the generic SOL_BLUETOOTH that you shoul=
d be
>> >> =A0 =A0 =A0 =A0 using. So adding
>> >> =A0 =A0 =A0 =A0 > > > > SOL_ACL is not a viable option at all.
>> >> =A0 =A0 =A0 =A0 > > >
>> >> =A0 =A0 =A0 =A0 > > > This would be in a later patch, and SOL_BLUETOO=
TH,
>> >> =A0 =A0 =A0 =A0 ACL_FLUSH_TIMEOUT
>> >> =A0 =A0 =A0 =A0 > > > is fine too, or whatever you prefer.
>> >> =A0 =A0 =A0 =A0 > >
>> >> =A0 =A0 =A0 =A0 > > Why not just use BT_FLUSHABLE and have it always =
take a
>> >> =A0 =A0 =A0 =A0 timeout option
>> >> =A0 =A0 =A0 =A0 > > and then 0 means not flushable. And advantage of =
having it
>> >> =A0 =A0 =A0 =A0 separated?
>> >> =A0 =A0 =A0 =A0 >
>> >> =A0 =A0 =A0 =A0 > I think keeping them separate makes it clear that t=
he flush
>> >> =A0 =A0 =A0 =A0 timeout is
>> >> =A0 =A0 =A0 =A0 > global for a given ACL link, whereas the
>> >> =A0 =A0 =A0 =A0 flushable/non-flushable
>> >> =A0 =A0 =A0 =A0 > boolean is specific to a L2CAP channel. (Which is w=
hy I
>> >> =A0 =A0 =A0 =A0 suggested
>> >> =A0 =A0 =A0 =A0 > introducing a new level SOL_ACL for the ACL_FLUSH_T=
IMEOUT
>> >> =A0 =A0 =A0 =A0 option -
>> >> =A0 =A0 =A0 =A0 > since this option applies at the ACL level in the s=
tack).
>> >> =A0 =A0 =A0 =A0 >
>> >> =A0 =A0 =A0 =A0 > A specific advantage of this is that flushable pack=
ets can
>> >> =A0 =A0 =A0 =A0 be enabled
>> >> =A0 =A0 =A0 =A0 > without over-writing a previous flush timeout that =
was set
>> >> =A0 =A0 =A0 =A0 on a
>> >> =A0 =A0 =A0 =A0 > different L2CAP socket on the same ACL link. I gues=
s this
>> >> =A0 =A0 =A0 =A0 can also be
>> >> =A0 =A0 =A0 =A0 > achieved with getsockopt() but that is racy.
>> >>
>> >>
>> >> =A0 =A0 =A0 =A0 I am talking here about Enhanced Flush support and th=
at would
>> >> =A0 =A0 =A0 =A0 happen on
>> >> =A0 =A0 =A0 =A0 a per ACL handle basis. So it actually almost applies=
 on a per
>> >> =A0 =A0 =A0 =A0 L2CAP
>> >> =A0 =A0 =A0 =A0 socket level. Only exception is if you establish two =
or more
>> >> =A0 =A0 =A0 =A0 L2CAP
>> >> =A0 =A0 =A0 =A0 connections to the same remote device and set them al=
l to
>> >> =A0 =A0 =A0 =A0 flushable.
>> >> =A0 =A0 =A0 =A0 Then of course all of them will be flushed. So strict=
ly
>> >> =A0 =A0 =A0 =A0 speaking it
>> >> =A0 =A0 =A0 =A0 might be an ACL link feature, but we don't wanna use =
it that
>> >> =A0 =A0 =A0 =A0 way. And in
>> >> =A0 =A0 =A0 =A0 practice you won't have multiple concurrent flushable=
 L2CAP
>> >> =A0 =A0 =A0 =A0 connections
>> >> =A0 =A0 =A0 =A0 to one remote device anyway.
>> >>
>> >>
>> >> I agree that having 2 flush-able L2CAP channels to the same device
>> >> would probably not be common. But who knows what new profiles the
>> >> Bluetooth SIG will come up with that might also benefit from
>> >> flush-able ACL data. And if a use-case comes up, then your proposed
>> >> API will require programmers to write a racy getsockopt/setsockopt if
>> >> they want to turn on flushing on one l2cap connection without
>> >> affecting the ACL flush timeout set by another connection. Building
>> >> race conditions into an API seems like a sub-optimal design choice.
>> >
>> > are you expecting to change this frequently and from different parts o=
f
>> > the code during the lifetime of a socket. I just don't see that
>> > happening at all actually. Either you create a "flushable" socket or y=
ou
>> > don't. Fill me in on how you wanna actually use this feature.
>>
>> My use case is just for A2DP. I turn on flushing with a timeout of say
>> 160ms just before starting streaming of A2DP data, and turn it off
>> when I finish. This is not a problem with either API proposal.
>
> I count this as creating socket, setting flushable and then using it.
> Then closing it. And especially in A2DP case where the media socket is
> brought up and taken down a lot that is a proper usage. However I do
> expect that each socket should not change from flushable to
> non-flushable in mid term usage. While potentially possible it don't see
> its usage at all.
>
> So we could even force the flushable option into non-changeable after
> the socket has been connected. Like changing the MTU afterwards makes no
> sense.
>
>> Where it becomes a problem is if there is a reason to have two
>> flush-able L2CAP connections to the same host. With your API proposal,
>> the second connection has no way of turning on flushing without
>> over-writing the flush timeout set by the first socket. You could
>> implement another API to read the current flush timeout, and have the
>> second socket read that API, but thats racy.
>>
>> If this is not a use-case you care about, then ok. But I just want to
>> point out that this is a problem that will be baked into the API - and
>> will require ugly workarounds in userspace as soon as someone requires
>> 2 flushable L2CAP connections to one host. Given the rate at which
>> Bluetooth changes and new profiles and use cases are added I would not
>> be so quick to dismiss this use case.
>
> So my idea would actually be that every socket can has its own flush
> timeout, but the core than picks the time to actually do the flushing of
> packets. Also we can not have one socket change a socket option of
> another one. It is a per socket option and not a global one.

I think you are confused. This patch does not implement HCI Enhance
Flush Command. The flush timeout that I am referring to is passed to
the Bluetooth Chipset with the HCI Write Automatic Flush Timeout
command. Which is why it is global for the ACL link.

>
> On other possible way would be to use CMSG details to inform sockets
> about flushable packets. We have to see how useful that is. Since the
> flushable is only useful for the time in between the packet is hold in
> the Bluetooth chip buffers and hasn't been transmitted over the air.
> Once the packet is on the air, there is nothing to flush anymore. And
> with L2CAP ERTM this all becomes obsolete since we can flush at any time
> anyway. The retransmission takes care of any accidental flush.
>
> Regards
>
> Marcel
>
>
>