Re: [PATCH v6 02/13] multifd: Create page_size fields into both MultiFD{Recv,Send}Params

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> We were calling qemu_target_page_size() left and right.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

[Adding Richard]

> (Copying in Peter Maydell)
> Your problem here is most of these files are target independent
> so you end up calling the qemu_target_page_size functions, which I guess
> you're seeing popup in some perf trace?
> I mean they're trivial functions but I guess you do get the function
> call.

Hi

There are several problems here:

- Richard complained in previous reviews that we were calling
  qemu_target_page_size() inside loops or more than once per function
  (He was right)

- qemu_target_page_size() name is so long that basically means that I
  had to split the line for each appearance.

- All migration code assumes that the value is constant for a current
  migration, it can change.

So I decided to cache the value in the structure and call it a day.  The
same for the other page_count field.

I have never seen that function on a performance profile, so this is
just a taste/aesthetic issue.

I think your patch is still good, but it don't cover any of the issues
that I just listed.

Thanks, Juan.


>
> I wonder about the following patch instead
> (Note i've removed the const on the structure here); I wonder how this
> does performance wise for everyone:
>
>
> From abc7da46736b18b6138868ccc0b11901169e1dfd Mon Sep 17 00:00:00 2001
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Date: Mon, 16 May 2022 19:54:31 +0100
> Subject: [PATCH] target-page: Maintain target_page variable even for
>  non-variable
> Content-type: text/plain
>
> On architectures that define TARGET_PAGE_BITS_VARY, the 'target_page'
> structure gets filled in at run time by the number of bits and the
> TARGET_PAGE_BITS and TARGET_PAGE macros use that rather than being
> constant.
>
> On non-variable pagesize systems target_page is not filled in, and we
> rely on TARGET_PAGE_SIZE being compile time defined.
>
> The problem is that for source files that are target-independent
> they end up calling qemu_target_page_size to read the size, and that
> function call is annoying.
>
> Improve this by always filling in 'target_page' even for non-variable
> size CPUs, and inlining the functions that previously returned
> the macro values (that may have been constant) to return the
> values read from target_page.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>