Re: [PATCH v11 06/12] x86/mm: use INVLPGB for kernel TLB flushes

From: Dave Hansen
Date: Tue Feb 18 2025 - 17:27:36 EST


On 2/18/25 10:00, Rik van Riel wrote:
> On Sat, 2025-02-15 at 02:08 +0000, Yosry Ahmed wrote:
>> So I think what Dave wants (and I agree) is:
>> if (!broadcast_kernel_range_flush(info))
>> ipi_kernel_range_flush(info)
>>
>> Where ipi_kernel_range_flush() contains old_thing1() and oldthing2().

That's OK-ish. But it still smells of hacking in the new concept without
refactoring things properly.

Let's logically inline the code that we've got. I think it actually
ends up looking something like this:

if (cpu_feature_enabled(X86_FEATURE_INVLPGB)) {
if (info->end == TLB_FLUSH_ALL) {
invlpgb_flush_all();
} else {
for_each(addr)
invlpgb_flush_addr_nosync(addr, nr);
}
} else {
if (info->end == TLB_FLUSH_ALL)
on_each_cpu(do_flush_tlb_all, NULL, 1);
else
on_each_cpu(do_kernel_range_flush, info, 1);
}

Where we've got two inputs:

1. INVLPGB support (or not)
2. TLB_FLUSH_ALL (basically ranged or full flush)

So I think we should group by *one* of those. The above groups by
INVLPGB support and this groups by TLB_FLUSH_ALL:

if (info->end == TLB_FLUSH_ALL) {
if (cpu_feature_enabled(X86_FEATURE_INVLPGB)) {
invlpgb_flush_all();
} else {
on_each_cpu(do_flush_tlb_all, NULL, 1);
}
} else {
if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
for_each(addr)
invlpgb_flush_addr_nosync(addr, nr);
else
on_each_cpu(do_kernel_range_flush, info, 1);
}

So, if we create some helpers that give some consistent naming:

static tlb_flush_all_ipi(...)
{
on_each_cpu(do_flush_tlb_all, NULL, 1);
}

static tlb_flush_all(...)
{
if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
invlpgb_flush_all(...);
else
tlb_flush_all_ipi(...);
}

and then also create the ranged equivalents (which internally have the
same cpu_feature_enabled() check):

tlb_flush_range_ipi(...)
invlpgb_flush_range(...)

Then we can have the top-level code be:

if (info->end == TLB_FLUSH_ALL)
tlb_flush_all(info);
else
tlb_flush_range(info);

That actually looks way nicer than what we have today. For bonus points,
if a third way of flushing the TLB showed up, it would slot right in:

static tlb_flush_all(...)
{
if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
invlpgb_flush_all(...);
+ else if cpu_feature_enabled(X86_FEATURE_RAR))
+ rar_flush_all(...);
else
tlb_flush_all_ipi(...);
}

That's *exactly* the way we want the code to read. At the higher level,
it's deciding based on the generic thing that *everybody* cares about:
ranged or full flush. Then, at the lower level, it's deciding how to
implement that high-level flush concept.