On Wed, Dec 09, 2020 at 10:33:41AM +0800, Zenghui Yu wrote: > Hi Peter, > > Thanks for having a look at it. > > On 2020/12/8 23:16, Peter Xu wrote: > > Hi, Zenghui, > > > > On Tue, Dec 08, 2020 at 07:40:13PM +0800, Zenghui Yu wrote: > > > The kernel KVM_CLEAR_DIRTY_LOG interface has align requirement on both the > > > start and the size of the given range of pages. We have been careful to > > > handle the unaligned cases when performing CLEAR on one slot. But it seems > > > that we forget to take the unaligned *size* case into account when > > > preparing bitmap for the interface, and we may end up clearing dirty > > > status > > > for pages outside of [start, start + size). > > > > Thanks for the patch, though my understanding is that this is not a bug. > > > > Please have a look at kvm_memslot_init_dirty_bitmap() where we'll allocate > > the > > dirty bitmap to be aligned to 8 bytes (assuming that's the possible max of > > the > > value sizeof(unsigned long)). That exactly covers 64 pages. > > > > So here as long as start_delta==0 (so the value of "bmap_npages - size / > > psize" > > won't really matter a lot, imho), then we'll definitely have > > KVMSlot.dirty_bmap > > long enough to cover the range we'd like to clear. > > I agree. But actually I'm not saying that KVMSlot.dirty_bmap is not > long enough. What I was having in mind is something like: > > // psize = qemu_real_host_page_size; > // slot.start_addr = 0; > // slot.memory_size = 64 * psize; > > kvm_log_clear_one_slot(slot, as, 0 * psize, 32 * psize); --> [1] > kvm_log_clear_one_slot(slot, as, 32 * psize, 32 * psize); --> [2] > > So the @size is not aligned with 64 pages. Before this patch, we'll > clear dirty status for all pages(0-63) through [1]. It looks to me that > this violates the caller's expectation since we only want to clear > pages(0-31).
Now I see; I think you're right. :) > > As I said, I don't think this will happen in practice -- the migration > code should always provide us with a 64-page aligned section (right?). Yes, migration is the major consumer, and that should be guaranteed indeed, see CLEAR_BITMAP_SHIFT_MIN. Not sure about VGA - that should try to do log clear even without migration, but I guess that satisfies the 64-page alignment too, since it's not a huge number (256KB). The VGA buffer size could be related to screen resolution, then N*1024*768 could still guarantee a safe use of the fast path. > I'm just thinking about the correctness of the specific algorithm used > by kvm_log_clear_one_slot(). Yeah, then I think it's okay to have this, just in case someday we'll hit it. Acked-by: Peter Xu <pet...@redhat.com> (It would be nicer if above example could be squashed into commit message) Thanks, -- Peter Xu
