| Message ID | 20241217212201.23376-1-farosas@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest/migration: Restore include for postcopy | expand |
On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to > utils") moved the ufd_version_check() function to another file but > failed to bring along the <sys/syscall> include, which is necessary to > pull in <asm/unistd.h> for __NR_userfaultd. > > Restore the missing include. Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth explicit mention that in the commit message if so, only when you merge. > > While here, remove the ifdef __linux__ that's redundant and fix a > couple of typos. > > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") > Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Maybe we don't need to be as careful on old kernels anymore especially in tests, because userfaultfd syscall existed for ~10 years. So if we want we can start requiring __NR_userfaultfd present for __linux__, then no way to miss such spot next time.
On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote: > On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: > > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to > > utils") moved the ufd_version_check() function to another file but > > failed to bring along the <sys/syscall> include, which is necessary to > > pull in <asm/unistd.h> for __NR_userfaultd. > > > > Restore the missing include. > > Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth > explicit mention that in the commit message if so, only when you merge. > > > > > While here, remove the ifdef __linux__ that's redundant and fix a > > couple of typos. > > > > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Maybe we don't need to be as careful on old kernels anymore especially in > tests, because userfaultfd syscall existed for ~10 years. So if we want we > can start requiring __NR_userfaultfd present for __linux__, then no way to > miss such spot next time. Yes, I think that check is obsolete, based on our supported platforms list. It would suffice to just check __linux__. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote: >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to >> > utils") moved the ufd_version_check() function to another file but >> > failed to bring along the <sys/syscall> include, which is necessary to >> > pull in <asm/unistd.h> for __NR_userfaultd. >> > >> > Restore the missing include. >> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth >> explicit mention that in the commit message if so, only when you merge. >> >> > >> > While here, remove the ifdef __linux__ that's redundant and fix a >> > couple of typos. >> > >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> >> >> Reviewed-by: Peter Xu <peterx@redhat.com> >> >> Maybe we don't need to be as careful on old kernels anymore especially in >> tests, because userfaultfd syscall existed for ~10 years. So if we want we >> can start requiring __NR_userfaultfd present for __linux__, then no way to >> miss such spot next time. > > Yes, I think that check is obsolete, based on our supported platforms > list. It would suffice to just check __linux__. This breaks the cross builds. It seems the __NR_userfaultfd was actually stopping several archs from reaching ufd_version_check(). Since <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find the 'ioctl' symbol: https://gitlab.com/farosas/qemu/-/pipelines/1594332399 Of course I could just include <sys/ioctl.h> unconditionally, but the fact that new code is not being built means the assumption that we can imply __NR_userfaultfd from __linux__ alone is not correct.
On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote: > >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: > >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to > >> > utils") moved the ufd_version_check() function to another file but > >> > failed to bring along the <sys/syscall> include, which is necessary to > >> > pull in <asm/unistd.h> for __NR_userfaultd. > >> > > >> > Restore the missing include. > >> > >> Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth > >> explicit mention that in the commit message if so, only when you merge. > >> > >> > > >> > While here, remove the ifdef __linux__ that's redundant and fix a > >> > couple of typos. > >> > > >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> > >> Reviewed-by: Peter Xu <peterx@redhat.com> > >> > >> Maybe we don't need to be as careful on old kernels anymore especially in > >> tests, because userfaultfd syscall existed for ~10 years. So if we want we > >> can start requiring __NR_userfaultfd present for __linux__, then no way to > >> miss such spot next time. > > > > Yes, I think that check is obsolete, based on our supported platforms > > list. It would suffice to just check __linux__. > > This breaks the cross builds. It seems the __NR_userfaultfd was actually > stopping several archs from reaching ufd_version_check(). Since > <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find > the 'ioctl' symbol: > > https://gitlab.com/farosas/qemu/-/pipelines/1594332399 > > Of course I could just include <sys/ioctl.h> unconditionally, but the > fact that new code is not being built means the assumption that we can > imply __NR_userfaultfd from __linux__ alone is not correct. What is the new code referenced here? I still don't yet understand how it would fail if we move sys/ioctl.h out. PS: we don't necessarily need to do it right now to remove that ifdef, but I'm just curious.
On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote: > >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: > >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to > >> > utils") moved the ufd_version_check() function to another file but > >> > failed to bring along the <sys/syscall> include, which is necessary to > >> > pull in <asm/unistd.h> for __NR_userfaultd. > >> > > >> > Restore the missing include. > >> > >> Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth > >> explicit mention that in the commit message if so, only when you merge. > >> > >> > > >> > While here, remove the ifdef __linux__ that's redundant and fix a > >> > couple of typos. > >> > > >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> > >> Reviewed-by: Peter Xu <peterx@redhat.com> > >> > >> Maybe we don't need to be as careful on old kernels anymore especially in > >> tests, because userfaultfd syscall existed for ~10 years. So if we want we > >> can start requiring __NR_userfaultfd present for __linux__, then no way to > >> miss such spot next time. > > > > Yes, I think that check is obsolete, based on our supported platforms > > list. It would suffice to just check __linux__. > > This breaks the cross builds. It seems the __NR_userfaultfd was actually > stopping several archs from reaching ufd_version_check(). Since > <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find > the 'ioctl' symbol: > > https://gitlab.com/farosas/qemu/-/pipelines/1594332399 > > Of course I could just include <sys/ioctl.h> unconditionally, but the > fact that new code is not being built means the assumption that we can > imply __NR_userfaultfd from __linux__ alone is not correct. I think removing __NR_userfaultfd is still correct. The problem is that the ufd_version_check code is wrapped in a conditional that is not the same as the conditional that pulls in ioctl.h. That pre-existing bug should be fixed regardless, and once that's done, removing __NR_userfaultfd wouldn't be an issue. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Dec 18, 2024 at 12:12:11PM -0300, Fabiano Rosas wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Tue, Dec 17, 2024 at 04:47:39PM -0500, Peter Xu wrote: >> >> On Tue, Dec 17, 2024 at 06:22:01PM -0300, Fabiano Rosas wrote: >> >> > Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to >> >> > utils") moved the ufd_version_check() function to another file but >> >> > failed to bring along the <sys/syscall> include, which is necessary to >> >> > pull in <asm/unistd.h> for __NR_userfaultd. >> >> > >> >> > Restore the missing include. >> >> >> >> Ohhhhhhh.. so postcopy tests will always be skipped as of now? Maybe worth >> >> explicit mention that in the commit message if so, only when you merge. >> >> >> >> > >> >> > While here, remove the ifdef __linux__ that's redundant and fix a >> >> > couple of typos. >> >> > >> >> > Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") >> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> >> >> >> >> Reviewed-by: Peter Xu <peterx@redhat.com> >> >> >> >> Maybe we don't need to be as careful on old kernels anymore especially in >> >> tests, because userfaultfd syscall existed for ~10 years. So if we want we >> >> can start requiring __NR_userfaultfd present for __linux__, then no way to >> >> miss such spot next time. >> > >> > Yes, I think that check is obsolete, based on our supported platforms >> > list. It would suffice to just check __linux__. >> >> This breaks the cross builds. It seems the __NR_userfaultfd was actually >> stopping several archs from reaching ufd_version_check(). Since >> <sys/ioctl.h> is under HOST_X86_64, these new instances now fail to find >> the 'ioctl' symbol: >> >> https://gitlab.com/farosas/qemu/-/pipelines/1594332399 >> >> Of course I could just include <sys/ioctl.h> unconditionally, but the >> fact that new code is not being built means the assumption that we can >> imply __NR_userfaultfd from __linux__ alone is not correct. > > I think removing __NR_userfaultfd is still correct. The problem is that > the ufd_version_check code is wrapped in a conditional that is not the > same as the conditional that pulls in ioctl.h. That pre-existing bug > should be fixed regardless, and once that's done, removing __NR_userfaultfd > wouldn't be an issue. Ah, the <sys/ioctl.h> include used to be under the userfaultfd ifdefs. I got confused because kvm_dirty_ring_supported() also needs the ioctl and it was introduced without moving the ioctl to a common ifdef. > > With regards, > Daniel
diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c index 525bf1eed4..d18841e357 100644 --- a/tests/qtest/migration/migration-util.c +++ b/tests/qtest/migration/migration-util.c @@ -22,8 +22,12 @@ #include "migration/bootfile.h" #include "migration/migration-util.h" -/* for uffd_version_check() */ -#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD) +#if defined(__linux__) +#include <sys/syscall.h> +#endif + +/* for ufd_version_check() */ +#if defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD) #include <sys/eventfd.h> #include "qemu/userfaultfd.h" #endif @@ -297,7 +301,7 @@ bool probe_o_direct_support(const char *tmpfs) } #endif -#if defined(__linux__) && defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD) +#if defined(__NR_userfaultfd) && defined(CONFIG_EVENTFD) bool ufd_version_check(bool *uffd_feature_thread_id) { struct uffdio_api api_struct; @@ -333,7 +337,7 @@ bool ufd_version_check(bool *uffd_feature_thread_id) #else bool ufd_version_check(bool *uffd_feature_thread_id) { - g_test_message("Skipping test: Userfault not available (builtdtime)"); + g_test_message("Skipping test: Userfault not available (buildtime)"); return false; } #endif
Commit 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") moved the ufd_version_check() function to another file but failed to bring along the <sys/syscall> include, which is necessary to pull in <asm/unistd.h> for __NR_userfaultd. Restore the missing include. While here, remove the ifdef __linux__ that's redundant and fix a couple of typos. Fixes: 124a3c58b8 ("tests/qtest/migration: Move ufd_version_check to utils") Signed-off-by: Fabiano Rosas <farosas@suse.de> --- tests/qtest/migration/migration-util.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)