Re: [PATCH v13 07/14] x86/mm: global ASID allocation helper functions

[Rik van Riel <riel@surriel.com>]

On Mon, 2025-02-24 at 15:16 +0100, Borislav Petkov wrote:
> On Sun, Feb 23, 2025 at 02:48:57PM -0500, Rik van Riel wrote:
> > 
> >  arch/x86/include/asm/mmu.h      |  11 +++
> >  arch/x86/include/asm/tlbflush.h |  43 ++++++++++
> >  arch/x86/mm/tlb.c               | 144
> > +++++++++++++++++++++++++++++++-
> >  3 files changed, 195 insertions(+), 3 deletions(-)
> 
> arch/x86/mm/tlb.c:378:6: warning: no previous prototype for
> ‘destroy_context_free_global_asid’ [-Wmissing-prototypes]
>   378 | void destroy_context_free_global_asid(struct mm_struct *mm)
>       |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/tlb.c:355:13: warning: ‘use_global_asid’ defined but not
> used [-Wunused-function]
>   355 | static void use_global_asid(struct mm_struct *mm)
>       |             ^~~~~~~~~~~~~~~
> arch/x86/mm/tlb.c:327:13: warning: ‘mm_active_cpus_exceeds’ defined
> but not used [-Wunused-function]
>   327 | static bool mm_active_cpus_exceeds(struct mm_struct *mm, int
> threshold)
>       |             ^~~~~~~~~~~~~~~~~~~~~~
> 
> If those functions are going to remain global they better get a
> proper prefix
> like "tlb_".
> 
I've renamed the global function to 
tlb_destroy_context_free_global_asid

> And add the functions with their first use - no need to pre-add them.

That's what I originally had. Dave requested I split
out the patch into multiple ones.

That means either introducing helper functions in a
separate patch, or coming up with one version of the
code in one patch, and then replacing that code in
the next, resulting in a bunch of extra code to review.

https://lore.kernel.org/linux-kernel/b067a9fc-ff5f-4baa-a1ff-3fa749ae4d44@intel.com/
> 
> > 
> > +static inline void assign_mm_global_asid(struct mm_struct *mm, u16
> > asid)
> 
> mm_assign_global_asid()

Done.

> > +/*
> > + * Global ASIDs are allocated for multi-threaded processes that
> > are
> > + * active on multiple CPUs simultaneously, giving each of those
> > + * processes the same PCIDs on every CPU, for use with hardware-
> > assisted
> 
> "the same PCID" or "the same PCIDs"?
> 
> Does a multithreaded process get one or more than one PCIDs? I'd
> expect only
> one ofc.

It's only one. Fixed the commment.

> 
> > +	asid = find_next_zero_bit(global_asid_used,
> > MAX_ASID_AVAILABLE, last_global_asid);
> > +
> > +	if (asid >= MAX_ASID_AVAILABLE) {
> 
> 	if (asid >= MAX_ASID_AVAILABLE && !global_asid_available)

Done.

> > +/*
> > + * Check whether a process is currently active on more than
> > "threshold" CPUs.
> 
> @threshold - then it is clear that you mean the function argument.
> 
Done. Thank you.

> > + * This is a cheap estimation on whether or not it may make sense
> > to assign
> > + * a global ASID to this process, and use broadcast TLB
> > invalidation.
> > + */
> > +static bool mm_active_cpus_exceeds(struct mm_struct *mm, int
> > threshold)
> > +{
> > +	int count = 0;
> > +	int cpu;
> 
> Is this function supposed to hold some sort of a lock?

I don't think we care too much about total accuracy here.

Not holding up other CPUs is probably more important.

> 
> > +
> > +	/* This quick check should eliminate most single threaded
> > programs. */
> > +	if (cpumask_weight(mm_cpumask(mm)) <= threshold)
> > +		return false;
> > +
> > +	/* Slower check to make sure. */
> > +	for_each_cpu(cpu, mm_cpumask(mm)) {
> 
> Needs CPU hotplug protection?

I don't see CPU hotplug protection in most other loops
that iterate over CPUs and do nothing but read some
per-CPU data.

What are we trying to protect against?

What kind of protection do we need?

> 
> > +	/* The last global ASID was consumed while waiting for the
> > lock. */
> > +	if (!global_asid_available) {
> > +		VM_WARN_ONCE(1, "Ran out of global ASIDs\n");
> 
> And? Why do we want to warn here? We need to reset global ASIDs
> anyway.

This warning prints if we run out of global ASIDs,
but have more processes in the system that want one.

This basically helps us figure out whether or not
we should bother implementing more aggressive ASID
re-use (like ARM and RISCV have), which might
require us to figure out how to make that re-use
more scalable than it is today.

> 
> > +		return;
> > +	}
> > +
> > +	asid = allocate_global_asid();
> > +	if (!asid)
> > +		return;
> > +
> > +	assign_mm_global_asid(mm, asid);
> > +}
> > +
> > +void destroy_context_free_global_asid(struct mm_struct *mm)
> 
> That name is a mouthful. mm_free_global_asid() is just fine.
> 
Done.

-- 
All Rights Reversed.