Re: [PATCH v3 0/5] Guestperf: miscellaneous refinement and enrichment

[Peter Xu <peterx@redhat.com>]

On Sat, Nov 30, 2024 at 10:01:43PM +0800, Yong Huang wrote:
> On Fri, Nov 29, 2024 at 9:44 PM Fabiano Rosas <farosas@suse.de> wrote:
> 
> > Fabiano Rosas <farosas@suse.de> writes:
> >
> > > Yong Huang <yong.huang@smartx.com> writes:
> > >
> > >> On Wed, Oct 23, 2024 at 11:06 PM Fabiano Rosas <farosas@suse.de> wrote:
> > >>
> > >>> yong.huang@smartx.com writes:
> > >>>
> > >>> > From: Hyman Huang <yong.huang@smartx.com>
> > >>> >
> > >>> > v3:
> > >>> > 1. Remove the two redundant assignments in [PATCH v2 2/5] suggested
> > by
> > >>> Daniel
> > >>> >
> > >>> > Please review, thanks
> > >>> > Yong
> > >>> >
> > >>> > v2:
> > >>> > 1. Update the MAINTAINERS section suggested by Fabiano Rosas
> > >>> > 2. Ensure the dependencies when build the initrd-stress.img
> > suggested by
> > >>> Daniel
> > >>> > 3. Fix some bugs
> > >>> >
> > >>> > v1:
> > >>> > The previous patchset:
> > >>> >
> > >>>
> > https://lore.kernel.org/qemu-devel/cover.1722957352.git.yong.huang@smartx.com/
> > >>> > does not made the necessary changes and tests for the upstream
> > version.
> > >>> >
> > >>> > This patchset works for that:
> > >>> > 1. Move the guestperf to scripts directory suggested by Fabiano Rosas
> > >>> > 2. Make initrd-stress.img built by default suggested by Fabiano Rosas
> > >>> > 3. Make the necessary changes to adapt the latest multifd behavior
> > >>> > 4. A nitpick for multifd migration
> > >>> > 5. Support multifd compression option
> > >>> >
> > >>> > Hyman Huang (5):
> > >>> >   tests/migration: Move the guestperf tool to scripts directory
> > >>> >   tests/migration: Make initrd-stress.img built by default
> > >>> >   guestperf: Support deferred migration for multifd
> > >>> >   guestperf: Nitpick the inconsistent parameters
> > >>> >   guestperf: Introduce multifd compression option
> > >>> >
> > >>> >  MAINTAINERS                                   |  5 +++
> > >>> >  .../migration/guestperf-batch.py              |  0
> > >>> >  .../migration/guestperf-plot.py               |  0
> > >>> >  {tests => scripts}/migration/guestperf.py     |  0
> > >>> >  .../migration/guestperf/__init__.py           |  0
> > >>> >  .../migration/guestperf/comparison.py         | 15 ++++++++-
> > >>> >  .../migration/guestperf/engine.py             | 33
> > ++++++++++++++++---
> > >>> >  .../migration/guestperf/hardware.py           |  0
> > >>> >  .../migration/guestperf/plot.py               |  0
> > >>> >  .../migration/guestperf/progress.py           |  0
> > >>> >  .../migration/guestperf/report.py             |  0
> > >>> >  .../migration/guestperf/scenario.py           |  7 ++--
> > >>> >  .../migration/guestperf/shell.py              |  3 ++
> > >>> >  .../migration/guestperf/timings.py            |  0
> > >>> >  tests/migration/meson.build                   | 30 +++++++++--------
> > >>> >  15 files changed, 73 insertions(+), 20 deletions(-)
> > >>> >  rename {tests => scripts}/migration/guestperf-batch.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf-plot.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/__init__.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/comparison.py (89%)
> > >>> >  rename {tests => scripts}/migration/guestperf/engine.py (93%)
> > >>> >  rename {tests => scripts}/migration/guestperf/hardware.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/plot.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/progress.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/report.py (100%)
> > >>> >  rename {tests => scripts}/migration/guestperf/scenario.py (93%)
> > >>> >  rename {tests => scripts}/migration/guestperf/shell.py (98%)
> > >>> >  rename {tests => scripts}/migration/guestperf/timings.py (100%)
> > >>>
> > >>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > >>>
> > >>>
> > >>> Thinking out loud a little bit, it's still somewhat obscure from which
> > >>> directory this script should be called. None of these invocations work:
> > >>>
> > >>> $ ./scripts/migration/guestperf.py
> > >>>
> > >>> $ ./build/scripts/migration/guestperf.py
> > >>>
> > >>> $ cd scripts/migration
> > >>> $ ./guestperf.py
> > >>>
> > >>> $ cd build/scripts/migration
> > >>> $ ./guestperf.py
> > >>>
> > >>> Failed to open file “tests/migration/initrd-stress.img”: open() failed:
> > >>> No such file or directory
> > >>>
> > >>> This is the only one that works:
> > >>> $ cd build
> > >>> $ ./scripts/migration/guestperf.py
> > >>>
> > >>> Maybe we could improve that somehow in the future.
> > >>>
> > >>>
> > >> This patchset could be queued now,
> > >> Or after refining the documents mentioned above later?
> > >
> > > I think it's ok. Let me just rebase this on top of the other series that
> > > touches the tests/migration/ directory. I'll repost soon.
> >
> > Well, not so fast, the CI doesn't like the addition of stress.c:
> >
> > https://gitlab.com/farosas/qemu/-/jobs/8504697373
> > clang: error: argument unused during compilation: '-pie'
> 
> 
> > https://gitlab.com/farosas/qemu/-/jobs/8504697378
> > undefined reference to `_DYNAMIC'
> >
> 
> OK, I'll fix that once I have time.

Yong,

Note that the conflict is against this patch (now in migration-next, so
will be in 10.0's pull):

https://lore.kernel.org/r/20241127182901.529-4-farosas@suse.de

When you rebase, feel free to do that on top of migration-next if it didn't
yet land master branch, could be easier for all of us:

https://gitlab.com/peterx/qemu/-/tree/migration-next

For the stress.c warnings/error, I saw another one warning with gcc when
built it locally, then I found Fabiano's CI run also has it:

https://gitlab.com/farosas/qemu/-/jobs/8504697347

Which has:

[3381/3611] Linking target tests/migration/stress
/usr/bin/ld: /usr/lib64/libglib-2.0.a(gutils.c.o): in function `g_get_user_database_entry':
(.text+0xeb): warning: Using 'getpwnam_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/bin/ld: (.text+0x2be): warning: Using 'getpwuid' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/bin/ld: (.text+0x134): warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

Feel free to look at it too if you like. Nothing should be relevant to your
changes, so I think it could be there for a while when compilers upgrade.

Thanks,

-- 
Peter Xu