1. Patchew
  2. Libvirt
  3. View series

[PATCH] meson: improve CPU affinity routines check

Roman Bogorodskiy posted 1 patch 2 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20211121155855.15213-1-bogorodskiy@gmail.com
meson.build           | 4 +++-
src/util/virprocess.c | 8 ++++----
2 files changed, 7 insertions(+), 5 deletions(-)
[PATCH] meson: improve CPU affinity routines check
Posted by Roman Bogorodskiy 2 years, 5 months ago 
Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
the sched.h header as well [1]. To make these routines visible,
users have to define _WITH_CPU_SET_T.

This breaks current detection. Specifically, meson sees the
sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
define unlocks Linux implementation of virProcessSetAffinity() and other
functions, which fails to build on FreeBSD because cpu_set_t is not
visible as _WITH_CPU_SET_T is not defined.

For now, change detection to the following:

 - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
   available through sched.h
 - Explicitly check the sched.h header instead of assuming its presence
   if WITH_SCHED_SETSCHEDULER is defined

1:
https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
---
 meson.build           | 4 +++-
 src/util/virprocess.c | 8 ++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 9022bcfdc9..e4f36e8574 100644
--- a/meson.build
+++ b/meson.build
@@ -553,7 +553,6 @@ functions = [
   'posix_fallocate',
   'posix_memalign',
   'prlimit',
-  'sched_getaffinity',
   'sched_setscheduler',
   'setgroups',
   'setns',
@@ -602,6 +601,7 @@ headers = [
   'net/if.h',
   'pty.h',
   'pwd.h',
+  'sched.h',
   'sys/auxv.h',
   'sys/ioctl.h',
   'sys/mount.h',
@@ -671,6 +671,8 @@ 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/util/virprocess.c b/src/util/virprocess.c
index 6de3f36f52..81de90200e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -35,7 +35,7 @@
 # include <sys/time.h>
 # include <sys/resource.h>
 #endif
-#if WITH_SCHED_SETSCHEDULER
+#if WITH_SCHED_H
 # include <sched.h>
 #endif
 
@@ -480,7 +480,7 @@ int virProcessKillPainfully(pid_t pid, bool force)
     return virProcessKillPainfullyDelay(pid, force, 0, false);
 }
 
-#if WITH_SCHED_GETAFFINITY
+#if WITH_DECL_CPU_SET_T
 
 int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet)
 {
@@ -626,7 +626,7 @@ virProcessGetAffinity(pid_t pid)
     return ret;
 }
 
-#else /* WITH_SCHED_GETAFFINITY */
+#else /* WITH_DECL_CPU_SET_T */
 
 int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED,
                           virBitmap *map G_GNUC_UNUSED,
@@ -646,7 +646,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED)
                          _("Process CPU affinity is not supported on this platform"));
     return NULL;
 }
-#endif /* WITH_SCHED_GETAFFINITY */
+#endif /* WITH_DECL_CPU_SET_T */
 
 
 int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
-- 
2.33.1
Re: [PATCH] meson: improve CPU affinity routines check
Posted by Martin Kletzander 2 years, 5 months ago 
On Sun, Nov 21, 2021 at 07:58:55PM +0400, Roman Bogorodskiy wrote:
>Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
>the sched.h header as well [1]. To make these routines visible,
>users have to define _WITH_CPU_SET_T.
>
>This breaks current detection. Specifically, meson sees the
>sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
>define unlocks Linux implementation of virProcessSetAffinity() and other
>functions, which fails to build on FreeBSD because cpu_set_t is not
>visible as _WITH_CPU_SET_T is not defined.
>
>For now, change detection to the following:
>
> - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
>   available through sched.h
> - Explicitly check the sched.h header instead of assuming its presence
>   if WITH_SCHED_SETSCHEDULER is defined
>

Makes sense, although even the sched_getaffinity() is guarded by the new
_WITH_CPU_SET_T so there should be no error with the current code, am I
right?  And if I understand correctly we cannot use the FreeBSD
implementation as is because they do not have all the CPU_* macros we
need, right?

