On Mon, Feb 6, 2023 at 3:41 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Fri, Feb 03, 2023 at 11:28:19AM -0800, Vipin Sharma wrote:
> > No need to check all of the conditions in __handle_changed_spte() as
> > clearing dirty log only involves resetting dirty or writable bit.
>
> Channelling Sean: State what the patch does first.
>
> >
> > Make atomic change to dirty or writable bit and mark pfn dirty.
>
> This is way too vague. And the old code already did both of these
> things. What's changed is that the bits are being cleared with an atomic
> AND and taking advantage of the fact that that avoids needing to deal
> with changes to volatile bits.
>
> Please also explain what effect this has on @record_dirty_log and why it
> can be opportunistically cleaned up in this commit.
>
Okay, I will try to be better in the next one.
> > Iteration 3 clear dirty log time: 1.881043086s
> > Disabling dirty logging time: 2.930387523s
> > Get dirty log over 3 iterations took 0.006191681s.
> > (Avg 0.002063893s/iteration)
> > Clear dirty log over 3 iterations took 6.148156531s. (Avg 2.049385510s/iteration)
>
> Can you trim these results to just show the clear times? (assuming none
> of the rest are relevant)
I was not sure if just showing clear dirty times will be acceptable or
not. I will update the message to only show clear dirty log time and
average.
>
> >
> > +static inline u64 kvm_tdp_mmu_clear_spte_bit(struct tdp_iter *iter, u64 mask)
> > +{
>
> Make "bit" plural as long as the parameter is a raw mask.
>
> Also drop "kvm_" since this is not intended to be called outside the TDP
> MMU. (It'd be nice to make the same cleanup to the read/write
> functions if you feel like it.)
>
Sounds good.
> > @@ -1678,7 +1665,7 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
> > gfn_t gfn, unsigned long mask, bool wrprot)
> > {
> > struct tdp_iter iter;
> > - u64 new_spte;
> > + u64 clear_bits;
>
> nit: clear_bit since it's always a single bit?
Yes.
>
> >
> > rcu_read_lock();
> >
> > @@ -1694,18 +1681,22 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
> > mask &= ~(1UL << (iter.gfn - gfn));
> >
> > if (wrprot || spte_ad_need_write_protect(iter.old_spte)) {
> > - if (is_writable_pte(iter.old_spte))
> > - new_spte = iter.old_spte & ~PT_WRITABLE_MASK;
> > - else
> > + if (!is_writable_pte(iter.old_spte))
> > continue;
> > +
> > + clear_bits = PT_WRITABLE_MASK;
> > } else {
> > - if (iter.old_spte & shadow_dirty_mask)
> > - new_spte = iter.old_spte & ~shadow_dirty_mask;
> > - else
> > + if (!(iter.old_spte & shadow_dirty_mask))
> > continue;
>
> You can factor out the continue check now that you have clear_bits. e.g.
>
> if (wrprot || spte_ad_need_write_protect(iter.old_spte))
> clear_bits = PT_WRITABLE_MASK;
> else
> clear_bits = shadow_dirty_mask;
>
> if (!(iter->old_spte & clear_bits))
> continue;
>
> iter.old_spte = kvm_tdp_mmu_clear_spte_bit(&iter, clear_bits);
>
Yeah, this is better. Even better if I just initialize like:
u64 clear_bits = shadow_dirty_mask;
This will also get rid of the else part.
