Re: [PATCH 03/12] libxenguest: short-circuit "all-dirty" handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/06/2021 14:18, Jan Beulich wrote:
> For one it is unnecessary to fill a perhaps large chunk of memory with
> all ones. Add a new parameter to send_dirty_pages() for callers to
> indicate so.
>
> Then it is further unnecessary to allocate the dirty bitmap altogether
> when all that's ever going to happen is a single all-dirty run.

The allocation is deliberate, and does want to stay where it is IMO.

Single all-dirty runs are a debugging technique only.  All production
cases are live, and you don't want to fail midway through because a
late, large, memory allocation failed.


As for the send_{dirty,all}_pages() split, that was deliberate to keep
the logic simple.  The logdirty bitmap is tiny (in comparison to other
structures) outside of artificial cases like this.

What you've done with this change is rendered send_all_pages()
redundant, but not actually taken it out of the code, thereby
complicating it.  At the moment, this doesn't look to be an improvement.

> @@ -807,8 +798,11 @@ static int setup(struct xc_sr_context *c
>      if ( rc )
>          goto err;
>  
> -    dirty_bitmap = xc_hypercall_buffer_alloc_pages(
> -        xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
> +    dirty_bitmap = ctx->save.live || ctx->stream_type != XC_STREAM_PLAIN
> +        ? xc_hypercall_buffer_alloc_pages(
> +              xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)))
> +        : (void *)-1L;

This is a pointer loaded with a timebomb, which doesn't trigger NULL
pointer checks, and for which {set,clear}_bit(dirty_bitmap, large_pfn)
won't fault and will instead corrupt random areas of the address space.

~Andrew