[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h
On 13/03/2025 7:43 pm, Nicola Vetrini wrote: > On 2025-03-13 14:35, Andrew Cooper wrote: >> On 13/03/2025 12:59 pm, Jan Beulich wrote: >>> On 12.03.2025 18:45, Andrew Cooper wrote: >>>> xen/mm.h includes asm/tlbflush.h almost at the end, which creates a >>>> horrible >>>> tangle. This is in order to provide two common files with an >>>> abstraction over >>>> the x86-specific TLB clock logic. >>>> >>>> First, introduce CONFIG_HAS_TLB_CLOCK, selected by x86 only. Next, >>>> introduce >>>> xen/tlb-clock.h, providing empty stubs, and include this into >>>> memory.c and >>>> page_alloc.c >>>> >>>> No functional change. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >>>> --- >>>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx> >>>> CC: Michal Orzel <michal.orzel@xxxxxxx> >>>> CC: Jan Beulich <jbeulich@xxxxxxxx> >>>> CC: Julien Grall <julien@xxxxxxx> >>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> >>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx> >>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> >>>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx> >>>> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> >>>> CC: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx> >>>> >>>> There is still a mess here with the common vs x86 split, but it's >>>> better >>>> contained than before. >>>> --- >>>> xen/arch/x86/Kconfig | 1 + >>>> xen/common/Kconfig | 3 +++ >>>> xen/common/memory.c | 1 + >>>> xen/common/page_alloc.c | 1 + >>>> xen/include/xen/mm.h | 27 -------------------- >>>> xen/include/xen/tlb-clock.h | 49 >>>> +++++++++++++++++++++++++++++++++++++ >>>> 6 files changed, 55 insertions(+), 27 deletions(-) >>>> create mode 100644 xen/include/xen/tlb-clock.h >>>> > > >>> However, see below. >>> >>>> + arch_flush_tlb_mask(&mask); >>>> + } >>>> +} >>>> + >>>> +#else /* !CONFIG_HAS_TLB_CLOCK */ >>>> + >>>> +struct page_info; >>>> +static inline void accumulate_tlbflush( >>>> + bool *need_tlbflush, const struct page_info *page, >>>> + uint32_t *tlbflush_timestamp) {} >>>> +static inline void filtered_flush_tlb_mask(uint32_t >>>> tlbflush_timestamp) {} >>> Is doing nothing here correct? >> >> Yeah, it's not, but this only occurred to me after sending the series. >> >> Interestingly, CI is green across the board for ARM, which suggests to >> me that this logic isn't getting a workout. >> >>> mark_page_free() can set a page's >>> ->u.free.need_tlbflush. And with that flag set the full >>> >>> static inline void accumulate_tlbflush( >>> bool *need_tlbflush, const struct page_info *page, >>> uint32_t *tlbflush_timestamp) >>> { >>> if ( page->u.free.need_tlbflush && >>> page->tlbflush_timestamp <= tlbflush_current_time() && >>> (!*need_tlbflush || >>> page->tlbflush_timestamp > *tlbflush_timestamp) ) >>> { >>> *need_tlbflush = true; >>> *tlbflush_timestamp = page->tlbflush_timestamp; >>> } >>> } >>> >>> reduces to (considering that tlbflush_current_time() resolves to >>> constant 0, >>> which also implies every page's ->tlbflush_timestamp is only ever 0) >>> >>> static inline void accumulate_tlbflush( >>> bool *need_tlbflush, const struct page_info *page, >>> uint32_t *tlbflush_timestamp) >>> { >>> if ( !*need_tlbflush ) >>> *need_tlbflush = true; >>> } >>> >>> which means a not-stubbed-out filtered_flush_tlb_mask(), with >>> tlbflush_filter() >>> doing nothing, would actually invoke arch_flush_tlb_mask() (with all >>> online CPUs >>> set in the mask) when called. And arch_flush_tlb_mask() isn't a >>> no-op on Arm. >> >> Yes. Sadly, fixing this (without Eclair complaining in the middle of >> the series) isn't as easy as I'd hoped. >> > > Hi Andrew, > > I didn't quite follow the whole thread (been busy the last couple of > days), but could you explain briefly what's the issue here? Just a > link to a failing pipeline should be fine as well. There isn't one. But to untangle this the easy way, I'd need to have a duplicate declaration for arch_flush_tlb_mask() for a patch of two. Which I know Eclair would complain about, and therefore I need to find a different way to untangle it. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |