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 eitherI 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
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
