Re: [Qemu-devel] [PATCH 2/2] find_ram_offset: Align ram_addr_t allocation on long boundaries

[Paolo Bonzini]

On 05/01/2018 18:01, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> The dirty bitmaps are built from 'long'sand there is fast-path code
> for synchronising the case where the RAMBlock is aligned to the start
> of a long boundary.  Align the allocation to this boundary
> to cause the fast path to be used.
> 
> Offsets before change:
> address@hidden:find_ram_offset size: 0x1e0000 @ 0x8000000
> address@hidden:find_ram_offset size: 0x20000 @ 0x81e0000
> address@hidden:find_ram_offset size: 0x20000 @ 0x8200000
> address@hidden:find_ram_offset size: 0x1000000 @ 0x8220000
> address@hidden:find_ram_offset size: 0x10000 @ 0x9220000
> address@hidden:find_ram_offset size: 0x40000 @ 0x9230000
> address@hidden:find_ram_offset size: 0x200000 @ 0x9270000
> address@hidden:find_ram_offset size: 0x1000 @ 0x9470000
> address@hidden:find_ram_offset size: 0x1000 @ 0x9471000
> 
> after change:
> address@hidden:find_ram_offset size: 0x1e0000 @ 0x8000000
> address@hidden:find_ram_offset size: 0x20000 @ 0x8200000
> address@hidden:find_ram_offset size: 0x20000 @ 0x8240000
> address@hidden:find_ram_offset size: 0x1000000 @ 0x8280000
> address@hidden:find_ram_offset size: 0x10000 @ 0x9280000
> address@hidden:find_ram_offset size: 0x40000 @ 0x92c0000
> address@hidden:find_ram_offset size: 0x200000 @ 0x9300000
> address@hidden:find_ram_offset size: 0x1000 @ 0x9500000
> address@hidden:find_ram_offset size: 0x1000 @ 0x9540000
> 
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  exec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index 7966570231..644603f05e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1694,6 +1694,11 @@ static ram_addr_t find_ram_offset(ram_addr_t size)
>              }
>          }
>  
> +        /* Align blocks to start on a 'long' in the bitmap
> +         * which makes the bitmap sync'ing take the fast path.
> +         */
> +        end = ROUND_UP(end, BITS_PER_LONG << TARGET_PAGE_BITS);

I think this should be done before the nested loop.  Otherwise, "next -
end" can become negative, which is always going to be >= size.  It's
also very large, so it's likely to fail the mingap test, but it's buggy
nevertheless.

In fact "end" could be renamed to "candidate" which explains more
clearly what's going on.  I'll post a v2 shortly...

Thanks,

Paolo


>          /* If it fits remember our place and remember the size
>           * of gap, but keep going so that we might find a smaller
>           * gap to fill so avoiding fragmentation.
>