[PATCH 0/2] meson: Fix/improve detection of scheduler-related functionality

This applies on top of [1]. Test pipeline: [2] Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux. [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N... [2] https://gitlab.com/abologna/libvirt/-/pipelines/1192942974 Andrea Bolognani (2): meson: Restore check for sched_getaffinity() meson: Check for sched_get_priority_min() meson.build | 4 ++-- src/ch/ch_process.c | 1 + src/util/virprocess.c | 12 ++++++------ 3 files changed, 9 insertions(+), 8 deletions(-) -- 2.43.2

Commit c07cf0a68693 replaced this check with one for the presence of cpu_set_t. The idea at the time was that only sched_{get,set}affinity() were visible by default, while making cpu_set_t visible required defining _WITH_CPU_SET_T. So libvirt would detect the function and attempt to use it, but the code would not compile because the necessary data type had not been made accessible. The commit in question brought three FreeBSD commits as evidence of this. While [1] and [2] do indeed seem to support this explanation, [3] from just a few days later made it so that not just cpu_set_t, but also the functions, required user action to be visible. This arguably would have made the change unnecessary. However, [4] from roughly a month later changed things once again: it completely removed _WITH_CPU_SET_T, making both the functions and the data type visible by default. This is the status quo that seems to have persisted until today. If one were to check any recent FreeBSD build job performed as part of our CI pipeline, for example [5] and [6] for FreeBSD 13 and 14 respectively, they would be able to confirm that in both cases cpu_set_t is detected as available. Since there is no longer a difference between the availability of the functions and that of the data type, go back to what we had before. This has the interesting side-effect of fixing a bug introduced by the commit in question. When detection was changed from the function to the data type, most uses of WITH_SCHED_GETAFFINITY were replaced with uses of WITH_DECL_CPU_SET_T, but not all of them: specifically, those that decided whether qemuProcessInitCpuAffinity() would be actually implemented or replaced with a no-op stub were not updated, which means that we've been running the stub version everywhere except on FreeBSD ever since. The code has been copied to the Cloud Hypervisor driver in the meantime, which is similarly affected. Now that we're building the actual implementation, we need to add virnuma.h to the includes. As a nice bonus this also makes things work correctly on GNU/Hurd, where cpu_set_t is available but sched_{get,set}affinity() are non-working stubs. [1] https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b... [2] https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c4599301... [3] https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2... [4] https://cgit.freebsd.org/src/commit/?id=5e04571cf3cf4024be926976a6abf19626df... [5] https://gitlab.com/libvirt/libvirt/-/jobs/6266401204 [6] https://gitlab.com/libvirt/libvirt/-/jobs/6266401205 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 3 +-- src/ch/ch_process.c | 1 + src/util/virprocess.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/meson.build b/meson.build index 7845f60ff7..a80a65e447 100644 --- a/meson.build +++ b/meson.build @@ -584,6 +584,7 @@ functions = [ 'posix_fallocate', 'posix_memalign', 'prlimit', + 'sched_getaffinity', 'sched_setscheduler', 'setgroups', 'setrlimit', @@ -669,8 +670,6 @@ symbols = [ # Check for BSD approach for setting MAC addr [ 'net/if_dl.h', 'link_addr', '#include <sys/types.h>\n#include <sys/socket.h>' ], - - [ 'sched.h', 'cpu_set_t' ], ] if host_machine.system() == 'linux' diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 3bde9d9dcf..a0aca88928 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -33,6 +33,7 @@ #include "virfile.h" #include "virjson.h" #include "virlog.h" +#include "virnuma.h" #include "virstring.h" #include "ch_interface.h" diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 647f687cb8..8e4440b45f 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -500,7 +500,7 @@ virProcessGetAffinity(pid_t pid) return ret; } -#elif WITH_DECL_CPU_SET_T +#elif defined(WITH_SCHED_GETAFFINITY) int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet) { @@ -592,7 +592,7 @@ virProcessGetAffinity(pid_t pid) return ret; } -#else /* WITH_DECL_CPU_SET_T */ +#else /* ! (defined(WITH_BSD_CPU_AFFINITY) || defined(WITH_SCHED_GETAFFINITY)) */ int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED, virBitmap *map G_GNUC_UNUSED, @@ -612,7 +612,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED) _("Process CPU affinity is not supported on this platform")); return NULL; } -#endif /* WITH_DECL_CPU_SET_T */ +#endif /* ! (defined(WITH_BSD_CPU_AFFINITY) || defined(WITH_SCHED_GETAFFINITY)) */ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids) -- 2.43.2

virProcessSetScheduler() uses not just sched_setscheduler() but also sched_get_priority_{min,max}(). Currently we assume that the former being available implies that the latter are as well, but that's not the case for at least GNU/Hurd. Make sure all functions are actually available before attempting to use them. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 1 + src/util/virprocess.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/meson.build b/meson.build index a80a65e447..7f68c9da89 100644 --- a/meson.build +++ b/meson.build @@ -584,6 +584,7 @@ functions = [ 'posix_fallocate', 'posix_memalign', 'prlimit', + 'sched_get_priority_min', 'sched_getaffinity', 'sched_setscheduler', 'setgroups', diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8e4440b45f..5cdf3e3eb9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1573,7 +1573,7 @@ virProcessExitWithStatus(int status) exit(value); } -#if WITH_SCHED_SETSCHEDULER +#if defined(WITH_SCHED_SETSCHEDULER) && defined(WITH_SCHED_GET_PRIORITY_MIN) static int virProcessSchedTranslatePolicy(virProcessSchedPolicy policy) @@ -1667,7 +1667,7 @@ virProcessSetScheduler(pid_t pid, return 0; } -#else /* ! WITH_SCHED_SETSCHEDULER */ +#else /* ! (defined(WITH_SCHED_SETSCHEDULER) && defined(WITH_SCHED_GET_PRIORITY_MIN)) */ int virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, @@ -1682,7 +1682,7 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, return -1; } -#endif /* !WITH_SCHED_SETSCHEDULER */ +#endif /* ! (defined(WITH_SCHED_SETSCHEDULER) && defined(WITH_SCHED_GET_PRIORITY_MIN)) */ /* * Get all stat fields for a process based on pid and tid: -- 2.43.2

On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N...
Any reason for not merging these ^^^ yet? It would be nice to merge them before the release and have one broken release less. Michal

On Wed, Feb 28, 2024 at 12:37:54PM +0100, Michal Prívozník wrote:
On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N...
Any reason for not merging these ^^^ yet? It would be nice to merge them before the release and have one broken release less.
I'm giving Roman a chance to provide feedback, since I'm changing code that he introduced specifically for FreeBSD. -- Andrea Bolognani / Red Hat / Virtualization

On 2/28/24 13:40, Andrea Bolognani wrote:
On Wed, Feb 28, 2024 at 12:37:54PM +0100, Michal Prívozník wrote:
On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N...
Any reason for not merging these ^^^ yet? It would be nice to merge them before the release and have one broken release less.
I'm giving Roman a chance to provide feedback, since I'm changing code that he introduced specifically for FreeBSD.
I think there was enough room given for feedback. Unfortunately, it missed the release, but there's no reason for it to miss another one. Michal

On Fri, Mar 08, 2024 at 10:47:12AM +0100, Michal Prívozník wrote:
On 2/28/24 13:40, Andrea Bolognani wrote:
On Wed, Feb 28, 2024 at 12:37:54PM +0100, Michal Prívozník wrote:
On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N...
Any reason for not merging these ^^^ yet? It would be nice to merge them before the release and have one broken release less.
I'm giving Roman a chance to provide feedback, since I'm changing code that he introduced specifically for FreeBSD.
I think there was enough room given for feedback. Unfortunately, it missed the release, but there's no reason for it to miss another one.
Agreed. I'm off next week, but I'll push the patches once I come back unless I hear otherwise. That will still give us plenty of time to revert them before release, should that turn out to be necessary. I could also push the patches in this series at the same time, if someone ACKed them... *hint hint* ;) -- Andrea Bolognani / Red Hat / Virtualization

