Re: [PATCH v2 03/13] libxenguest: deal with log-dirty op stats overflow

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 03/13] libxenguest: deal with log-dirty op stats overflow
Date: Thu, 19 Aug 2021 13:06:54 +0200	[thread overview]
Message-ID: <f0e9c8ad-dfa2-77ac-b3db-02cba59e843c@suse.com> (raw)
In-Reply-To: <f69e5a2b-cd8c-3534-ab7b-427fb1fc1cdc@suse.com>

On 19.08.2021 12:20, Juergen Gross wrote:
> On 05.07.21 17:13, Jan Beulich wrote:
>> In send_memory_live() the precise value the dirty_count struct field
>> gets initialized to doesn't matter much (apart from the triggering of
>> the log message in send_dirty_pages(), see below), but it is important
>> that it not be zero on the first iteration (or else send_dirty_pages()
>> won't get called at all). Saturate the initializer value at the maximum
>> value the field can hold.
>>
>> While there also initialize struct precopy_stats' respective field to a
>> more sane value: We don't really know how many dirty pages there are at
>> that point.
>>
>> In suspend_and_send_dirty() and verify_frames() the local variables
>> don't need initializing at all, as they're only an output from the
>> hypercall which gets invoked first thing.
>>
>> In send_checkpoint_dirty_pfn_list() the local variable can be dropped
>> altogether: It's optional to xc_logdirty_control() and not used anywhere
>> else.
>>
>> Note that in case the clipping actually takes effect, the "Bitmap
>> contained more entries than expected..." log message will trigger. This
>> being just an informational message, I don't think this is overly
>> concerning.
> 
> Is there any real reason why the width of the stats fields can't be
> expanded to avoid clipping? This could avoid the need to set the
> initial value to -1, which seems one of the more controversial changes.

While not impossible, it comes with a price tag, as we'd either need
to decouple xc_shadow_op_stats_t from struct xen_domctl_shadow_op_stats
or alter the underlying domctl. Neither of which looked either
appealing or necessary to me; instead I'm still struggling with
Andrew's comments, yet I didn't receive any clarification of further
explanation. Plus I continue to think that statistics output like this
shouldn't be assumed to be precise anyway, and for practical purposes
I don't think it really matters how large the counts actually are once
they've moved into the billions.

Jan