[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 28 Jun 2021 10:26:28 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AxLzyCEHDHA3/Ym7Xddqw9HTbN/RxvULE1e9RfZ/EXI=; b=MOu4+7kYd6wd6SuSinaK+J9hBq7mnmWFL6e9ZjPROsWBcDymAk3FYBgMBkXl8k2cQDpNyp8o3RE0uzYJAGpaOtezuKJSJ1vd97hPCO/GhtubWEup777Vb2OJKzOWKCOWaxH9yxLYwD73Uypg9jOeyqSSEpIxpLBRWPuTsM+uoPvbeVPTJzQgyHc/OQstQyCvql2iTT0zW/PPoIG7QZUnYm2zsdZAUVysJtfGNjiXDIHXrQiKK7LGgdaoOMb1wvC5EjOdskesFOEeBw8FwJ9UjpVScQesHLZepcvNNieK3pO7xBQ/AmgzVIBGvnIw6PK1O7Q1KcMV6OYgSEeF+4LB3Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eVn2xdUXLC+ZBtz7WknD49TPvXrnJMd2YbE7WrqdYtmJDFLwk9QVoHJlCoAJWMtGS3DEcNixSQUtik904Fp9DRHBgGQWXurJjnFFwoI+yzCUXaf31WopMFPRUC8Mt9yN0oWqyW2deUvQqS4mW1rLapVb2NsUDIqEuBk+hR7ZB2fUNYXY0xy3jgkkVr114wLYDt23NDKZTy9BDCRu+HNqyjdGZWp+y0vj4gDEJC9/I2ElAdMJwYrb5OQsctPy68z9FflFaJG65pJ3p6i3lQ14gU22XFifibAWTJTYk59MVq4JTYFOopviUlTlyA/gZ4ukNMR2GFKDxjHN9xkxU66+KA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 28 Jun 2021 08:26:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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

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




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.