On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N... [2] https://gitlab.com/abologna/libvirt/-/pipelines/1192942974
Andrea Bolognani (2): meson: Restore check for sched_getaffinity() meson: Check for sched_get_priority_min()
meson.build | 4 ++-- src/ch/ch_process.c | 1 + src/util/virprocess.c | 12 ++++++------ 3 files changed, 9 insertions(+), 8 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> but let me just say that I had to dig [1] out of the archives only to apply these cleanly (even though they were reviewed long time ago and agreed to be pushed "soon") did not help. Michal

On Tue, Mar 19, 2024 at 05:45:10PM +0100, Michal Prívozník wrote:
On 2/27/24 19:30, Andrea Bolognani wrote:
This applies on top of [1]. Test pipeline: [2]
Upon further investigation, I have determined that not only we are unintentionally using the Linux APIs on FreeBSD instead of the native ones, but we are also accidentally skipping some setup steps on Linux.
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/CX26N... [2] https://gitlab.com/abologna/libvirt/-/pipelines/1192942974
Andrea Bolognani (2): meson: Restore check for sched_getaffinity() meson: Check for sched_get_priority_min()
meson.build | 4 ++-- src/ch/ch_process.c | 1 + src/util/virprocess.c | 12 ++++++------ 3 files changed, 9 insertions(+), 8 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
but let me just say that I had to dig [1] out of the archives only to apply these cleanly (even though they were reviewed long time ago and agreed to be pushed "soon") did not help.
Yeah, sorry for the annoyance. And thanks a lot for the review! I've pushed everything now. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani
-
Michal Prívozník