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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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.
> > 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