Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210901002621.414016-1-luiz.dentz@gmail.com> <9067109F-D8FD-47E9-AD02-FB6B7DFFDB3E@holtmann.org>
In-Reply-To: <9067109F-D8FD-47E9-AD02-FB6B7DFFDB3E@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 1 Sep 2021 12:16:10 -0700
Message-ID: <CABBYNZ+O+HQ7=464AMZBQC5Pe3h-W14AnBs1ufany6ZkKSxLkg@mail.gmail.com>
Subject: Re: [PATCH 1/4] Bluetooth: Add bt_skb_sendmsg helper
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Marcel,

On Tue, Aug 31, 2021 at 10:44 PM Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>
> Hi Luiz,
>
> > bt_skb_sendmsg helps takes care of allocation the skb and copying the
> > the contents of msg over to the skb while checking for possible errors
> > so it should be safe to call it without holding lock_sock.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/bluetooth.h | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/=
bluetooth.h
> > index 9125effbf448..f858efcf9f40 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -420,6 +420,32 @@ static inline struct sk_buff *bt_skb_send_alloc(st=
ruct sock *sk,
> >       return NULL;
> > }
> >
> > +/* Shall not be called with lock_sock held */
> > +static inline struct sk_buff *bt_skb_sendmsg(struct sock *sk,
> > +                                          struct msghdr *msg,
> > +                                          size_t len, size_t header,
> > +                                          size_t footer)
> > +{
> > +     struct sk_buff *skb;
> > +     int err;
> > +
> > +     skb =3D bt_skb_send_alloc(sk, len + header + footer,
> > +                             msg->msg_flags & MSG_DONTWAIT, &err);
> > +     if (!skb)
> > +             return ERR_PTR(err);
> > +
> > +     skb_reserve(skb, header);
>
> I am not with you on this one since bt_skb_send_alloc already calls skb_r=
eserve in the first place.

Afaik skb_reserve can be called multiple since it just add more
headroom/reduce tailroom:

static inline void skb_reserve(struct sk_buff *skb, int len)
{
  skb->data +=3D len;
  skb->tail +=3D len;
}

The RFCOMM does exactly that:

-               skb =3D sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
-                               msg->msg_flags & MSG_DONTWAIT, &err);
-               if (!skb) {
-                       if (sent =3D=3D 0)
-                               sent =3D err;
-                       break;
-               }
-               skb_reserve(skb, RFCOMM_SKB_HEAD_RESERVE);

> /**
>  *      skb_reserve - adjust headroom
>  *      @skb: buffer to alter
>  *      @len: bytes to move
>  *
>  *      Increase the headroom of an empty &sk_buff by reducing the tail
>  *      room. This is only allowed for an empty buffer.
>  */
>
> In addition we have this comment here. So what kind of headroom do we nee=
d with this SKB?

We need the BT_SKB_RESERVE(ACL + L2CAP) +
RFCOMM_SKB_HEAD_RESERVE(RFCOMM header) +
RFCOMM_SKB_TAIL_RESERVE(RFCOMM footer) as tailroom, the len need to
accomodate all of them since they are considered part of the data
(there is no separate storage).

> And wouldn=E2=80=99t it be better to actually assign a tailroom instead o=
f just a large enough buffer?

Yep, though I think the skb_tailroom_reserve does not create a
separate area in the buffer it just marks it so the likes of
__skb_grow can grow the skb, anyway it shall be possible to use it as
well.

> /**
>  *      skb_tailroom_reserve - adjust reserved_tailroom
>  *      @skb: buffer to alter
>  *      @mtu: maximum amount of headlen permitted
>  *      @needed_tailroom: minimum amount of reserved_tailroom
>  *
>  *      Set reserved_tailroom so that headlen can be as large as possible=
 but
>  *      not larger than mtu and tailroom cannot be smaller than
>  *      needed_tailroom.
>  *      The required headroom should already have been reserved before us=
ing
>  *      this function.
>  */
>
> We also have this capability inside the SKBs.
>
> So while the basic idea of this patchset seems fine, we need to figure ou=
t the details and not overload us in spaghetti code by wanting to have a co=
mmon bt_skb_* helper. Maybe it is actually not helpful in this case since w=
e just have to have a too large parameter list to satisfy all 3 users.

Actually having proper helpers for this might actually simplify the
code since then we can enforce that both headroom and tailroom are
properly set as currently each protocol is doing this in its own way.

>
> Regards
>
> Marcel
>


--=20
Luiz Augusto von Dentz