>1:
>https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
>https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
>https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
>
>Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>---
> meson.build           | 4 +++-
> src/util/virprocess.c | 8 ++++----
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
>diff --git a/meson.build b/meson.build
>index 9022bcfdc9..e4f36e8574 100644
>--- a/meson.build
>+++ b/meson.build
>@@ -553,7 +553,6 @@ functions = [
>   'posix_fallocate',
>   'posix_memalign',
>   'prlimit',
>-  'sched_getaffinity',
>   'sched_setscheduler',
>   'setgroups',
>   'setns',
>@@ -602,6 +601,7 @@ headers = [
>   'net/if.h',
>   'pty.h',
>   'pwd.h',
>+  'sched.h',
>   'sys/auxv.h',
>   'sys/ioctl.h',
>   'sys/mount.h',
>@@ -671,6 +671,8 @@ 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/util/virprocess.c b/src/util/virprocess.c
>index 6de3f36f52..81de90200e 100644
>--- a/src/util/virprocess.c
>+++ b/src/util/virprocess.c
>@@ -35,7 +35,7 @@
> # include <sys/time.h>
> # include <sys/resource.h>
> #endif
>-#if WITH_SCHED_SETSCHEDULER
>+#if WITH_SCHED_H
> # include <sched.h>
> #endif
>
>@@ -480,7 +480,7 @@ int virProcessKillPainfully(pid_t pid, bool force)
>     return virProcessKillPainfullyDelay(pid, force, 0, false);
> }
>
>-#if WITH_SCHED_GETAFFINITY
>+#if WITH_DECL_CPU_SET_T
>
> int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet)
> {
>@@ -626,7 +626,7 @@ virProcessGetAffinity(pid_t pid)
>     return ret;
> }
>
>-#else /* WITH_SCHED_GETAFFINITY */
>+#else /* WITH_DECL_CPU_SET_T */
>
> int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED,
>                           virBitmap *map G_GNUC_UNUSED,
>@@ -646,7 +646,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED)
>                          _("Process CPU affinity is not supported on this platform"));
>     return NULL;
> }
>-#endif /* WITH_SCHED_GETAFFINITY */
>+#endif /* WITH_DECL_CPU_SET_T */
>
>
> int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
>-- 
>2.33.1
>
Re: [PATCH] meson: improve CPU affinity routines check
Posted by Roman Bogorodskiy 2 years, 5 months ago 
  Martin Kletzander wrote:

> On Sun, Nov 21, 2021 at 07:58:55PM +0400, Roman Bogorodskiy wrote:
> >Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
> >the sched.h header as well [1]. To make these routines visible,
> >users have to define _WITH_CPU_SET_T.
> >
> >This breaks current detection. Specifically, meson sees the
> >sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
> >define unlocks Linux implementation of virProcessSetAffinity() and other
> >functions, which fails to build on FreeBSD because cpu_set_t is not
> >visible as _WITH_CPU_SET_T is not defined.
> >
> >For now, change detection to the following:
> >
> > - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
> >   available through sched.h
> > - Explicitly check the sched.h header instead of assuming its presence
> >   if WITH_SCHED_SETSCHEDULER is defined
> >
> 
> Makes sense, although even the sched_getaffinity() is guarded by the new
> _WITH_CPU_SET_T so there should be no error with the current code, am I

Nope, as I tried to explain in the commit message, the current code is
not working properly as it's using cc.has_function() for 'sched_getaffinity'
and this check passes because it's checking the specified function in
the standard library, so it works regardless of _WITH_CPU_SET_T. And as
it finds the sched_getaffinity(), the build still fails because we don't
set _WITH_CPU_SET_T.

> right?  And if I understand correctly we cannot use the FreeBSD
> implementation as is because they do not have all the CPU_* macros we
> need, right?

Frankly speaking, I haven't yet tested this implementation. Currently,
virprocess.c is using the original FreeBSD interface: cpuset_(get|set)affinity.
Generally, it would be a good thing to use the same implementation for
both Linux and FreeBSD, however, this Linux-compatible interface in
FreeBSD is not yet available in any FreeBSD release, so we'll have to keep
the old code for some time anyway, and also I need to test if it
actually works for us and figure out how to better detect and pass this _WITH_CPU_SET_T
with meson.

