On Mon, Jan 13, 2025 at 08:48:28AM -0800, Sean Christopherson wrote: > On Mon, Jan 13, 2025, Yan Zhao wrote: > > On Fri, Jan 10, 2025 at 05:04:08PM -0800, Sean Christopherson wrote: > > > @@ -163,14 +157,31 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring, > > > continue; > > > } > > > } > > > - kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > + > > > + /* > > > + * Reset the slot for all the harvested entries that have been > > > + * gathered, but not yet fully processed. > > > + */ > > I really like the logs as it took me quite a while figuring out how this part of > > the code works :) > > > > Does "processed" mean the entries have been reset, and "gathered" means they've > > been read from the ring? > > Yeah. > > > I'm not sure, but do you like this version? e.g. > > "Combined reset of the harvested entries that can be identified by curr_slot > > plus cur_offset+mask" ? > > I have no objection to documenting the mechanics *and* the high level intent, > but I definitely want to document the "what", not just the "how". > > > > + if (mask) > > > + kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > + > > > + /* > > > + * The current slot was reset or this is the first harvested > > > + * entry, (re)initialize the metadata. > > > + */ > > What about > > "Save the current slot and cur_offset (with mask initialized to 1) to check if > > any future entries can be found for a combined reset." ? > > Hmm, what if I add a comment at the top to document the overall behavior and the > variables, > > /* > * To minimize mmu_lock contention, batch resets for harvested entries > * whose gfns are in the same slot, and are within N frame numbers of > * each other, where N is the number of bits in an unsigned long. For > * simplicity, process the current set of entries when the next entry > * can't be included in the batch. > * > * Track the current batch slot, the gfn offset into the slot for the > * batch, and the bitmask of gfns that need to be reset (relative to > * offset). Note, the offset may be adjusted backwards, e.g. so that > * a sequence of gfns X, X-1, ... X-N can be batched. > */ Nice log. > u32 cur_slot, next_slot; > u64 cur_offset, next_offset; > unsigned long mask = 0; > struct kvm_dirty_gfn *entry; > > and then keep this as: > > /* > * The current slot was reset or this is the first harvested > * entry, (re)initialize the batching metadata. > */ > > > > > > cur_slot = next_slot; > > > cur_offset = next_offset; > > > mask = 1; > > > first_round = false; > > > } > > > > > > - kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > + /* > > > + * Perform a final reset if there are harvested entries that haven't > > > + * been processed. The loop only performs a reset when an entry can't > > > + * be coalesced, i.e. always leaves at least one entry pending. > > The loop only performs a reset when an entry can be coalesced? > > No, if an entry can be coalesced then the loop doesn't perform a reset. Does > this read better? > > /* > * Perform a final reset if there are harvested entries that haven't > * been processed, which is guaranteed if at least one harvested was > * found. The loop only performs a reset when the "next" entry can't > * be batched with "current" the entry(s), and that reset processes the > * _current_ entry(s), i.e. the last harvested entry, a.k.a. next, will > * will always be left pending. > */ LGTM! > > > + */ > > > + if (mask) > > > + kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > > > > /* > > > * The request KVM_REQ_DIRTY_RING_SOFT_FULL will be cleared > > > -- > > > 2.47.1.613.gc27f4b7a9f-goog > > >
