On 19.08.21 13:51, Jan Beulich wrote:
> On 19.08.2021 13:25, Juergen Gross wrote:
>> On 19.08.21 13:06, Jan Beulich wrote:
>>> 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
>>
>> I was thinking about the domctl.
>>
>> Apart of struct xen_sysctl_page_offline_op this seems to be the only
>> left domctl/sysctl structure limiting guest or host size to values
>> being relevant. Changing those would be a sensible thing to do IMO.
> 
> Yet in the context of v1 of this series, which included "x86/paging:
> deal with log-dirty stats overflow" (now commit 17e91570c5a4) we
> settled on these fields not needing widening. This doesn't prevent
> us doing what you suggest, but it would look pretty odd to me at
> least.

Sorry I was too busy at that time to have a detailed look at the
patches.

TBH I'd rather undo the stats overflow handling and widen the fields.
This is a rather simple patch and a much cleaner solution in the end.

I'm not insisting on that, but in case I had to decide this would be my
preferred way to handle it.


Juergen