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

[Yosry Ahmed]

On Tue, Feb 18, 2025 at 02:27:31PM -0800, Dave Hansen wrote:
> 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);
> 	}

Yeah an if/else structure is better than using the invlpgb helper and
falling back to IPIs if it returns false, and I also prefer grouping by
the flush scope (range/flush).

Thanks for the illustrations :)

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