| Message ID | 3938636ceaafc589f6d12fa6fe50e1c6a1c76bef.1739097949.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add io watcher to be used with TX timestamping | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | fail | 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 |
| tedd_an/BuildEll | success | Build ELL PASS |
| tedd_an/BluezMake | success | Bluez Make PASS |
| tedd_an/MakeCheck | success | Bluez Make Check PASS |
| tedd_an/MakeDistcheck | success | Make Distcheck PASS |
| tedd_an/CheckValgrind | success | Check Valgrind PASS |
| tedd_an/CheckSmatch | warning | CheckSparse WARNING 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 |
| tedd_an/bluezmakeextell | success | Make External ELL PASS |
| tedd_an/ScanBuild | success | Scan Build PASS |
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
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 > >
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
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
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
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 > > >
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);