Re: [PATCH 1/2] x86/shadow: defer/avoid paging_mfn_is_dirty() invocation

[Andrew Cooper <amc96@xxxxxxxx>]

On 01/12/2021 10:35, Jan Beulich wrote:
> paging_mfn_is_dirty() is moderately expensive, so avoid its use unless
> its result might actually change anything. This means moving the
> surrounding if() down below all other checks that can result in clearing
> _PAGE_RW from sflags, in order to then check whether _PAGE_RW is
> actually still set there before calling the function.
>
> While moving the block of code, fold two if()s and make a few style
> adjustments.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> TBD: Perhaps the dirty-VRAM piece also wants moving down a little, such
>      that all three "level == 1" conditionals can be folded?

TBH, lots of the layout looks dubious, but I'm not sure how worthwhile
micro-optimising it is.  This particular rearrangement is surely
unmeasurable.

Fixing the (mis)use of mfn_valid() in the logdirty infrastructure will
gain a far larger improvement, because that's dropping a fair number of
lfence's from multiple paths, but it's still the case that anything here
is rare-to-non-existent in production workloads, and vastly dominated by
the VMExit cost even when migrating shadow VMs.

>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -604,23 +604,6 @@ _sh_propagate(struct vcpu *v,
>                    && !(gflags & _PAGE_DIRTY)) )
>          sflags &= ~_PAGE_RW;
>  
> -    // shadow_mode_log_dirty support
> -    //
> -    // Only allow the guest write access to a page a) on a demand fault,
> -    // or b) if the page is already marked as dirty.
> -    //
> -    // (We handle log-dirty entirely inside the shadow code, without using 
> the
> -    // p2m_ram_logdirty p2m type: only HAP uses that.)
> -    if ( unlikely((level == 1) && shadow_mode_log_dirty(d)) )
> -    {
> -        if ( mfn_valid(target_mfn) ) {
> -            if ( ft & FETCH_TYPE_WRITE )
> -                paging_mark_dirty(d, target_mfn);
> -            else if ( !paging_mfn_is_dirty(d, target_mfn) )
> -                sflags &= ~_PAGE_RW;
> -        }
> -    }
> -
>  #ifdef CONFIG_HVM
>      if ( unlikely(level == 1) && is_hvm_domain(d) )
>      {
> @@ -661,6 +644,25 @@ _sh_propagate(struct vcpu *v,
>                    ) )
>          sflags &= ~_PAGE_RW;
>  
> +    /*
> +     * shadow_mode_log_dirty support
> +     *
> +     * Only allow the guest write access to a page a) on a demand fault,
> +     * or b) if the page is already marked as dirty.
> +     *
> +     * (We handle log-dirty entirely inside the shadow code, without using 
> the
> +     * p2m_ram_logdirty p2m type: only HAP uses that.)
> +     */
> +    if ( level == 1 && unlikely(shadow_mode_log_dirty(d)) &&
> +         mfn_valid(target_mfn) )
> +    {
> +        if ( ft & FETCH_TYPE_WRITE )
> +            paging_mark_dirty(d, target_mfn);
> +        else if ( (sflags & _PAGE_RW) &&
> +                  !paging_mfn_is_dirty(d, target_mfn) )
> +            sflags &= ~_PAGE_RW;

This is almost crying out for a logdirty_test_and_maybe_set() helper,
because the decent of the logdirty trie is common between the two, but
as this would be the only user, probably not worth it.

However, the more I look at the logdirty logic, the more it is clear
that all the mfn_valid() tests should change to MFN_INVALID, and the
result will be far more efficient.

~Andrew

> +    }
> +
>      // PV guests in 64-bit mode use two different page tables for user vs
>      // supervisor permissions, making the guest's _PAGE_USER bit irrelevant.
>      // It is always shadowed as present...
>
>