Re: [RFC v2 7/9] x86/mm: Introduce Remote Action Request

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

On Tue, 2025-05-20 at 14:29 +0300, Nadav Amit wrote:
> Not a full review, but..
> 
> > On 20 May 2025, at 4:02, Rik van Riel <riel@surriel.com> wrote:
> > 
> > +/*
> > + * This is a modified version of smp_call_function_many() of
> > kernel/smp.c,
> 
> The updated function names is smp_call_function_many_cond() and it is
> not aligned with smp_call_rar_many. I think the new version is
> (suprisingly)
> better, so it’d be beneficial to bring smp_call_rar_many() to be like
> the
> updated one in smp.c.
> 
Agreed, it will be good to conditionally not send 
the RAR vector to some CPUs, especially ones that
are in deeper idle states.

That means structuring the code more like
smp_call_function_many_cond()

> > +	/*
> > +	 * Can deadlock when called with interrupts disabled.
> > +	 * We allow cpu's that are not yet online though, as no
> > one else can
> > +	 * send smp call function interrupt to this cpu and as
> > such deadlocks
> > +	 * can't happen.
> > +	 */
> > +	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> > +		     && !oops_in_progress &&
> > !early_boot_irqs_disabled);
> 
> I thought you agreed to change it to make it use lockdep instead (so
> it will
> be compiled out without LOCKDEP), like done in
> smp_call_function_many_cond()
> 
I thought I had made that change in my tree.

I guess I lost it in a rebase :(

> > +
> > +	/* Try to fastpath.  So, what's a CPU they want?  Ignoring
> > this one. */
> > +	cpu = cpumask_first_and(mask, cpu_online_mask);
> > +	if (cpu == this_cpu)
> > +		cpu = cpumask_next_and(cpu, mask,
> > cpu_online_mask);
> > +
> 
> Putting aside the rest of the code, I see you don’t call
> should_flush_tlb().
> I think it is worth mentioning in commit log or comment the rationale
> behind
> it (and maybe benchmarks to justify it).
> 
> 
The long term plan here is to simply have the originating
CPU included in the cpumask, and have it send a RAR
request to itself.

That way all the CPUs can invalidate their entries in
parallel, without any extra code.

-- 
All Rights Reversed.