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

[Marcel Holtmann <marcel@holtmann.org>]

Hi Nick,

>         > > > >> >> >> Right now Bluez always requests flushable ACL
>         packets (but does not
>         > > > >> >> >> set a flush timeout, so effectively they are
>         non-flushable):
>         > > > >> >> >>
>         > > > >> >> >> However it is desirable to use an ACL flush
>         timeout on A2DP packets so
>         > > > >> >> >> that if the ACL packets block for some reason
>         then the LM can flush
>         > > > >> >> >> them to make room for newer packets.
>         > > > >> >> >>
>         > > > >> >> >> Is it reasonable for Bluez to use the 0x00 ACL
>         packet boundary flag by
>         > > > >> >> >> default (non-flushable packet), and let
>         userspace request flushable
>         > > > >> >> >> packets on A2DP L2CAP sockets with the socket
>         option
>         > > > >> >> >> L2CAP_LM_RELIABLE.
>         > > > >> >> >
>         > > > >> >> > the reliable option has a different meaning. It
>         comes back from the old
>         > > > >> >> > Bluetooth 1.1 qualification days where we had to
>         tests on L2CAP that had
>         > > > >> >> > to confirm that we can detect malformed packets
>         and report them. These
>         > > > >> >> > days it is just fine to drop them.
>         > > > >> >>
>         > > > >> >> Got it, how about introducing
>         > > > >> >>
>         > > > >> >> #define L2CAP_LM_FLUSHABLE 0x0040
>         > > > >> >
>         > > > >> > that l2cap_sock_setsockopt_old() sets this didn't
>         give you a hint that
>         > > > >> > we might wanna deprecate this socket options ;)
>         > > > >> >
>         > > > >> > I need to read up on the flushable stuff, but in
>         the end it deserves its
>         > > > >> > own socket option. Also an ioctl() to actually
>         trigger Enhanced flush
>         > > > >> > might be needed.
>         > > > >> >
>         > > > >> >> struct l2cap_pinfo {
>         > > > >> >>    ...
>         > > > >> >>    __u8 flushable;
>         > > > >> >> }
>         > > > >> >
>         > > > >> > Sure. In the long run we need to turn this into a
>         bitmask. We are just
>         > > > >> > wasting memory here.
>         > > > >>
>         > > > >> Attached is an updated patch, that checks the LMP
>         features bitmask
>         > > > >> before using the new non-flushable packet type.
>         > > > >>
>         > > > >> I am still using L2CAP_LM_FLUSHABLE socket option in
>         > > > >> l2cap_sock_setsockopt_old(), which I don't think you
>         are happy with.
>         > > > >> So how about a new option:
>         > > > >>
>         > > > >> SOL_L2CAP, L2CAP_ACL_FLUSH
>         > > > >> which has a default value of 0, and can be set to 1
>         to make the ACL
>         > > > >> data sent by this L2CAP socket flushable.
>         > > >
>         > > > Was this proposal ok?
>         > >
>         > > Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
>         > >
>         > > > >> In a later commit we would then add
>         > > > >> SOL_ACL, ACL_FLUSH_TIMEOUT
>         > > > >> That is used to set an automatic flush timeout for
>         the ACL link on a
>         > > > >> L2CAP socket. Note that SOL_ACL is new.
>         > > > >
>         > > > > can I stop you right here (without even looking at the
>         patch). We do
>         > > > > have the generic SOL_BLUETOOTH that you should be
>         using. So adding
>         > > > > SOL_ACL is not a viable option at all.
>         > > >
>         > > > This would be in a later patch, and SOL_BLUETOOTH,
>         ACL_FLUSH_TIMEOUT
>         > > > is fine too, or whatever you prefer.
>         > >
>         > > Why not just use BT_FLUSHABLE and have it always take a
>         timeout option
>         > > and then 0 means not flushable. And advantage of having it
>         separated?
>         >
>         > I think keeping them separate makes it clear that the flush
>         timeout is
>         > global for a given ACL link, whereas the
>         flushable/non-flushable
>         > boolean is specific to a L2CAP channel. (Which is why I
>         suggested
>         > introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT
>         option -
>         > since this option applies at the ACL level in the stack).
>         >
>         > A specific advantage of this is that flushable packets can
>         be enabled
>         > without over-writing a previous flush timeout that was set
>         on a
>         > different L2CAP socket on the same ACL link. I guess this
>         can also be
>         > achieved with getsockopt() but that is racy.
>         
>         
>         I am talking here about Enhanced Flush support and that would
>         happen on
>         a per ACL handle basis. So it actually almost applies on a per
>         L2CAP
>         socket level. Only exception is if you establish two or more
>         L2CAP
>         connections to the same remote device and set them all to
>         flushable.
>         Then of course all of them will be flushed. So strictly
>         speaking it
>         might be an ACL link feature, but we don't wanna use it that
>         way. And in
>         practice you won't have multiple concurrent flushable L2CAP
>         connections
>         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 of
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 you
don't. Fill me in on how you wanna actually use this feature.

> But its not worth arguing over. SOL_BLUETOOTH, BT_FLUSHABLE is fine
> (or BT_FLUSH_TIMEOUT instead).

I would call it BT_FLUSHABLE as of now. Since that is how the
specification calls it. However I do have to refresh my memory with the
actual details. I haven't read that part of the specification in a long
time.

Regards

Marcel