| Message ID | 20191111125530.26579-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | tests/migration: use the common library function | expand |
On 11/11/2019 13.55, Alex Bennée wrote: > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Could you please add at least a short patch description? (Why is this change necessary / a good idea?) Thanks, Thomas > --- > tests/migration/stress.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/migration/stress.c b/tests/migration/stress.c > index 0c239646934..915389b53ae 100644 > --- a/tests/migration/stress.c > +++ b/tests/migration/stress.c > @@ -31,7 +31,7 @@ const char *argv0; > > static int gettid(void) > { > - return syscall(SYS_gettid); > + return qemu_get_thread_id(); > } > > static __attribute__((noreturn)) void exit_failure(void) >
Thomas Huth <thuth@redhat.com> writes: > On 11/11/2019 13.55, Alex Bennée wrote: >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > Could you please add at least a short patch description? (Why is this > change necessary / a good idea?) It's just a minor clean-up Dave happened to comment on last week. Using the helper function is preferable given it abstracts away any system differences for the same information. This is unlike linux-user which has it's own reasons for using syscall wrappers. > > Thanks, > Thomas > > >> --- >> tests/migration/stress.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tests/migration/stress.c b/tests/migration/stress.c >> index 0c239646934..915389b53ae 100644 >> --- a/tests/migration/stress.c >> +++ b/tests/migration/stress.c >> @@ -31,7 +31,7 @@ const char *argv0; >> >> static int gettid(void) >> { >> - return syscall(SYS_gettid); >> + return qemu_get_thread_id(); >> } >> >> static __attribute__((noreturn)) void exit_failure(void) >> -- Alex Bennée
On 11/11/2019 15.11, Alex Bennée wrote: > > Thomas Huth <thuth@redhat.com> writes: > >> On 11/11/2019 13.55, Alex Bennée wrote: >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> Could you please add at least a short patch description? (Why is this >> change necessary / a good idea?) > > It's just a minor clean-up Dave happened to comment on last week. Using > the helper function is preferable given it abstracts away any system > differences for the same information. But this also changes the behavior on non-Linux systems (i.e. the *BSDs and macOS), since they will now use getpid() instead of gettid ... is that the intended change here? Thomas >>> --- >>> tests/migration/stress.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tests/migration/stress.c b/tests/migration/stress.c >>> index 0c239646934..915389b53ae 100644 >>> --- a/tests/migration/stress.c >>> +++ b/tests/migration/stress.c >>> @@ -31,7 +31,7 @@ const char *argv0; >>> >>> static int gettid(void) >>> { >>> - return syscall(SYS_gettid); >>> + return qemu_get_thread_id(); >>> } >>> >>> static __attribute__((noreturn)) void exit_failure(void) >>> > > > -- > Alex Bennée >
On Mon, 11 Nov 2019 at 14:41, Thomas Huth <thuth@redhat.com> wrote: > > On 11/11/2019 15.11, Alex Bennée wrote: > > > > Thomas Huth <thuth@redhat.com> writes: > > > >> On 11/11/2019 13.55, Alex Bennée wrote: > >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> > >> Could you please add at least a short patch description? (Why is this > >> change necessary / a good idea?) > > > > It's just a minor clean-up Dave happened to comment on last week. Using > > the helper function is preferable given it abstracts away any system > > differences for the same information. > > But this also changes the behavior on non-Linux systems (i.e. the *BSDs > and macOS), since they will now use getpid() instead of gettid ... is > that the intended change here? Does the 'stress' program work on those OSes? For that matter, does it work on Linux? As far as I can tell we don't compile stress.c on any host, since the only thing that depends on tests/migration/stress$(EXESUF) is tests/migration/initrd-stress.img, and nothing depends on that. Nothing creates tests/migration/ in the build dir so trying to build tests/migration/stress in an out-of-tree config fails: CC tests/migration/stress.o /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:359:1: fatal error: opening dependency file tests/migration/stress.d: No such file or directory } ^ compilation terminated. ...and if I fix that by manually creating the directory then it fails to link: CC tests/migration/stress.o LINK tests/migration/stress tests/migration/stress.o: In function `get_command_arg_str': /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:107: undefined reference to `g_strndup' /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:109: undefined reference to `g_strdup' tests/migration/stress.o: In function `get_command_arg_ull': /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:129: undefined reference to `g_free' /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:132: undefined reference to `g_free' tests/migration/stress.o: In function `stress': /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:253: undefined reference to `pthread_create' collect2: error: ld returned 1 exit status /home/petmay01/linaro/qemu-from-laptop/qemu/tests/Makefile.include:849: recipe for target 'tests/migration/stress' failed Is this dead code ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 11 Nov 2019 at 14:41, Thomas Huth <thuth@redhat.com> wrote: >> >> On 11/11/2019 15.11, Alex Bennée wrote: >> > >> > Thomas Huth <thuth@redhat.com> writes: >> > >> >> On 11/11/2019 13.55, Alex Bennée wrote: >> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> >> >> Could you please add at least a short patch description? (Why is this >> >> change necessary / a good idea?) >> > >> > It's just a minor clean-up Dave happened to comment on last week. Using >> > the helper function is preferable given it abstracts away any system >> > differences for the same information. >> >> But this also changes the behavior on non-Linux systems (i.e. the *BSDs >> and macOS), since they will now use getpid() instead of gettid ... is >> that the intended change here? > > Does the 'stress' program work on those OSes? For that matter, > does it work on Linux? > > As far as I can tell we don't compile stress.c on any host, > since the only thing that depends on tests/migration/stress$(EXESUF) > is tests/migration/initrd-stress.img, and nothing depends on that. > > Nothing creates tests/migration/ in the build dir so trying > to build tests/migration/stress in an out-of-tree config fails: > > CC tests/migration/stress.o > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:359:1: > fatal error: opening dependency file tests/migration/stress.d: No such > file or directory > } > ^ > compilation terminated. > > ...and if I fix that by manually creating the directory then > it fails to link: > > CC tests/migration/stress.o > LINK tests/migration/stress > tests/migration/stress.o: In function `get_command_arg_str': > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:107: > undefined reference to `g_strndup' > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:109: > undefined reference to `g_strdup' > tests/migration/stress.o: In function `get_command_arg_ull': > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:129: > undefined reference to `g_free' > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:132: > undefined reference to `g_free' > tests/migration/stress.o: In function `stress': > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration/stress.c:253: > undefined reference to `pthread_create' > collect2: error: ld returned 1 exit status > /home/petmay01/linaro/qemu-from-laptop/qemu/tests/Makefile.include:849: > recipe for target 'tests/migration/stress' failed > > Is this dead code ? It was introduced around 3 years ago by Daniel for stress testing. The instructions in: 409437e16df273fc5f78f6cd1cb53023eaeb9b72 Author: Daniel P. Berrangé <berrange@redhat.com> AuthorDate: Wed Jul 20 14:23:13 2016 +0100 Commit: Amit Shah <amit.shah@redhat.com> CommitDate: Fri Jul 22 13:23:39 2016 +0530 tests: introduce a framework for testing migration performance say to use: make tests/migration/initrd-stress.img And that has indeed bitrotted over time. All the other tweaks since are passing through clean ups. > > thanks > -- PMM -- Alex Bennée
diff --git a/tests/migration/stress.c b/tests/migration/stress.c index 0c239646934..915389b53ae 100644 --- a/tests/migration/stress.c +++ b/tests/migration/stress.c @@ -31,7 +31,7 @@ const char *argv0; static int gettid(void) { - return syscall(SYS_gettid); + return qemu_get_thread_id(); } static __attribute__((noreturn)) void exit_failure(void)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- tests/migration/stress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)