Re: [PATCH 4/8] xen/common: Split tlb-clock.h out of mm.h

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@citrix.com>
>>> ---
>>> CC: Anthony PERARD <anthony.perard@vates.tech>
>>> CC: Michal Orzel <michal.orzel@amd.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Julien Grall <julien@xen.org>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>>> 
>>> 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.

>> I therefore think that while moving stuff into a separate header makes 
>> sense,
>> HAS_TLB_CLOCK isn't overly useful to introduce.
> 
> It takes a cpumask_t off the stack, because we can pass cpu_online_mask
> straight into arch_flush_tlb_mask(), and it removes a bitmap_copy that
> the compiler can't optimise out.
> 
> ~Andrew

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253