| Message ID | 3bb2018b-8e28-6469-6b6c-c6de935bf669@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: address observation made while working on XSA-387 | expand |
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@suse.com> > --- > 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... > >
On 01.12.2021 19:33, Andrew Cooper wrote: > 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@suse.com> >> --- >> 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. The point of the rearrangement suggestion was source tidying far more than micro-optimizing. > 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. Quite possible, but entirely orthogonal to this change. >> @@ -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. I'm struggling to see how this would improve the code construct without altering the behavior. But then you say anyway that introducing one probably isn't worth it as long as there's just one use site (and there aren't ever two decents of the trie, so there's not really anything to be saved performance wise). > 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. As said - an orthogonal change; nothing to be folded into here imo. Jan
At 11:35 +0100 on 01 Dec (1638358515), 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@suse.com> Reviewed-by: Tim Deegan <tim@xen.org> > --- > TBD: Perhaps the dirty-VRAM piece also wants moving down a little, such > that all three "level == 1" conditionals can be folded? I have no strong feelings on that either way. Cheers, Tim.
--- 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; + } + // 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...
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@suse.com> --- TBD: Perhaps the dirty-VRAM piece also wants moving down a little, such that all three "level == 1" conditionals can be folded?