> >1:
> >https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
> >https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
> >https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
> >
> >Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> >---
> > meson.build           | 4 +++-
> > src/util/virprocess.c | 8 ++++----
> > 2 files changed, 7 insertions(+), 5 deletions(-)
> >
> >diff --git a/meson.build b/meson.build
> >index 9022bcfdc9..e4f36e8574 100644
> >--- a/meson.build
> >+++ b/meson.build
> >@@ -553,7 +553,6 @@ functions = [
> >   'posix_fallocate',
> >   'posix_memalign',
> >   'prlimit',
> >-  'sched_getaffinity',
> >   'sched_setscheduler',
> >   'setgroups',
> >   'setns',
> >@@ -602,6 +601,7 @@ headers = [
> >   'net/if.h',
> >   'pty.h',
> >   'pwd.h',
> >+  'sched.h',
> >   'sys/auxv.h',
> >   'sys/ioctl.h',
> >   'sys/mount.h',
> >@@ -671,6 +671,8 @@ 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/util/virprocess.c b/src/util/virprocess.c
> >index 6de3f36f52..81de90200e 100644
> >--- a/src/util/virprocess.c
> >+++ b/src/util/virprocess.c
> >@@ -35,7 +35,7 @@
> > # include <sys/time.h>
> > # include <sys/resource.h>
> > #endif
> >-#if WITH_SCHED_SETSCHEDULER
> >+#if WITH_SCHED_H
> > # include <sched.h>
> > #endif
> >
> >@@ -480,7 +480,7 @@ int virProcessKillPainfully(pid_t pid, bool force)
> >     return virProcessKillPainfullyDelay(pid, force, 0, false);
> > }
> >
> >-#if WITH_SCHED_GETAFFINITY
> >+#if WITH_DECL_CPU_SET_T
> >
> > int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet)
> > {
> >@@ -626,7 +626,7 @@ virProcessGetAffinity(pid_t pid)
> >     return ret;
> > }
> >
> >-#else /* WITH_SCHED_GETAFFINITY */
> >+#else /* WITH_DECL_CPU_SET_T */
> >
> > int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED,
> >                           virBitmap *map G_GNUC_UNUSED,
> >@@ -646,7 +646,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED)
> >                          _("Process CPU affinity is not supported on this platform"));
> >     return NULL;
> > }
> >-#endif /* WITH_SCHED_GETAFFINITY */
> >+#endif /* WITH_DECL_CPU_SET_T */
> >
> >
> > int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
> >-- 
> >2.33.1
> >



Roman Bogorodskiy
Re: [PATCH] meson: improve CPU affinity routines check
Posted by Martin Kletzander 2 years, 5 months ago 
On Mon, Nov 22, 2021 at 03:22:11PM +0400, Roman Bogorodskiy wrote:
>  Martin Kletzander wrote:
>
>> On Sun, Nov 21, 2021 at 07:58:55PM +0400, Roman Bogorodskiy wrote:
>> >Recently, FreeBSD has got sched_get/setaffinity(3) implementations and
>> >the sched.h header as well [1]. To make these routines visible,
>> >users have to define _WITH_CPU_SET_T.
>> >
>> >This breaks current detection. Specifically, meson sees the
>> >sched_getaffinity() symbol and defines WITH_SCHED_GETAFFINITY. This
>> >define unlocks Linux implementation of virProcessSetAffinity() and other
>> >functions, which fails to build on FreeBSD because cpu_set_t is not
>> >visible as _WITH_CPU_SET_T is not defined.
>> >
>> >For now, change detection to the following:
>> >
>> > - Instead of checking sched_getaffinity(), check if 'cpu_set_t' is
>> >   available through sched.h
>> > - Explicitly check the sched.h header instead of assuming its presence
>> >   if WITH_SCHED_SETSCHEDULER is defined
>> >
>>
>> Makes sense, although even the sched_getaffinity() is guarded by the new
>> _WITH_CPU_SET_T so there should be no error with the current code, am I
>
>Nope, as I tried to explain in the commit message, the current code is
>not working properly as it's using cc.has_function() for 'sched_getaffinity'
>and this check passes because it's checking the specified function in
>the standard library, so it works regardless of _WITH_CPU_SET_T. And as
>it finds the sched_getaffinity(), the build still fails because we don't
>set _WITH_CPU_SET_T.
>

