Re: [PATCH v6 1/4] Bluetooth: hci_sock: Add support for BT_{SND,RCV}BUF

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Marcel,

On Wed, Sep 22, 2021 at 7:19 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>> This adds support for BT_{SND,RCV}BUF so userspace can set MTU based on
> >>> the channel usage.
> >>>
> >>> Fixes: https://github.com/bluez/bluez/issues/201
> >>>
> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>> ---
> >>> net/bluetooth/hci_sock.c | 102 ++++++++++++++++++++++++++++++++++-----
> >>> 1 file changed, 91 insertions(+), 11 deletions(-)
> >>
> >> so I applied patches 1, 3 and 4 to bluetooth-next tree.
> >>
> >> The patch 2 needs a more details review when I have time since I remember there were cases where the SKB copy was really needed.
> >
> > Is that something not covered by CI testing? Note we still have a copy
> > done internally with create_monitor_ctrl_command.
>
> there are cases where the SKB handed down is in hdev->send() and that one is owned by the driver to make whatever modifications to headroom it pleases. So if the stack needs to send it out via other sockets, it needs a copy.
>
> We can optimize here, but need to be careful.

I guess you are referring to instances of hci_send_to_channel, which
appears there only one instance that is changed in the diff bellow:

        if (chan->channel == HCI_CHANNEL_CONTROL) {
-               struct sk_buff *skb;
+               struct sk_buff *cmd;

                /* Send event to monitor */
-               skb = create_monitor_ctrl_command(sk, index, opcode, len,
-                                                 buf + sizeof(*hdr));
-               if (skb) {
-                       hci_send_to_channel(HCI_CHANNEL_MONITOR, skb,
+               cmd = create_monitor_ctrl_command(sk, index, opcode, len,
+                                                 skb->data + sizeof(*hdr));
+               if (cmd) {
+                       hci_send_to_channel(HCI_CHANNEL_MONITOR, cmd,
                                            HCI_SOCK_TRUSTED, NULL);
-                       kfree_skb(skb);
+                       kfree_skb(cmd);
                }
        }

Ive only changed the variable name since the original code use skb
which is now used as the original data instead of buf (extra copy), so
nothing has changed in this regard, also running this with KSAN, etc,
doesn't seem to produce any traces nor there seems to be anything
wrong in btmon either.

Note with this we are eliminating the extra copy on buf:

msg -> skb (new) vs msg-> buf -> skb (old)

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz