* [PATCH BlueZ 0/3] Add io watcher to be used with TX timestamping
@ 2025-02-09 10:46 Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-09 10:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
This is rebase of a patchset sent earlier, for making BlueZ tolerate
TX timestamps in MSG_ERRQUEUE so it does not mistake them for errors.
This was one of the two ways to deal with this issue. The other one
would add a kernel sockopt() that disables (some or all) errqueue
wakeups. For that, it might make sense to add it not only for
Bluetooth, but I did not work on this, so no such patch exists.
Don't know which one of these would be the preferable one.
Pauli Virtanen (3):
shared/io-glib: add watcher to be used with TX timestamping
avdtp: don't consider TX timestamps as errors
bap: don't consider TX timestamps as errors
Makefile.am | 1 +
acinclude.m4 | 3 +-
configure.ac | 2 +-
profiles/audio/avdtp.c | 5 +-
profiles/audio/bap.c | 14 ++--
src/shared/bap.c | 3 +
src/shared/io-glib.c | 157 ++++++++++++++++++++++++++++++++++++++++-
src/shared/io-glib.h | 20 ++++++
8 files changed, 195 insertions(+), 10 deletions(-)
create mode 100644 src/shared/io-glib.h
--
2.48.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH BlueZ 1/3] shared/io-glib: add watcher to be used with TX timestamping
2025-02-09 10:46 [PATCH BlueZ 0/3] Add io watcher to be used with TX timestamping Pauli Virtanen
@ 2025-02-09 10:46 ` Pauli Virtanen
2025-02-09 11:12 ` Add io " bluez.test.bot
` (2 more replies)
2025-02-09 10:46 ` [PATCH BlueZ 2/3] avdtp: don't consider TX timestamps as errors Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 3/3] bap: " Pauli Virtanen
2 siblings, 3 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-09 10:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Add special implementation of fd watcher GSource for audio use.
For audio use cases, sound server may turn on TX timestamping on a
socket that we are watching. In this case, we shall not consider the TX
timestamping POLLERR as a socket error condition, nor read the TX
timestamps.
When TX timestamps appear in errqueue, switch from fd poll wait to
polling the fd at regular intervals. This is because unread errqueue
causes poll() to wake up immediately, so the mainloop cannot block on
that, and we have to use a timer instead with some reasonable timeout
for the use case.
This rate limits wakeups on new TX timestamps we aren't going to read,
and also avoids the busy looping if timestamping was left on but
errqueue is not flushed.
Implement this only for io-glib; it is only needed for audio use cases
that anyway are using glib. Uses features from GLib 2.36 (from 2013) so
update configure.ac also.
---
Makefile.am | 1 +
acinclude.m4 | 3 +-
configure.ac | 2 +-
src/shared/io-glib.c | 157 ++++++++++++++++++++++++++++++++++++++++++-
src/shared/io-glib.h | 20 ++++++
5 files changed, 179 insertions(+), 4 deletions(-)
create mode 100644 src/shared/io-glib.h
diff --git a/Makefile.am b/Makefile.am
index 0821530e6..f958e2b99 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -253,6 +253,7 @@ shared_sources += src/shared/shell.c src/shared/shell.h
endif
src_libshared_glib_la_SOURCES = $(shared_sources) \
+ src/shared/io-glib.h \
src/shared/io-glib.c \
src/shared/timeout-glib.c \
src/shared/mainloop-glib.c \
diff --git a/acinclude.m4 b/acinclude.m4
index 168117840..598986d6e 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -63,8 +63,7 @@ AC_DEFUN([COMPILER_FLAGS], [
with_cflags="$with_cflags -Wformat -Wformat-security"
with_cflags="$with_cflags -Wstringop-overflow"
with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
- with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
- with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
+ with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_36"
fi
AC_SUBST([WARNING_CFLAGS], $with_cflags)
])
diff --git a/configure.ac b/configure.ac
index 75841e4c9..d2b0bab2f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
])
-PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
+PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.36)
if (test "${enable_threads}" = "yes"); then
AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
index 754043db1..bea9b2c32 100644
--- a/src/shared/io-glib.c
+++ b/src/shared/io-glib.c
@@ -13,10 +13,14 @@
#endif
#include <errno.h>
+#include <sys/socket.h>
#include <glib.h>
#include "src/shared/io.h"
+#include "src/shared/io-glib.h"
+
+#define IO_ERR_WATCH_RATELIMIT (500 * G_TIME_SPAN_MILLISECOND)
struct io_watch {
struct io *io;
@@ -29,11 +33,19 @@ struct io_watch {
struct io {
int ref_count;
GIOChannel *channel;
+ bool err_watch;
struct io_watch *read_watch;
struct io_watch *write_watch;
struct io_watch *disconnect_watch;
};
+struct io_err_watch {
+ GSource source;
+ GIOChannel *io;
+ GIOCondition events;
+ gpointer tag;
+};
+
static struct io *io_ref(struct io *io)
{
if (!io)
@@ -179,10 +191,17 @@ static struct io_watch *watch_new(struct io *io, GIOCondition cond,
prio = cond == G_IO_HUP ? G_PRIORITY_DEFAULT_IDLE : G_PRIORITY_DEFAULT;
- watch->id = g_io_add_watch_full(io->channel, prio,
+ if (!io->err_watch)
+ watch->id = g_io_add_watch_full(io->channel, prio,
+ cond | G_IO_ERR | G_IO_NVAL,
+ watch_callback, watch,
+ watch_destroy);
+ else
+ watch->id = io_glib_add_err_watch_full(io->channel, prio,
cond | G_IO_ERR | G_IO_NVAL,
watch_callback, watch,
watch_destroy);
+
if (watch->id == 0) {
watch_destroy(watch);
return NULL;
@@ -250,6 +269,15 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
return io_set_handler(io, G_IO_HUP, callback, user_data, destroy);
}
+bool io_set_use_err_watch(struct io *io, bool err_watch)
+{
+ if (!io)
+ return false;
+
+ io->err_watch = err_watch;
+ return true;
+}
+
ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
{
int fd;
@@ -278,3 +306,130 @@ bool io_shutdown(struct io *io)
return g_io_channel_shutdown(io->channel, TRUE, NULL)
== G_IO_STATUS_NORMAL;
}
+
+/*
+ * GSource implementation that tolerates non-empty MSG_ERRQUEUE, without
+ * attempting to flush it. This is intended for use with TX timestamping in
+ * cases where someone else is reading the timestamps and we are only interested
+ * in POLLHUP or socket errors.
+ */
+
+static gint64 io_err_watch_wakeup;
+
+static gboolean io_err_watch_dispatch(GSource *source,
+ GSourceFunc callback, gpointer user_data)
+{
+ struct io_err_watch *watch = (void *)source;
+ const GIOFunc func = (void *)callback;
+ const gint64 timeout = IO_ERR_WATCH_RATELIMIT;
+ GIOCondition cond;
+ int fd;
+
+ if (!func)
+ return FALSE;
+
+ fd = g_io_channel_unix_get_fd(watch->io);
+
+ /*
+ * If woken up by POLLERR only, and SO_ERROR is not set, ignore this
+ * event. Also disable polling for some time so that we don't consume
+ * too much CPU on events we are not interested in, or busy loop if
+ * nobody is flushing the errqueue.
+ */
+
+ if (watch->tag)
+ cond = g_source_query_unix_fd(&watch->source, watch->tag);
+ else
+ cond = 0;
+
+ if (cond == G_IO_ERR) {
+ int err, ret;
+ socklen_t len = sizeof(err);
+
+ ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
+ if (ret == 0 && err == 0) {
+ g_source_remove_unix_fd(&watch->source, watch->tag);
+ watch->tag = NULL;
+
+ /* io_err watches all wake up at the same time */
+ if (!io_err_watch_wakeup)
+ io_err_watch_wakeup = g_get_monotonic_time()
+ + timeout;
+
+ g_source_set_ready_time(&watch->source,
+ io_err_watch_wakeup);
+ return TRUE;
+ }
+ }
+
+ if (g_source_get_ready_time(&watch->source) != -1) {
+ g_assert(!watch->tag);
+ io_err_watch_wakeup = 0;
+ watch->tag = g_source_add_unix_fd(&watch->source, fd,
+ watch->events);
+ g_source_set_ready_time(&watch->source, -1);
+ }
+
+ cond &= watch->events;
+
+ if (cond)
+ return func(watch->io, cond, user_data);
+ else
+ return TRUE;
+}
+
+static void io_err_watch_finalize(GSource *source)
+{
+ struct io_err_watch *watch = (void *)source;
+
+ if (watch->tag)
+ g_source_remove_unix_fd(&watch->source, watch->tag);
+
+ g_io_channel_unref(watch->io);
+}
+
+guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
+ GIOCondition events,
+ GIOFunc func, gpointer user_data,
+ GDestroyNotify notify)
+{
+ static GSourceFuncs source_funcs = {
+ .dispatch = io_err_watch_dispatch,
+ .finalize = io_err_watch_finalize,
+ };
+ GSourceFunc callback = (void *)func;
+ struct io_err_watch *watch;
+ gint fd;
+ guint id;
+
+ g_return_val_if_fail(!(events & (G_IO_IN | G_IO_OUT)), 0);
+ g_return_val_if_fail(events, 0);
+ g_return_val_if_fail(func, 0);
+
+ fd = g_io_channel_unix_get_fd(io);
+
+ watch = (void *)g_source_new(&source_funcs,
+ sizeof(struct io_err_watch));
+
+ watch->io = g_io_channel_ref(io);
+ watch->events = events;
+ watch->tag = g_source_add_unix_fd(&watch->source, fd, events);
+
+ g_source_set_name((void *)watch, "io_glib_err_watch");
+ g_source_set_callback(&watch->source, callback, user_data, notify);
+
+ if (priority != G_PRIORITY_DEFAULT)
+ g_source_set_priority(&watch->source, priority);
+
+ id = g_source_attach(&watch->source, NULL);
+ g_source_unref(&watch->source);
+
+ return id;
+}
+
+guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events, GIOFunc func,
+ gpointer user_data)
+{
+ return io_glib_add_err_watch_full(io, G_PRIORITY_DEFAULT, events,
+ func, user_data, NULL);
+}
diff --git a/src/shared/io-glib.h b/src/shared/io-glib.h
new file mode 100644
index 000000000..1db6fd468
--- /dev/null
+++ b/src/shared/io-glib.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ *
+ * BlueZ - Bluetooth protocol stack for Linux
+ *
+ * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
+ *
+ *
+ */
+
+#include <glib.h>
+
+guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events,
+ GIOFunc func, gpointer user_data);
+guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
+ GIOCondition events, GIOFunc func,
+ gpointer user_data,
+ GDestroyNotify notify);
+
+bool io_set_use_err_watch(struct io *io, bool err_watch);
--
2.48.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH BlueZ 2/3] avdtp: don't consider TX timestamps as errors
2025-02-09 10:46 [PATCH BlueZ 0/3] Add io watcher to be used with TX timestamping Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
@ 2025-02-09 10:46 ` Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 3/3] bap: " Pauli Virtanen
2 siblings, 0 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-09 10:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Use io_add_err_watch to avoid considering TX timestamps as errors in the
transport io channel.
---
profiles/audio/avdtp.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 80fbe847e..94abe311b 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -31,6 +31,8 @@
#include "btio/btio.h"
#include "src/btd.h"
#include "src/log.h"
+#include "src/shared/io.h"
+#include "src/shared/io-glib.h"
#include "src/shared/timeout.h"
#include "src/shared/util.h"
#include "src/shared/queue.h"
@@ -866,7 +868,8 @@ proceed:
avdtp_sep_set_state(session, sep, AVDTP_STATE_OPEN);
- stream->io_id = g_io_add_watch(io, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+ stream->io_id = io_glib_add_err_watch(io,
+ G_IO_ERR | G_IO_HUP | G_IO_NVAL,
(GIOFunc) transport_cb, stream);
/* Release pending IO */
--
2.48.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH BlueZ 3/3] bap: don't consider TX timestamps as errors
2025-02-09 10:46 [PATCH BlueZ 0/3] Add io watcher to be used with TX timestamping Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 2/3] avdtp: don't consider TX timestamps as errors Pauli Virtanen
@ 2025-02-09 10:46 ` Pauli Virtanen
2 siblings, 0 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-09 10:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Pauli Virtanen
Use io_add_err_watch to avoid considering TX timestamps as errors in the
transport io channel.
---
profiles/audio/bap.c | 14 +++++++++-----
src/shared/bap.c | 3 +++
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index 77df9455a..a1947f9cd 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -39,6 +39,8 @@
#include "src/btd.h"
#include "src/dbus-common.h"
#include "src/shared/util.h"
+#include "src/shared/io.h"
+#include "src/shared/io-glib.h"
#include "src/shared/att.h"
#include "src/shared/queue.h"
#include "src/shared/gatt-db.h"
@@ -2059,8 +2061,9 @@ static void setup_connect_io(struct bap_data *data, struct bap_setup *setup,
return;
}
- setup->io_id = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
- setup_io_disconnected, setup);
+ setup->io_id = io_glib_add_err_watch(io,
+ G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ setup_io_disconnected, setup);
setup->io = io;
setup->cig_active = !defer;
@@ -2119,7 +2122,8 @@ static void setup_connect_io_broadcast(struct bap_data *data,
return;
}
- setup->io_id = g_io_add_watch(io, G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+ setup->io_id = io_glib_add_err_watch(io,
+ G_IO_HUP | G_IO_ERR | G_IO_NVAL,
setup_io_disconnected, setup);
setup->io = io;
@@ -2797,7 +2801,7 @@ static void bap_connecting(struct bt_bap_stream *stream, bool state, int fd,
if (!setup->io) {
io = g_io_channel_unix_new(fd);
- setup->io_id = g_io_add_watch(io,
+ setup->io_id = io_glib_add_err_watch(io,
G_IO_HUP | G_IO_ERR | G_IO_NVAL,
setup_io_disconnected, setup);
setup->io = io;
@@ -2842,7 +2846,7 @@ static void bap_connecting_bcast(struct bt_bap_stream *stream, bool state,
if (!setup->io) {
io = g_io_channel_unix_new(fd);
- setup->io_id = g_io_add_watch(io,
+ setup->io_id = io_glib_add_err_watch(io,
G_IO_HUP | G_IO_ERR | G_IO_NVAL,
setup_io_disconnected, setup);
setup->io = io;
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 6ffeefa41..b24566c88 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -20,6 +20,7 @@
#include "lib/uuid.h"
#include "src/shared/io.h"
+#include "src/shared/io-glib.h"
#include "src/shared/queue.h"
#include "src/shared/util.h"
#include "src/shared/timeout.h"
@@ -2652,6 +2653,8 @@ static struct bt_bap_stream_io *stream_io_new(struct bt_bap *bap, int fd)
DBG(bap, "fd %d", fd);
+ io_set_use_err_watch(io, true);
+
sio = new0(struct bt_bap_stream_io, 1);
sio->bap = bap;
sio->io = io;
--
2.48.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: Add io watcher to be used with TX timestamping
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
@ 2025-02-09 11:12 ` bluez.test.bot
2025-02-10 15:13 ` [PATCH BlueZ 1/3] shared/io-glib: add " Luiz Augusto von Dentz
2025-02-11 0:10 ` Add io " bluez.test.bot
2 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2025-02-09 11:12 UTC (permalink / raw)
To: linux-bluetooth, pav
[-- Attachment #1: Type: text/plain, Size: 525 bytes --]
This is an automated email and please do not reply to this email.
Dear Submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.
----- Output -----
error: patch failed: acinclude.m4:63
error: acinclude.m4: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ 1/3] shared/io-glib: add watcher to be used with TX timestamping
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
2025-02-09 11:12 ` Add io " bluez.test.bot
@ 2025-02-10 15:13 ` Luiz Augusto von Dentz
2025-02-10 18:15 ` Pauli Virtanen
2025-02-11 0:10 ` Add io " bluez.test.bot
2 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2025-02-10 15:13 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
Hi Pauli,
On Sun, Feb 9, 2025 at 5:46 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Add special implementation of fd watcher GSource for audio use.
>
> For audio use cases, sound server may turn on TX timestamping on a
> socket that we are watching. In this case, we shall not consider the TX
> timestamping POLLERR as a socket error condition, nor read the TX
> timestamps.
>
> When TX timestamps appear in errqueue, switch from fd poll wait to
> polling the fd at regular intervals. This is because unread errqueue
> causes poll() to wake up immediately, so the mainloop cannot block on
> that, and we have to use a timer instead with some reasonable timeout
> for the use case.
>
> This rate limits wakeups on new TX timestamps we aren't going to read,
> and also avoids the busy looping if timestamping was left on but
> errqueue is not flushed.
>
> Implement this only for io-glib; it is only needed for audio use cases
> that anyway are using glib. Uses features from GLib 2.36 (from 2013) so
> update configure.ac also.
> ---
> Makefile.am | 1 +
> acinclude.m4 | 3 +-
> configure.ac | 2 +-
> src/shared/io-glib.c | 157 ++++++++++++++++++++++++++++++++++++++++++-
> src/shared/io-glib.h | 20 ++++++
> 5 files changed, 179 insertions(+), 4 deletions(-)
> create mode 100644 src/shared/io-glib.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 0821530e6..f958e2b99 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -253,6 +253,7 @@ shared_sources += src/shared/shell.c src/shared/shell.h
> endif
>
> src_libshared_glib_la_SOURCES = $(shared_sources) \
> + src/shared/io-glib.h \
> src/shared/io-glib.c \
> src/shared/timeout-glib.c \
> src/shared/mainloop-glib.c \
> diff --git a/acinclude.m4 b/acinclude.m4
> index 168117840..598986d6e 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -63,8 +63,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> with_cflags="$with_cflags -Wformat -Wformat-security"
> with_cflags="$with_cflags -Wstringop-overflow"
> with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
> - with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> - with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> + with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_36"
> fi
> AC_SUBST([WARNING_CFLAGS], $with_cflags)
> ])
> diff --git a/configure.ac b/configure.ac
> index 75841e4c9..d2b0bab2f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
> ])
>
>
> -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
> +PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.36)
I hope this doesn't come with other surprises though, afaik glib had
the bad habit of adding its own set of dependencies which made us
stick to such old versions.
>
> if (test "${enable_threads}" = "yes"); then
> AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
> diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> index 754043db1..bea9b2c32 100644
> --- a/src/shared/io-glib.c
> +++ b/src/shared/io-glib.c
> @@ -13,10 +13,14 @@
> #endif
>
> #include <errno.h>
> +#include <sys/socket.h>
>
> #include <glib.h>
>
> #include "src/shared/io.h"
> +#include "src/shared/io-glib.h"
> +
> +#define IO_ERR_WATCH_RATELIMIT (500 * G_TIME_SPAN_MILLISECOND)
>
> struct io_watch {
> struct io *io;
> @@ -29,11 +33,19 @@ struct io_watch {
> struct io {
> int ref_count;
> GIOChannel *channel;
> + bool err_watch;
> struct io_watch *read_watch;
> struct io_watch *write_watch;
> struct io_watch *disconnect_watch;
> };
>
> +struct io_err_watch {
> + GSource source;
> + GIOChannel *io;
> + GIOCondition events;
> + gpointer tag;
> +};
> +
> static struct io *io_ref(struct io *io)
> {
> if (!io)
> @@ -179,10 +191,17 @@ static struct io_watch *watch_new(struct io *io, GIOCondition cond,
>
> prio = cond == G_IO_HUP ? G_PRIORITY_DEFAULT_IDLE : G_PRIORITY_DEFAULT;
>
> - watch->id = g_io_add_watch_full(io->channel, prio,
> + if (!io->err_watch)
> + watch->id = g_io_add_watch_full(io->channel, prio,
> + cond | G_IO_ERR | G_IO_NVAL,
> + watch_callback, watch,
> + watch_destroy);
> + else
> + watch->id = io_glib_add_err_watch_full(io->channel, prio,
> cond | G_IO_ERR | G_IO_NVAL,
> watch_callback, watch,
> watch_destroy);
> +
> if (watch->id == 0) {
> watch_destroy(watch);
> return NULL;
> @@ -250,6 +269,15 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> return io_set_handler(io, G_IO_HUP, callback, user_data, destroy);
> }
>
> +bool io_set_use_err_watch(struct io *io, bool err_watch)
> +{
> + if (!io)
> + return false;
> +
> + io->err_watch = err_watch;
> + return true;
> +}
> +
> ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> {
> int fd;
> @@ -278,3 +306,130 @@ bool io_shutdown(struct io *io)
> return g_io_channel_shutdown(io->channel, TRUE, NULL)
> == G_IO_STATUS_NORMAL;
> }
> +
> +/*
> + * GSource implementation that tolerates non-empty MSG_ERRQUEUE, without
> + * attempting to flush it. This is intended for use with TX timestamping in
> + * cases where someone else is reading the timestamps and we are only interested
> + * in POLLHUP or socket errors.
> + */
> +
> +static gint64 io_err_watch_wakeup;
> +
> +static gboolean io_err_watch_dispatch(GSource *source,
> + GSourceFunc callback, gpointer user_data)
> +{
> + struct io_err_watch *watch = (void *)source;
> + const GIOFunc func = (void *)callback;
> + const gint64 timeout = IO_ERR_WATCH_RATELIMIT;
> + GIOCondition cond;
> + int fd;
> +
> + if (!func)
> + return FALSE;
> +
> + fd = g_io_channel_unix_get_fd(watch->io);
> +
> + /*
> + * If woken up by POLLERR only, and SO_ERROR is not set, ignore this
> + * event. Also disable polling for some time so that we don't consume
> + * too much CPU on events we are not interested in, or busy loop if
> + * nobody is flushing the errqueue.
> + */
Not sure if I asked about this before, but have you consider disabling
POLLERR completely in case we detect the errqueue is in use? Because
even with rate limit I think we may still impact the system, the only
problem is then if by disabling POLLERR we would get in trouble
detecting disconnections? It shall result in POLLHUP though, so if we
only care about it for disconnection I assume it would be sufficient.
> + if (watch->tag)
> + cond = g_source_query_unix_fd(&watch->source, watch->tag);
> + else
> + cond = 0;
> +
> + if (cond == G_IO_ERR) {
> + int err, ret;
> + socklen_t len = sizeof(err);
> +
> + ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
> + if (ret == 0 && err == 0) {
> + g_source_remove_unix_fd(&watch->source, watch->tag);
> + watch->tag = NULL;
> +
> + /* io_err watches all wake up at the same time */
> + if (!io_err_watch_wakeup)
> + io_err_watch_wakeup = g_get_monotonic_time()
> + + timeout;
> +
> + g_source_set_ready_time(&watch->source,
> + io_err_watch_wakeup);
> + return TRUE;
> + }
> + }
> +
> + if (g_source_get_ready_time(&watch->source) != -1) {
> + g_assert(!watch->tag);
> + io_err_watch_wakeup = 0;
> + watch->tag = g_source_add_unix_fd(&watch->source, fd,
> + watch->events);
> + g_source_set_ready_time(&watch->source, -1);
> + }
> +
> + cond &= watch->events;
> +
> + if (cond)
> + return func(watch->io, cond, user_data);
> + else
> + return TRUE;
> +}
> +
> +static void io_err_watch_finalize(GSource *source)
> +{
> + struct io_err_watch *watch = (void *)source;
> +
> + if (watch->tag)
> + g_source_remove_unix_fd(&watch->source, watch->tag);
> +
> + g_io_channel_unref(watch->io);
> +}
> +
> +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> + GIOCondition events,
> + GIOFunc func, gpointer user_data,
> + GDestroyNotify notify)
> +{
> + static GSourceFuncs source_funcs = {
> + .dispatch = io_err_watch_dispatch,
> + .finalize = io_err_watch_finalize,
> + };
> + GSourceFunc callback = (void *)func;
> + struct io_err_watch *watch;
> + gint fd;
> + guint id;
> +
> + g_return_val_if_fail(!(events & (G_IO_IN | G_IO_OUT)), 0);
> + g_return_val_if_fail(events, 0);
> + g_return_val_if_fail(func, 0);
> +
> + fd = g_io_channel_unix_get_fd(io);
> +
> + watch = (void *)g_source_new(&source_funcs,
> + sizeof(struct io_err_watch));
> +
> + watch->io = g_io_channel_ref(io);
> + watch->events = events;
> + watch->tag = g_source_add_unix_fd(&watch->source, fd, events);
> +
> + g_source_set_name((void *)watch, "io_glib_err_watch");
> + g_source_set_callback(&watch->source, callback, user_data, notify);
> +
> + if (priority != G_PRIORITY_DEFAULT)
> + g_source_set_priority(&watch->source, priority);
> +
> + id = g_source_attach(&watch->source, NULL);
> + g_source_unref(&watch->source);
> +
> + return id;
> +}
> +
> +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events, GIOFunc func,
> + gpointer user_data)
> +{
> + return io_glib_add_err_watch_full(io, G_PRIORITY_DEFAULT, events,
> + func, user_data, NULL);
> +}
> diff --git a/src/shared/io-glib.h b/src/shared/io-glib.h
> new file mode 100644
> index 000000000..1db6fd468
> --- /dev/null
> +++ b/src/shared/io-glib.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + *
> + * BlueZ - Bluetooth protocol stack for Linux
> + *
> + * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
> + *
> + *
> + */
> +
> +#include <glib.h>
> +
> +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events,
> + GIOFunc func, gpointer user_data);
> +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> + GIOCondition events, GIOFunc func,
> + gpointer user_data,
> + GDestroyNotify notify);
> +
Hmm, I think it is probably not a good idea to start using headers
like this, I'd consider just making it return 0 for non-glib.
> +bool io_set_use_err_watch(struct io *io, bool err_watch);
> --
> 2.48.1
>
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ 1/3] shared/io-glib: add watcher to be used with TX timestamping
2025-02-10 15:13 ` [PATCH BlueZ 1/3] shared/io-glib: add " Luiz Augusto von Dentz
@ 2025-02-10 18:15 ` Pauli Virtanen
2025-02-10 18:33 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-10 18:15 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
ma, 2025-02-10 kello 10:13 -0500, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Sun, Feb 9, 2025 at 5:46 AM Pauli Virtanen <pav@iki.fi> wrote:
> >
> > Add special implementation of fd watcher GSource for audio use.
> >
> > For audio use cases, sound server may turn on TX timestamping on a
> > socket that we are watching. In this case, we shall not consider the TX
> > timestamping POLLERR as a socket error condition, nor read the TX
> > timestamps.
> >
> > When TX timestamps appear in errqueue, switch from fd poll wait to
> > polling the fd at regular intervals. This is because unread errqueue
> > causes poll() to wake up immediately, so the mainloop cannot block on
> > that, and we have to use a timer instead with some reasonable timeout
> > for the use case.
> >
> > This rate limits wakeups on new TX timestamps we aren't going to read,
> > and also avoids the busy looping if timestamping was left on but
> > errqueue is not flushed.
> >
> > Implement this only for io-glib; it is only needed for audio use cases
> > that anyway are using glib. Uses features from GLib 2.36 (from 2013) so
> > update configure.ac also.
> > ---
> > Makefile.am | 1 +
> > acinclude.m4 | 3 +-
> > configure.ac | 2 +-
> > src/shared/io-glib.c | 157 ++++++++++++++++++++++++++++++++++++++++++-
> > src/shared/io-glib.h | 20 ++++++
> > 5 files changed, 179 insertions(+), 4 deletions(-)
> > create mode 100644 src/shared/io-glib.h
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 0821530e6..f958e2b99 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -253,6 +253,7 @@ shared_sources += src/shared/shell.c src/shared/shell.h
> > endif
> >
> > src_libshared_glib_la_SOURCES = $(shared_sources) \
> > + src/shared/io-glib.h \
> > src/shared/io-glib.c \
> > src/shared/timeout-glib.c \
> > src/shared/mainloop-glib.c \
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index 168117840..598986d6e 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -63,8 +63,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> > with_cflags="$with_cflags -Wformat -Wformat-security"
> > with_cflags="$with_cflags -Wstringop-overflow"
> > with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
> > - with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> > - with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> > + with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_36"
> > fi
> > AC_SUBST([WARNING_CFLAGS], $with_cflags)
> > ])
> > diff --git a/configure.ac b/configure.ac
> > index 75841e4c9..d2b0bab2f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
> > ])
> >
> >
> > -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
> > +PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.36)
>
> I hope this doesn't come with other surprises though, afaik glib had
> the bad habit of adding its own set of dependencies which made us
> stick to such old versions.
Glib 2.36 was released in 2013, I hope all BlueZ builds use something
newer...
We might want to keep the max version limit in maintainer mode, though,
to get warnings if using too new API features.
>
> >
> > if (test "${enable_threads}" = "yes"); then
> > AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
> > diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> > index 754043db1..bea9b2c32 100644
> > --- a/src/shared/io-glib.c
> > +++ b/src/shared/io-glib.c
> > @@ -13,10 +13,14 @@
> > #endif
> >
> > #include <errno.h>
> > +#include <sys/socket.h>
> >
> > #include <glib.h>
> >
> > #include "src/shared/io.h"
> > +#include "src/shared/io-glib.h"
> > +
> > +#define IO_ERR_WATCH_RATELIMIT (500 * G_TIME_SPAN_MILLISECOND)
> >
> > struct io_watch {
> > struct io *io;
> > @@ -29,11 +33,19 @@ struct io_watch {
> > struct io {
> > int ref_count;
> > GIOChannel *channel;
> > + bool err_watch;
> > struct io_watch *read_watch;
> > struct io_watch *write_watch;
> > struct io_watch *disconnect_watch;
> > };
> >
> > +struct io_err_watch {
> > + GSource source;
> > + GIOChannel *io;
> > + GIOCondition events;
> > + gpointer tag;
> > +};
> > +
> > static struct io *io_ref(struct io *io)
> > {
> > if (!io)
> > @@ -179,10 +191,17 @@ static struct io_watch *watch_new(struct io *io, GIOCondition cond,
> >
> > prio = cond == G_IO_HUP ? G_PRIORITY_DEFAULT_IDLE : G_PRIORITY_DEFAULT;
> >
> > - watch->id = g_io_add_watch_full(io->channel, prio,
> > + if (!io->err_watch)
> > + watch->id = g_io_add_watch_full(io->channel, prio,
> > + cond | G_IO_ERR | G_IO_NVAL,
> > + watch_callback, watch,
> > + watch_destroy);
> > + else
> > + watch->id = io_glib_add_err_watch_full(io->channel, prio,
> > cond | G_IO_ERR | G_IO_NVAL,
> > watch_callback, watch,
> > watch_destroy);
> > +
> > if (watch->id == 0) {
> > watch_destroy(watch);
> > return NULL;
> > @@ -250,6 +269,15 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> > return io_set_handler(io, G_IO_HUP, callback, user_data, destroy);
> > }
> >
> > +bool io_set_use_err_watch(struct io *io, bool err_watch)
> > +{
> > + if (!io)
> > + return false;
> > +
> > + io->err_watch = err_watch;
> > + return true;
> > +}
> > +
> > ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> > {
> > int fd;
> > @@ -278,3 +306,130 @@ bool io_shutdown(struct io *io)
> > return g_io_channel_shutdown(io->channel, TRUE, NULL)
> > == G_IO_STATUS_NORMAL;
> > }
> > +
> > +/*
> > + * GSource implementation that tolerates non-empty MSG_ERRQUEUE, without
> > + * attempting to flush it. This is intended for use with TX timestamping in
> > + * cases where someone else is reading the timestamps and we are only interested
> > + * in POLLHUP or socket errors.
> > + */
> > +
> > +static gint64 io_err_watch_wakeup;
> > +
> > +static gboolean io_err_watch_dispatch(GSource *source,
> > + GSourceFunc callback, gpointer user_data)
> > +{
> > + struct io_err_watch *watch = (void *)source;
> > + const GIOFunc func = (void *)callback;
> > + const gint64 timeout = IO_ERR_WATCH_RATELIMIT;
> > + GIOCondition cond;
> > + int fd;
> > +
> > + if (!func)
> > + return FALSE;
> > +
> > + fd = g_io_channel_unix_get_fd(watch->io);
> > +
> > + /*
> > + * If woken up by POLLERR only, and SO_ERROR is not set, ignore this
> > + * event. Also disable polling for some time so that we don't consume
> > + * too much CPU on events we are not interested in, or busy loop if
> > + * nobody is flushing the errqueue.
> > + */
>
> Not sure if I asked about this before, but have you consider disabling
> POLLERR completely in case we detect the errqueue is in use? Because
> even with rate limit I think we may still impact the system, the only
> problem is then if by disabling POLLERR we would get in trouble
> detecting disconnections? It shall result in POLLHUP though, so if we
> only care about it for disconnection I assume it would be sufficient.
It's not possible to disable wakeups on POLLERR & POLLHUP:
https://man.archlinux.org/man/poll.2.en
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/eventpoll.c?h=v6.13&id=ffd294d346d185b70e28b1a28abe367bbfe53c04#n2391
You could use EPOLLET to only wake up once per event -- but bluez would
then still wakeup on the 7.5ms interval. Glib also doesn't use epoll(),
so there's more parts of GSource to reimplement, need to poll from
separate thread etc., so the implementation gets more complex than
here.
To get something better than the timer-polled version, I think you'd
need to add some new flag to kernel for this.
> > + if (watch->tag)
> > + cond = g_source_query_unix_fd(&watch->source, watch->tag);
> > + else
> > + cond = 0;
> > +
> > + if (cond == G_IO_ERR) {
> > + int err, ret;
> > + socklen_t len = sizeof(err);
> > +
> > + ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
> > + if (ret == 0 && err == 0) {
> > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > + watch->tag = NULL;
> > +
> > + /* io_err watches all wake up at the same time */
> > + if (!io_err_watch_wakeup)
> > + io_err_watch_wakeup = g_get_monotonic_time()
> > + + timeout;
> > +
> > + g_source_set_ready_time(&watch->source,
> > + io_err_watch_wakeup);
> > + return TRUE;
> > + }
> > + }
> > +
> > + if (g_source_get_ready_time(&watch->source) != -1) {
> > + g_assert(!watch->tag);
> > + io_err_watch_wakeup = 0;
> > + watch->tag = g_source_add_unix_fd(&watch->source, fd,
> > + watch->events);
> > + g_source_set_ready_time(&watch->source, -1);
> > + }
> > +
> > + cond &= watch->events;
> > +
> > + if (cond)
> > + return func(watch->io, cond, user_data);
> > + else
> > + return TRUE;
> > +}
> > +
> > +static void io_err_watch_finalize(GSource *source)
> > +{
> > + struct io_err_watch *watch = (void *)source;
> > +
> > + if (watch->tag)
> > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > +
> > + g_io_channel_unref(watch->io);
> > +}
> > +
> > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > + GIOCondition events,
> > + GIOFunc func, gpointer user_data,
> > + GDestroyNotify notify)
> > +{
> > + static GSourceFuncs source_funcs = {
> > + .dispatch = io_err_watch_dispatch,
> > + .finalize = io_err_watch_finalize,
> > + };
> > + GSourceFunc callback = (void *)func;
> > + struct io_err_watch *watch;
> > + gint fd;
> > + guint id;
> > +
> > + g_return_val_if_fail(!(events & (G_IO_IN | G_IO_OUT)), 0);
> > + g_return_val_if_fail(events, 0);
> > + g_return_val_if_fail(func, 0);
> > +
> > + fd = g_io_channel_unix_get_fd(io);
> > +
> > + watch = (void *)g_source_new(&source_funcs,
> > + sizeof(struct io_err_watch));
> > +
> > + watch->io = g_io_channel_ref(io);
> > + watch->events = events;
> > + watch->tag = g_source_add_unix_fd(&watch->source, fd, events);
> > +
> > + g_source_set_name((void *)watch, "io_glib_err_watch");
> > + g_source_set_callback(&watch->source, callback, user_data, notify);
> > +
> > + if (priority != G_PRIORITY_DEFAULT)
> > + g_source_set_priority(&watch->source, priority);
> > +
> > + id = g_source_attach(&watch->source, NULL);
> > + g_source_unref(&watch->source);
> > +
> > + return id;
> > +}
> > +
> > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events, GIOFunc func,
> > + gpointer user_data)
> > +{
> > + return io_glib_add_err_watch_full(io, G_PRIORITY_DEFAULT, events,
> > + func, user_data, NULL);
> > +}
> > diff --git a/src/shared/io-glib.h b/src/shared/io-glib.h
> > new file mode 100644
> > index 000000000..1db6fd468
> > --- /dev/null
> > +++ b/src/shared/io-glib.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + *
> > + * BlueZ - Bluetooth protocol stack for Linux
> > + *
> > + * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
> > + *
> > + *
> > + */
> > +
> > +#include <glib.h>
> > +
> > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events,
> > + GIOFunc func, gpointer user_data);
> > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > + GIOCondition events, GIOFunc func,
> > + gpointer user_data,
> > + GDestroyNotify notify);
> > +
>
> Hmm, I think it is probably not a good idea to start using headers
> like this, I'd consider just making it return 0 for non-glib.
Ok.
> > +bool io_set_use_err_watch(struct io *io, bool err_watch);
> > --
> > 2.48.1
--
Pauli Virtanen
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ 1/3] shared/io-glib: add watcher to be used with TX timestamping
2025-02-10 18:15 ` Pauli Virtanen
@ 2025-02-10 18:33 ` Luiz Augusto von Dentz
2025-02-12 17:58 ` Pauli Virtanen
0 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2025-02-10 18:33 UTC (permalink / raw)
To: Pauli Virtanen; +Cc: linux-bluetooth
Hi Pauli,
On Mon, Feb 10, 2025 at 1:15 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ma, 2025-02-10 kello 10:13 -0500, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Sun, Feb 9, 2025 at 5:46 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > Add special implementation of fd watcher GSource for audio use.
> > >
> > > For audio use cases, sound server may turn on TX timestamping on a
> > > socket that we are watching. In this case, we shall not consider the TX
> > > timestamping POLLERR as a socket error condition, nor read the TX
> > > timestamps.
> > >
> > > When TX timestamps appear in errqueue, switch from fd poll wait to
> > > polling the fd at regular intervals. This is because unread errqueue
> > > causes poll() to wake up immediately, so the mainloop cannot block on
> > > that, and we have to use a timer instead with some reasonable timeout
> > > for the use case.
> > >
> > > This rate limits wakeups on new TX timestamps we aren't going to read,
> > > and also avoids the busy looping if timestamping was left on but
> > > errqueue is not flushed.
> > >
> > > Implement this only for io-glib; it is only needed for audio use cases
> > > that anyway are using glib. Uses features from GLib 2.36 (from 2013) so
> > > update configure.ac also.
> > > ---
> > > Makefile.am | 1 +
> > > acinclude.m4 | 3 +-
> > > configure.ac | 2 +-
> > > src/shared/io-glib.c | 157 ++++++++++++++++++++++++++++++++++++++++++-
> > > src/shared/io-glib.h | 20 ++++++
> > > 5 files changed, 179 insertions(+), 4 deletions(-)
> > > create mode 100644 src/shared/io-glib.h
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 0821530e6..f958e2b99 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -253,6 +253,7 @@ shared_sources += src/shared/shell.c src/shared/shell.h
> > > endif
> > >
> > > src_libshared_glib_la_SOURCES = $(shared_sources) \
> > > + src/shared/io-glib.h \
> > > src/shared/io-glib.c \
> > > src/shared/timeout-glib.c \
> > > src/shared/mainloop-glib.c \
> > > diff --git a/acinclude.m4 b/acinclude.m4
> > > index 168117840..598986d6e 100644
> > > --- a/acinclude.m4
> > > +++ b/acinclude.m4
> > > @@ -63,8 +63,7 @@ AC_DEFUN([COMPILER_FLAGS], [
> > > with_cflags="$with_cflags -Wformat -Wformat-security"
> > > with_cflags="$with_cflags -Wstringop-overflow"
> > > with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
> > > - with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
> > > - with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
> > > + with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_36"
> > > fi
> > > AC_SUBST([WARNING_CFLAGS], $with_cflags)
> > > ])
> > > diff --git a/configure.ac b/configure.ac
> > > index 75841e4c9..d2b0bab2f 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
> > > ])
> > >
> > >
> > > -PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
> > > +PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.36)
> >
> > I hope this doesn't come with other surprises though, afaik glib had
> > the bad habit of adding its own set of dependencies which made us
> > stick to such old versions.
>
> Glib 2.36 was released in 2013, I hope all BlueZ builds use something
> newer...
>
> We might want to keep the max version limit in maintainer mode, though,
> to get warnings if using too new API features.
>
> >
> > >
> > > if (test "${enable_threads}" = "yes"); then
> > > AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
> > > diff --git a/src/shared/io-glib.c b/src/shared/io-glib.c
> > > index 754043db1..bea9b2c32 100644
> > > --- a/src/shared/io-glib.c
> > > +++ b/src/shared/io-glib.c
> > > @@ -13,10 +13,14 @@
> > > #endif
> > >
> > > #include <errno.h>
> > > +#include <sys/socket.h>
> > >
> > > #include <glib.h>
> > >
> > > #include "src/shared/io.h"
> > > +#include "src/shared/io-glib.h"
> > > +
> > > +#define IO_ERR_WATCH_RATELIMIT (500 * G_TIME_SPAN_MILLISECOND)
> > >
> > > struct io_watch {
> > > struct io *io;
> > > @@ -29,11 +33,19 @@ struct io_watch {
> > > struct io {
> > > int ref_count;
> > > GIOChannel *channel;
> > > + bool err_watch;
> > > struct io_watch *read_watch;
> > > struct io_watch *write_watch;
> > > struct io_watch *disconnect_watch;
> > > };
> > >
> > > +struct io_err_watch {
> > > + GSource source;
> > > + GIOChannel *io;
> > > + GIOCondition events;
> > > + gpointer tag;
> > > +};
> > > +
> > > static struct io *io_ref(struct io *io)
> > > {
> > > if (!io)
> > > @@ -179,10 +191,17 @@ static struct io_watch *watch_new(struct io *io, GIOCondition cond,
> > >
> > > prio = cond == G_IO_HUP ? G_PRIORITY_DEFAULT_IDLE : G_PRIORITY_DEFAULT;
> > >
> > > - watch->id = g_io_add_watch_full(io->channel, prio,
> > > + if (!io->err_watch)
> > > + watch->id = g_io_add_watch_full(io->channel, prio,
> > > + cond | G_IO_ERR | G_IO_NVAL,
> > > + watch_callback, watch,
> > > + watch_destroy);
> > > + else
> > > + watch->id = io_glib_add_err_watch_full(io->channel, prio,
> > > cond | G_IO_ERR | G_IO_NVAL,
> > > watch_callback, watch,
> > > watch_destroy);
> > > +
> > > if (watch->id == 0) {
> > > watch_destroy(watch);
> > > return NULL;
> > > @@ -250,6 +269,15 @@ bool io_set_disconnect_handler(struct io *io, io_callback_func_t callback,
> > > return io_set_handler(io, G_IO_HUP, callback, user_data, destroy);
> > > }
> > >
> > > +bool io_set_use_err_watch(struct io *io, bool err_watch)
> > > +{
> > > + if (!io)
> > > + return false;
> > > +
> > > + io->err_watch = err_watch;
> > > + return true;
> > > +}
> > > +
> > > ssize_t io_send(struct io *io, const struct iovec *iov, int iovcnt)
> > > {
> > > int fd;
> > > @@ -278,3 +306,130 @@ bool io_shutdown(struct io *io)
> > > return g_io_channel_shutdown(io->channel, TRUE, NULL)
> > > == G_IO_STATUS_NORMAL;
> > > }
> > > +
> > > +/*
> > > + * GSource implementation that tolerates non-empty MSG_ERRQUEUE, without
> > > + * attempting to flush it. This is intended for use with TX timestamping in
> > > + * cases where someone else is reading the timestamps and we are only interested
> > > + * in POLLHUP or socket errors.
> > > + */
> > > +
> > > +static gint64 io_err_watch_wakeup;
> > > +
> > > +static gboolean io_err_watch_dispatch(GSource *source,
> > > + GSourceFunc callback, gpointer user_data)
> > > +{
> > > + struct io_err_watch *watch = (void *)source;
> > > + const GIOFunc func = (void *)callback;
> > > + const gint64 timeout = IO_ERR_WATCH_RATELIMIT;
> > > + GIOCondition cond;
> > > + int fd;
> > > +
> > > + if (!func)
> > > + return FALSE;
> > > +
> > > + fd = g_io_channel_unix_get_fd(watch->io);
> > > +
> > > + /*
> > > + * If woken up by POLLERR only, and SO_ERROR is not set, ignore this
> > > + * event. Also disable polling for some time so that we don't consume
> > > + * too much CPU on events we are not interested in, or busy loop if
> > > + * nobody is flushing the errqueue.
> > > + */
> >
> > Not sure if I asked about this before, but have you consider disabling
> > POLLERR completely in case we detect the errqueue is in use? Because
> > even with rate limit I think we may still impact the system, the only
> > problem is then if by disabling POLLERR we would get in trouble
> > detecting disconnections? It shall result in POLLHUP though, so if we
> > only care about it for disconnection I assume it would be sufficient.
>
> It's not possible to disable wakeups on POLLERR & POLLHUP:
>
> https://man.archlinux.org/man/poll.2.en
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/eventpoll.c?h=v6.13&id=ffd294d346d185b70e28b1a28abe367bbfe53c04#n2391
Ah, so that means POLLERR and POLLHUP are always enabled together?
> You could use EPOLLET to only wake up once per event -- but bluez would
> then still wakeup on the 7.5ms interval. Glib also doesn't use epoll(),
> so there's more parts of GSource to reimplement, need to poll from
> separate thread etc., so the implementation gets more complex than
> here.
>
> To get something better than the timer-polled version, I think you'd
> need to add some new flag to kernel for this.
Yeah, well I guess that would be even harder if we need to a new flag
since that would mean we would need to update glib to understand it
and then bump the dependency of it, or we would have to reimplement
the whole IO handling to use the non-glib version, the glib way will
most likely make releases adding a hard dependency for distros and
reimplementing the whole IO will probably require a lot of work, that
said Id welcome if we could work in that direction otherwise we need
another way to track the disconnection of ISO and A2DP streams.
> > > + if (watch->tag)
> > > + cond = g_source_query_unix_fd(&watch->source, watch->tag);
> > > + else
> > > + cond = 0;
> > > +
> > > + if (cond == G_IO_ERR) {
> > > + int err, ret;
> > > + socklen_t len = sizeof(err);
> > > +
> > > + ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
> > > + if (ret == 0 && err == 0) {
> > > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > > + watch->tag = NULL;
> > > +
> > > + /* io_err watches all wake up at the same time */
> > > + if (!io_err_watch_wakeup)
> > > + io_err_watch_wakeup = g_get_monotonic_time()
> > > + + timeout;
> > > +
> > > + g_source_set_ready_time(&watch->source,
> > > + io_err_watch_wakeup);
> > > + return TRUE;
> > > + }
> > > + }
> > > +
> > > + if (g_source_get_ready_time(&watch->source) != -1) {
> > > + g_assert(!watch->tag);
> > > + io_err_watch_wakeup = 0;
> > > + watch->tag = g_source_add_unix_fd(&watch->source, fd,
> > > + watch->events);
> > > + g_source_set_ready_time(&watch->source, -1);
> > > + }
> > > +
> > > + cond &= watch->events;
> > > +
> > > + if (cond)
> > > + return func(watch->io, cond, user_data);
> > > + else
> > > + return TRUE;
> > > +}
> > > +
> > > +static void io_err_watch_finalize(GSource *source)
> > > +{
> > > + struct io_err_watch *watch = (void *)source;
> > > +
> > > + if (watch->tag)
> > > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > > +
> > > + g_io_channel_unref(watch->io);
> > > +}
> > > +
> > > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > > + GIOCondition events,
> > > + GIOFunc func, gpointer user_data,
> > > + GDestroyNotify notify)
> > > +{
> > > + static GSourceFuncs source_funcs = {
> > > + .dispatch = io_err_watch_dispatch,
> > > + .finalize = io_err_watch_finalize,
> > > + };
> > > + GSourceFunc callback = (void *)func;
> > > + struct io_err_watch *watch;
> > > + gint fd;
> > > + guint id;
> > > +
> > > + g_return_val_if_fail(!(events & (G_IO_IN | G_IO_OUT)), 0);
> > > + g_return_val_if_fail(events, 0);
> > > + g_return_val_if_fail(func, 0);
> > > +
> > > + fd = g_io_channel_unix_get_fd(io);
> > > +
> > > + watch = (void *)g_source_new(&source_funcs,
> > > + sizeof(struct io_err_watch));
> > > +
> > > + watch->io = g_io_channel_ref(io);
> > > + watch->events = events;
> > > + watch->tag = g_source_add_unix_fd(&watch->source, fd, events);
> > > +
> > > + g_source_set_name((void *)watch, "io_glib_err_watch");
> > > + g_source_set_callback(&watch->source, callback, user_data, notify);
> > > +
> > > + if (priority != G_PRIORITY_DEFAULT)
> > > + g_source_set_priority(&watch->source, priority);
> > > +
> > > + id = g_source_attach(&watch->source, NULL);
> > > + g_source_unref(&watch->source);
> > > +
> > > + return id;
> > > +}
> > > +
> > > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events, GIOFunc func,
> > > + gpointer user_data)
> > > +{
> > > + return io_glib_add_err_watch_full(io, G_PRIORITY_DEFAULT, events,
> > > + func, user_data, NULL);
> > > +}
> > > diff --git a/src/shared/io-glib.h b/src/shared/io-glib.h
> > > new file mode 100644
> > > index 000000000..1db6fd468
> > > --- /dev/null
> > > +++ b/src/shared/io-glib.h
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + *
> > > + * BlueZ - Bluetooth protocol stack for Linux
> > > + *
> > > + * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
> > > + *
> > > + *
> > > + */
> > > +
> > > +#include <glib.h>
> > > +
> > > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events,
> > > + GIOFunc func, gpointer user_data);
> > > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > > + GIOCondition events, GIOFunc func,
> > > + gpointer user_data,
> > > + GDestroyNotify notify);
> > > +
> >
> > Hmm, I think it is probably not a good idea to start using headers
> > like this, I'd consider just making it return 0 for non-glib.
>
> Ok.
>
> > > +bool io_set_use_err_watch(struct io *io, bool err_watch);
> > > --
> > > 2.48.1
>
> --
> Pauli Virtanen
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: Add io watcher to be used with TX timestamping
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
2025-02-09 11:12 ` Add io " bluez.test.bot
2025-02-10 15:13 ` [PATCH BlueZ 1/3] shared/io-glib: add " Luiz Augusto von Dentz
@ 2025-02-11 0:10 ` bluez.test.bot
2 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2025-02-11 0:10 UTC (permalink / raw)
To: linux-bluetooth, pav
[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=931991
---Test result---
Test Summary:
CheckPatch PENDING 0.22 seconds
GitLint PENDING 0.22 seconds
BuildEll PASS 20.85 seconds
BluezMake PASS 1523.57 seconds
MakeCheck PASS 12.85 seconds
MakeDistcheck PASS 161.72 seconds
CheckValgrind PASS 216.97 seconds
CheckSmatch WARNING 286.41 seconds
bluezmakeextell PASS 100.80 seconds
IncrementalBuild PENDING 0.27 seconds
ScanBuild PASS 876.20 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:297:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:297:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:297:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH BlueZ 1/3] shared/io-glib: add watcher to be used with TX timestamping
2025-02-10 18:33 ` Luiz Augusto von Dentz
@ 2025-02-12 17:58 ` Pauli Virtanen
0 siblings, 0 replies; 10+ messages in thread
From: Pauli Virtanen @ 2025-02-12 17:58 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi,
ma, 2025-02-10 kello 13:33 -0500, Luiz Augusto von Dentz kirjoitti:
[clip]
> > It's not possible to disable wakeups on POLLERR & POLLHUP:
> >
> > https://man.archlinux.org/man/poll.2.en
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/eventpoll.c?h=v6.13&id=ffd294d346d185b70e28b1a28abe367bbfe53c04#n2391
>
> Ah, so that means POLLERR and POLLHUP are always enabled together?
I think the exact statement is that both POLLERR and POLLHUP are always
enabled, no way to disable kernel issuing wakeup on either condition.
> > You could use EPOLLET to only wake up once per event -- but bluez would
> > then still wakeup on the 7.5ms interval. Glib also doesn't use epoll(),
> > so there's more parts of GSource to reimplement, need to poll from
> > separate thread etc., so the implementation gets more complex than
> > here.
> >
> > To get something better than the timer-polled version, I think you'd
> > need to add some new flag to kernel for this.
>
> Yeah, well I guess that would be even harder if we need to a new flag
> since that would mean we would need to update glib to understand it
> and then bump the dependency of it, or we would have to reimplement
> the whole IO handling to use the non-glib version, the glib way will
> most likely make releases adding a hard dependency for distros and
> reimplementing the whole IO will probably require a lot of work, that
> said Id welcome if we could work in that direction otherwise we need
> another way to track the disconnection of ISO and A2DP streams.
Another alternative here is to go with socket option etc., which we
tried so that works, but maybe needs buy-in from netdev people.
> > > > + if (watch->tag)
> > > > + cond = g_source_query_unix_fd(&watch->source, watch->tag);
> > > > + else
> > > > + cond = 0;
> > > > +
> > > > + if (cond == G_IO_ERR) {
> > > > + int err, ret;
> > > > + socklen_t len = sizeof(err);
> > > > +
> > > > + ret = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &len);
> > > > + if (ret == 0 && err == 0) {
> > > > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > > > + watch->tag = NULL;
> > > > +
> > > > + /* io_err watches all wake up at the same time */
> > > > + if (!io_err_watch_wakeup)
> > > > + io_err_watch_wakeup = g_get_monotonic_time()
> > > > + + timeout;
> > > > +
> > > > + g_source_set_ready_time(&watch->source,
> > > > + io_err_watch_wakeup);
> > > > + return TRUE;
> > > > + }
> > > > + }
> > > > +
> > > > + if (g_source_get_ready_time(&watch->source) != -1) {
> > > > + g_assert(!watch->tag);
> > > > + io_err_watch_wakeup = 0;
> > > > + watch->tag = g_source_add_unix_fd(&watch->source, fd,
> > > > + watch->events);
> > > > + g_source_set_ready_time(&watch->source, -1);
> > > > + }
> > > > +
> > > > + cond &= watch->events;
> > > > +
> > > > + if (cond)
> > > > + return func(watch->io, cond, user_data);
> > > > + else
> > > > + return TRUE;
> > > > +}
> > > > +
> > > > +static void io_err_watch_finalize(GSource *source)
> > > > +{
> > > > + struct io_err_watch *watch = (void *)source;
> > > > +
> > > > + if (watch->tag)
> > > > + g_source_remove_unix_fd(&watch->source, watch->tag);
> > > > +
> > > > + g_io_channel_unref(watch->io);
> > > > +}
> > > > +
> > > > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > > > + GIOCondition events,
> > > > + GIOFunc func, gpointer user_data,
> > > > + GDestroyNotify notify)
> > > > +{
> > > > + static GSourceFuncs source_funcs = {
> > > > + .dispatch = io_err_watch_dispatch,
> > > > + .finalize = io_err_watch_finalize,
> > > > + };
> > > > + GSourceFunc callback = (void *)func;
> > > > + struct io_err_watch *watch;
> > > > + gint fd;
> > > > + guint id;
> > > > +
> > > > + g_return_val_if_fail(!(events & (G_IO_IN | G_IO_OUT)), 0);
> > > > + g_return_val_if_fail(events, 0);
> > > > + g_return_val_if_fail(func, 0);
> > > > +
> > > > + fd = g_io_channel_unix_get_fd(io);
> > > > +
> > > > + watch = (void *)g_source_new(&source_funcs,
> > > > + sizeof(struct io_err_watch));
> > > > +
> > > > + watch->io = g_io_channel_ref(io);
> > > > + watch->events = events;
> > > > + watch->tag = g_source_add_unix_fd(&watch->source, fd, events);
> > > > +
> > > > + g_source_set_name((void *)watch, "io_glib_err_watch");
> > > > + g_source_set_callback(&watch->source, callback, user_data, notify);
> > > > +
> > > > + if (priority != G_PRIORITY_DEFAULT)
> > > > + g_source_set_priority(&watch->source, priority);
> > > > +
> > > > + id = g_source_attach(&watch->source, NULL);
> > > > + g_source_unref(&watch->source);
> > > > +
> > > > + return id;
> > > > +}
> > > > +
> > > > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events, GIOFunc func,
> > > > + gpointer user_data)
> > > > +{
> > > > + return io_glib_add_err_watch_full(io, G_PRIORITY_DEFAULT, events,
> > > > + func, user_data, NULL);
> > > > +}
> > > > diff --git a/src/shared/io-glib.h b/src/shared/io-glib.h
> > > > new file mode 100644
> > > > index 000000000..1db6fd468
> > > > --- /dev/null
> > > > +++ b/src/shared/io-glib.h
> > > > @@ -0,0 +1,20 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + *
> > > > + * BlueZ - Bluetooth protocol stack for Linux
> > > > + *
> > > > + * Copyright (C) 2012-2014 Intel Corporation. All rights reserved.
> > > > + *
> > > > + *
> > > > + */
> > > > +
> > > > +#include <glib.h>
> > > > +
> > > > +guint io_glib_add_err_watch(GIOChannel *io, GIOCondition events,
> > > > + GIOFunc func, gpointer user_data);
> > > > +guint io_glib_add_err_watch_full(GIOChannel *io, gint priority,
> > > > + GIOCondition events, GIOFunc func,
> > > > + gpointer user_data,
> > > > + GDestroyNotify notify);
> > > > +
> > >
> > > Hmm, I think it is probably not a good idea to start using headers
> > > like this, I'd consider just making it return 0 for non-glib.
> >
> > Ok.
> >
> > > > +bool io_set_use_err_watch(struct io *io, bool err_watch);
> > > > --
> > > > 2.48.1
> >
> > --
> > Pauli Virtanen
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-12 17:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-09 10:46 [PATCH BlueZ 0/3] Add io watcher to be used with TX timestamping Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 1/3] shared/io-glib: add " Pauli Virtanen
2025-02-09 11:12 ` Add io " bluez.test.bot
2025-02-10 15:13 ` [PATCH BlueZ 1/3] shared/io-glib: add " Luiz Augusto von Dentz
2025-02-10 18:15 ` Pauli Virtanen
2025-02-10 18:33 ` Luiz Augusto von Dentz
2025-02-12 17:58 ` Pauli Virtanen
2025-02-11 0:10 ` Add io " bluez.test.bot
2025-02-09 10:46 ` [PATCH BlueZ 2/3] avdtp: don't consider TX timestamps as errors Pauli Virtanen
2025-02-09 10:46 ` [PATCH BlueZ 3/3] bap: " Pauli Virtanen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).