Hi Luiz,
to, 2024-03-14 kello 23:18 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Thu, Mar 14, 2024 at 2:23 PM Pauli Virtanen <pav@xxxxxx> wrote:
> >
> > Add socket option to disable POLLERR on non-empty socket error queue.
> >
> > Applications can use this for sleeping socket error POLLERR waits, also
> > when using TX timestamping. This is useful when multiple layers of the
> > stack are using the same socket.
>
> So the idea here is that bluetoothd would set BT_NO_ERRQUEUE_POLL to
> not be awakened by timestamps? I wonder why this works on per
> fd/socket level while SO_TIMESTAMPING does apply to all sockets
> though, or perhaps that is not really related to SO_TIMESTAMPING but
> errqueue being the same to all cloned sockets? I'm a little hesitant
> to start introducing new socket options if the whole point was to
> avoid doing so with the use of SO_TIMESTAMPING.
It works the same as SO_TIMESTAMPING, setting BT_NO_ERRQUEUE_POLL
applies to all fds referring to the socket, so it is not per fd either.
The idea is that Pipewire sets BT_NO_ERRQUEUE_POLL before enabling
SO_TIMESTAMPING. Then nobody gets POLLERR on errqueue messages.
Instead, it reads MSG_ERRQUEUE always when sending the next packet,
instead of using POLLERR. This works fine in practice and is tested.
I'd appreciate if we can agree on the whole-stack plan.
***
For sockets vs. fds, see net/socket.c:sockfd_lookup
dup() of fd creates new int fd pointing to the same struct file &
struct socket, but not duplicate struct socket/sock, see fs/file.c:1412
Passing fds via unix socket like in DBus seems the same, see
include/net/scm.h:scm_recv_one_fd & fs/file.c:receive_fd
Indeed, you can also see this in debug logs, e.g.
[53573.676885] iso_sock_getname:1103: sock 0000000018c143a2, sk 000000002aff8d2b (from BlueZ)
[53580.831753] iso_sock_getname:1103: sock 00000000419df212, sk 00000000526df659 (from BlueZ)
...
[53592.472038] iso_sock_sendmsg:1127: sock 0000000018c143a2, sk 000000002aff8d2b (from PW)
[53592.472062] iso_sock_sendmsg:1127: sock 00000000419df212, sk 00000000526df659 (from PW)
setsockopt(), errqueue, RX queue etc. are struct socket level stuff.
>
>
> > Signed-off-by: Pauli Virtanen <pav@xxxxxx>
> > ---
> > include/net/bluetooth/bluetooth.h | 9 +++-
> > net/bluetooth/af_bluetooth.c | 72 ++++++++++++++++++++++++++++++-
> > net/bluetooth/iso.c | 8 ++--
> > net/bluetooth/l2cap_sock.c | 8 ++--
> > net/bluetooth/sco.c | 8 ++--
> > 5 files changed, 91 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 9280e72093ee..dcee5384d715 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -240,6 +240,8 @@ struct bt_codecs {
> >
> > #define BT_ISO_BASE 20
> >
> > +#define BT_NO_ERRQUEUE_POLL 21
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > @@ -387,7 +389,8 @@ struct bt_sock {
> > enum {
> > BT_SK_DEFER_SETUP,
> > BT_SK_SUSPEND,
> > - BT_SK_PKT_STATUS
> > + BT_SK_PKT_STATUS,
> > + BT_SK_NO_ERRQUEUE_POLL
> > };
> >
> > struct bt_sock_list {
> > @@ -410,6 +413,10 @@ int bt_sock_stream_recvmsg(struct socket *sock, struct msghdr *msg,
> > size_t len, int flags);
> > __poll_t bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> > int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> > +int bt_sock_setsockopt(struct socket *sock, int level, int optname,
> > + sockptr_t optval, unsigned int optlen);
> > +int bt_sock_getsockopt(struct socket *sock, int level, int optname,
> > + char __user *optval, int __user *optlen);
> > int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> > int bt_sock_wait_ready(struct sock *sk, unsigned int msg_flags);
> >
> > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > index 67604ccec2f4..997197aa7b48 100644
> > --- a/net/bluetooth/af_bluetooth.c
> > +++ b/net/bluetooth/af_bluetooth.c
> > @@ -500,6 +500,12 @@ static inline __poll_t bt_accept_poll(struct sock *parent)
> > return 0;
> > }
> >
> > +static bool bt_sock_error_queue_poll(struct sock *sk)
> > +{
> > + return !test_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags) &&
> > + !skb_queue_empty_lockless(&sk->sk_error_queue);
> > +}
> > +
> > __poll_t bt_sock_poll(struct file *file, struct socket *sock,
> > poll_table *wait)
> > {
> > @@ -511,7 +517,7 @@ __poll_t bt_sock_poll(struct file *file, struct socket *sock,
> > if (sk->sk_state == BT_LISTEN)
> > return bt_accept_poll(sk);
> >
> > - if (sk->sk_err || !skb_queue_empty_lockless(&sk->sk_error_queue))
> > + if (sk->sk_err || bt_sock_error_queue_poll(sk))
> > mask |= EPOLLERR |
> > (sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? EPOLLPRI : 0);
> >
> > @@ -582,6 +588,70 @@ int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
> > }
> > EXPORT_SYMBOL(bt_sock_ioctl);
> >
> > +int bt_sock_setsockopt(struct socket *sock, int level, int optname,
> > + sockptr_t optval, unsigned int optlen)
> > +{
> > + struct sock *sk = sock->sk;
> > + int err = 0;
> > + u32 opt;
> > +
> > + if (level != SOL_BLUETOOTH)
> > + return -ENOPROTOOPT;
> > +
> > + lock_sock(sk);
> > +
> > + switch (optname) {
> > + case BT_NO_ERRQUEUE_POLL:
> > + if (copy_from_sockptr(&opt, optval, sizeof(opt))) {
> > + err = -EFAULT;
> > + break;
> > + }
> > +
> > + if (opt)
> > + set_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags);
> > + else
> > + clear_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags);
> > + break;
> > +
> > + default:
> > + err = -ENOPROTOOPT;
> > + break;
> > + }
> > +
> > + release_sock(sk);
> > + return err;
> > +}
> > +EXPORT_SYMBOL(bt_sock_setsockopt);
> > +
> > +int bt_sock_getsockopt(struct socket *sock, int level, int optname,
> > + char __user *optval, int __user *optlen)
> > +{
> > + struct sock *sk = sock->sk;
> > + int err = 0;
> > + u32 opt;
> > +
> > + if (level != SOL_BLUETOOTH)
> > + return -ENOPROTOOPT;
> > +
> > + lock_sock(sk);
> > +
> > + switch (optname) {
> > + case BT_NO_ERRQUEUE_POLL:
> > + opt = test_bit(BT_SK_NO_ERRQUEUE_POLL, &bt_sk(sk)->flags);
> > + if (put_user(opt, (u32 __user *)optval))
> > + err = -EFAULT;
> > + break;
> > +
> > + default:
> > + err = -ENOPROTOOPT;
> > + break;
> > + }
> > +
> > + release_sock(sk);
> > + return err;
> > +}
> > +EXPORT_SYMBOL(bt_sock_getsockopt);
> > +
> > /* This function expects the sk lock to be held when called */
> > int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
> > {
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index a77ab9df7994..6f5af549a8cc 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1596,8 +1596,8 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_setsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > @@ -1667,8 +1667,8 @@ static int iso_sock_getsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_getsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 9a9f933a748b..06277ce1fd6b 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -697,8 +697,8 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_getsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > @@ -1102,8 +1102,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_setsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > index b3c2af7c7d67..b8b1b314aed4 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -968,8 +968,8 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_setsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > @@ -1211,8 +1211,8 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > default:
> > - err = -ENOPROTOOPT;
> > - break;
> > + release_sock(sk);
> > + return bt_sock_getsockopt(sock, level, optname, optval, optlen);
> > }
> >
> > release_sock(sk);
> > --
> > 2.44.0
> >
> >
>
>
