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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.06.2021 19:02, Andrew Cooper wrote:
> 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.

I'm afraid I don't understand: I don't move _when_ the allocation
occurs; I only suppress the allocation (altogether) when the allocated
memory remains unused.

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

I view the remaining send_all_pages() as similarly useful (or not) as
e.g. send_domain_memory_checkpointed() (being merely a wrapper around
suspend_and_send_dirty()).

>> @@ -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.

Yeah, this isn't very nice, and gets done away with again in a later
patch. I'd prefer to keep it like it is (assuming the later change
will also go in), but if really deemed necessary I can move that code
re-arrangement here, such that the use of (void *)-1L wouldn't be
necessary anymore. (You may have noticed that all I did this for is
to be able to pass the !dirty_bitmap later in the function, and that
I deliberately only update the local variable, not the hbuf, making
pretty certain that this pointer isn't going to be de-referenced.)

Jan