On 19.08.2021 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.
And - just to make it very explicit - even if I went this route to address a controversial point, I'd still like to understand the reason for the original objection - if only for my own education. Jan