Looking at the patches you linked to it seems like sched_getaffinity()
is hidden under #ifdef _WITH_CPU_SET_T, which made me think that
cc.has_function() should not find it.

Anyway the code is fine as it is, I was just wondering.

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>> right?  And if I understand correctly we cannot use the FreeBSD
>> implementation as is because they do not have all the CPU_* macros we
>> need, right?
>
>Frankly speaking, I haven't yet tested this implementation. Currently,
>virprocess.c is using the original FreeBSD interface: cpuset_(get|set)affinity.
>Generally, it would be a good thing to use the same implementation for
>both Linux and FreeBSD, however, this Linux-compatible interface in
>FreeBSD is not yet available in any FreeBSD release, so we'll have to keep
>the old code for some time anyway, and also I need to test if it
>actually works for us and figure out how to better detect and pass this _WITH_CPU_SET_T
>with meson.
>

Maybe one day =)

>> >1:
>> >https://cgit.freebsd.org/src/commit/?id=43736b71dd051212d5c55be9fa21c45993017fbb
>> >https://cgit.freebsd.org/src/commit/?id=160b4b922b6021848b6b48afc894d16b879b7af2
>> >https://cgit.freebsd.org/src/commit/?id=90fa9705d5cd29cf11c5dc7319299788dec2546a
>> >
>> >Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
>> >---
>> > meson.build           | 4 +++-
>> > src/util/virprocess.c | 8 ++++----
>> > 2 files changed, 7 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/meson.build b/meson.build
>> >index 9022bcfdc9..e4f36e8574 100644
>> >--- a/meson.build
>> >+++ b/meson.build
>> >@@ -553,7 +553,6 @@ functions = [
>> >   'posix_fallocate',
>> >   'posix_memalign',
>> >   'prlimit',
>> >-  'sched_getaffinity',
>> >   'sched_setscheduler',
>> >   'setgroups',
>> >   'setns',
>> >@@ -602,6 +601,7 @@ headers = [
>> >   'net/if.h',
>> >   'pty.h',
>> >   'pwd.h',
>> >+  'sched.h',
>> >   'sys/auxv.h',
>> >   'sys/ioctl.h',
>> >   'sys/mount.h',
>> >@@ -671,6 +671,8 @@ 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/util/virprocess.c b/src/util/virprocess.c
>> >index 6de3f36f52..81de90200e 100644
>> >--- a/src/util/virprocess.c
>> >+++ b/src/util/virprocess.c
>> >@@ -35,7 +35,7 @@
>> > # include <sys/time.h>
>> > # include <sys/resource.h>
>> > #endif
>> >-#if WITH_SCHED_SETSCHEDULER
>> >+#if WITH_SCHED_H
>> > # include <sched.h>
>> > #endif
>> >
>> >@@ -480,7 +480,7 @@ int virProcessKillPainfully(pid_t pid, bool force)
>> >     return virProcessKillPainfullyDelay(pid, force, 0, false);
>> > }
>> >
>> >-#if WITH_SCHED_GETAFFINITY
>> >+#if WITH_DECL_CPU_SET_T
>> >
>> > int virProcessSetAffinity(pid_t pid, virBitmap *map, bool quiet)
>> > {
>> >@@ -626,7 +626,7 @@ virProcessGetAffinity(pid_t pid)
>> >     return ret;
>> > }
>> >
>> >-#else /* WITH_SCHED_GETAFFINITY */
>> >+#else /* WITH_DECL_CPU_SET_T */
>> >
>> > int virProcessSetAffinity(pid_t pid G_GNUC_UNUSED,
>> >                           virBitmap *map G_GNUC_UNUSED,
>> >@@ -646,7 +646,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED)
>> >                          _("Process CPU affinity is not supported on this platform"));
>> >     return NULL;
>> > }
>> >-#endif /* WITH_SCHED_GETAFFINITY */
>> >+#endif /* WITH_DECL_CPU_SET_T */
>> >
>> >
>> > int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
>> >--
>> >2.33.1
>> >
>
>
>
>Roman Bogorodskiy

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.