tests/migration: use the common library function

[PATCH] tests/migration: use the common library function

Posted by Alex Bennée 6 years ago

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 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)
-- 
2.20.1

Re: [PATCH] tests/migration: use the common library function

Posted by Thomas Huth 6 years ago

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)
>

Re: [PATCH] tests/migration: use the common library function

Posted by Alex Bennée 6 years ago

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

Re: [PATCH] tests/migration: use the common library function

Posted by Thomas Huth 6 years ago

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
>

Re: [PATCH] tests/migration: use the common library function

Posted by Peter Maydell 6 years ago

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

Re: [PATCH] tests/migration: use the common library function

Posted by Alex Bennée 6 years ago

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