| Message ID | ee33ad20-ef6e-504d-6987-59ccb166f8e4@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: reduce cache flushing overhead | expand |
On 19/04/2023 11:44 am, Jan Beulich wrote: > --- a/xen/arch/x86/flushtlb.c > +++ b/xen/arch/x86/flushtlb.c > @@ -232,7 +232,7 @@ unsigned int flush_area_local(const void > if ( flags & FLUSH_HVM_ASID_CORE ) > hvm_flush_guest_tlbs(); > > - if ( flags & FLUSH_CACHE ) > + if ( flags & (FLUSH_CACHE | FLUSH_WRITEBACK) ) Given that we already have FLUSH_CACHE, then adding writeback also seems fine, but we need to get the naming corrected first. We've got a file called flushtlb.c which flushes more than the TLBs now, and various APIs in it. We have a bunch of ARM specific APIs which AFAICT exist purely to prop up the ARM-specific gnttab_cache_flush(). That needs to go and live behind an ifdef and stop polluting other architectures with an incredibly short sighted hypercall interface decision. The "area" in the low level calls isn't good. Range might be better, but I'm not sure. The "mask" part of the name would be better as "some" or perhaps "others", to be a better counterpoint to "local". Some of the wrappers too really ought to be dropped - there are lots of them, and too few users to justify. But on to the main thing which caught my eye... The FLUSH in FLUSH_CACHE means the flush infrastructure, not "cache flushing", and FLUSH_WRITEBACK is nonsensical next to this. All other things we flush have a qualification that makes them clear in context. (other than the assist one which I'm going to time out objections to and revert back to name which made more sense.) At an absolutely minimum, FLUSH_CACHE first needs renaming to FLUSH_CACHE_EVICT and then this new one you're adding needs to be FLUSH_CACHE_WRITEBACK. Except... Is there any credible reason to have EVICT as an option by the end of this cleanup? CLDEMOTE does exist for a reason (reducing coherency traffic overhead when you know the consumer is on a different CPU), but it would be totally bogus to use this in an all or mask form, and you wouldn't want to use it in local form either, simply from an overhead point of view. We have evict behaviour simply because `clflush` was the only game in town for decades, not because evicting the cacheline is what you want actually want to do. ~Andrew
On 19.04.2023 21:56, Andrew Cooper wrote: > On 19/04/2023 11:44 am, Jan Beulich wrote: >> --- a/xen/arch/x86/flushtlb.c >> +++ b/xen/arch/x86/flushtlb.c >> @@ -232,7 +232,7 @@ unsigned int flush_area_local(const void >> if ( flags & FLUSH_HVM_ASID_CORE ) >> hvm_flush_guest_tlbs(); >> >> - if ( flags & FLUSH_CACHE ) >> + if ( flags & (FLUSH_CACHE | FLUSH_WRITEBACK) ) > > Given that we already have FLUSH_CACHE, then adding writeback also seems > fine, but we need to get the naming corrected first. > > We've got a file called flushtlb.c which flushes more than the TLBs now, > and various APIs in it. > > We have a bunch of ARM specific APIs which AFAICT exist purely to prop > up the ARM-specific gnttab_cache_flush(). That needs to go and live > behind an ifdef and stop polluting other architectures with an > incredibly short sighted hypercall interface decision. > > The "area" in the low level calls isn't good. Range might be better, > but I'm not sure. The "mask" part of the name would be better as "some" > or perhaps "others", to be a better counterpoint to "local". Some of > the wrappers too really ought to be dropped - there are lots of them, > and too few users to justify. > > But on to the main thing which caught my eye... > > The FLUSH in FLUSH_CACHE means the flush infrastructure, not "cache > flushing", and FLUSH_WRITEBACK is nonsensical next to this. I agree; I chose the name simply to avoid further changes, while still fitting the naming scheme (i.e. the FLUSH_ prefix). I'm okay to change to ... > All other > things we flush have a qualification that makes them clear in context. > (other than the assist one which I'm going to time out objections to and > revert back to name which made more sense.) > > At an absolutely minimum, FLUSH_CACHE first needs renaming to > FLUSH_CACHE_EVICT and then this new one you're adding needs to be > FLUSH_CACHE_WRITEBACK. ... these, but I don't think they're much better (still primarily because of the FLUSH_ prefixes). FTAOD - while I'm going to make these adjustments (adding a single prereq patch to carry out the initial rename), I don't really see me doing any of there other adjustments you were outlining above (at least not within this series). > Except... > > Is there any credible reason to have EVICT as an option by the end of > this cleanup? Yes: map_pages_to_xen(), hvm_shadow_handle_cd(), memory_type_changed(), and hvm_set_mem_pinned_cacheattr(), all mean to evict caches aiui. clean_and_invalidate_dcache_va_range(), as you say, really shouldn't be required, but I'm unconvinced we can easily drop support for a gnttab sub-op that's been around for a number of years. (The more with there (still) not being any support for GNTTAB_CACHE_SOURCE_GREF, I'm (still) thinking this shouldn't have been a gnttab sub-op in the first place, but something truly arch-specific [a platform-op, or a memory one, or ...].) Jan > CLDEMOTE does exist for a reason (reducing coherency traffic overhead > when you know the consumer is on a different CPU), but it would be > totally bogus to use this in an all or mask form, and you wouldn't want > to use it in local form either, simply from an overhead point of view. > > We have evict behaviour simply because `clflush` was the only game in > town for decades, not because evicting the cacheline is what you want > actually want to do. > > ~Andrew
On 20.04.2023 10:50, Jan Beulich wrote: > On 19.04.2023 21:56, Andrew Cooper wrote: >> But on to the main thing which caught my eye... >> >> The FLUSH in FLUSH_CACHE means the flush infrastructure, not "cache >> flushing", and FLUSH_WRITEBACK is nonsensical next to this. > > I agree; I chose the name simply to avoid further changes, while still > fitting the naming scheme (i.e. the FLUSH_ prefix). I'm okay to change > to ... > >> All other >> things we flush have a qualification that makes them clear in context. >> (other than the assist one which I'm going to time out objections to and >> revert back to name which made more sense.) >> >> At an absolutely minimum, FLUSH_CACHE first needs renaming to >> FLUSH_CACHE_EVICT and then this new one you're adding needs to be >> FLUSH_CACHE_WRITEBACK. > > ... these, but I don't think they're much better (still primarily > because of the FLUSH_ prefixes). Actually - are you going to insist on "first"? There would be less code churn if first I introduced and used FLUSH_CACHE_WRITEBACK, and only then renamed the few remaining FLUSH_CACHE to FLUSH_CACHE_EVICT. Jan
--- a/xen/arch/x86/flushtlb.c +++ b/xen/arch/x86/flushtlb.c @@ -232,7 +232,7 @@ unsigned int flush_area_local(const void if ( flags & FLUSH_HVM_ASID_CORE ) hvm_flush_guest_tlbs(); - if ( flags & FLUSH_CACHE ) + if ( flags & (FLUSH_CACHE | FLUSH_WRITEBACK) ) { const struct cpuinfo_x86 *c = ¤t_cpu_data; unsigned long sz = 0; @@ -245,13 +245,16 @@ unsigned int flush_area_local(const void c->x86_clflush_size && c->x86_cache_size && sz && ((sz >> 10) < c->x86_cache_size) ) { - cache_flush(va, sz); - flags &= ~FLUSH_CACHE; + if ( flags & FLUSH_CACHE ) + cache_flush(va, sz); + else + cache_writeback(va, sz); + flags &= ~(FLUSH_CACHE | FLUSH_WRITEBACK); } - else - { + else if ( flags & FLUSH_CACHE ) wbinvd(); - } + else + wbnoinvd(); } if ( flags & FLUSH_ROOT_PGTBL ) --- a/xen/arch/x86/include/asm/flushtlb.h +++ b/xen/arch/x86/include/asm/flushtlb.h @@ -135,6 +135,8 @@ void switch_cr3_cr4(unsigned long cr3, u #else # define FLUSH_NO_ASSIST 0 #endif + /* Write back data cache contents */ +#define FLUSH_WRITEBACK 0x10000 /* Flush local TLBs/caches. */ unsigned int flush_area_local(const void *va, unsigned int flags); @@ -194,7 +196,11 @@ static inline int clean_and_invalidate_d } static inline int clean_dcache_va_range(const void *p, unsigned long size) { - return clean_and_invalidate_dcache_va_range(p, size); + unsigned int order = get_order_from_bytes(size); + + /* sub-page granularity support needs to be added if necessary */ + flush_area_local(p, FLUSH_WRITEBACK | FLUSH_ORDER(order)); + return 0; } unsigned int guest_flush_tlb_flags(const struct domain *d);
The majority of the present callers really aren't after invalidating cache contents, but only after writeback. Make this available by simply extending the FLUSH_CACHE handling accordingly. No feature checks are required here: cache_writeback() falls back to cache_flush() as necessary, while WBNOINVD degenerates to WBINVD on older hardware. Signed-off-by: Jan Beulich <jbeulich@suse.com>