Re: [PATCH v2 02/22] x86/mm: Generalize LDT remap into "mm-local region"

[Brendan Jackman <jackmanb@google.com>]

On Fri Mar 20, 2026 at 7:47 PM UTC, Dave Hansen wrote:
> On 3/20/26 11:23, Brendan Jackman wrote:
>>  
>> +#ifdef CONFIG_MM_LOCAL_REGION
>> +static inline void mm_local_region_free(struct mm_struct *mm)
>> +{
>> +	if (mm_local_region_used(mm)) {
>> +		struct mmu_gather tlb;
>> +		unsigned long start = MM_LOCAL_BASE_ADDR;
>> +		unsigned long end = MM_LOCAL_END_ADDR;
>> +
>> +		/*
>> +		 * Although free_pgd_range() is intended for freeing user
>> +		 * page-tables, it also works out for kernel mappings on x86.
>> +		 * We use tlb_gather_mmu_fullmm() to avoid confusing the
>> +		 * range-tracking logic in __tlb_adjust_range().
>> +		 */
>> +		tlb_gather_mmu_fullmm(&tlb, mm);
>
> These are superficial nits and I need to go through this series in
> actual detail, 

Thanks, this series is pretty brutal so I'm very happy to receive
incremental reviews!

> but here are the nits:
>
> Indentation is bad. What you have here double-indents the whole function.
>
> Do this:
>
> 	struct mmu_gather tlb;
> 	unsigned long start = MM_LOCAL_BASE_ADDR;
> 	unsigned long end = MM_LOCAL_END_ADDR;
>
> 	if (!mm_local_region_used(mm))
> 		return;
>
> 	... rest of code here

Ack

>
>> +		 * We use tlb_gather_mmu_fullmm() to avoid confusing the
>> +		 * range-tracking logic in __tlb_adjust_range().
>> +		 */
>
> Imperative voice, please.

Yeah I don't think I'm ever gonna stop making this mistake. Any LLM
should be able to catch this for me, I think it's time to find a way to
get that into my pre-mail workflow.

> And a meta-comment:
>
>>  Documentation/arch/x86/x86_64/mm.rst    |   4 +-
>>  arch/x86/Kconfig                        |   2 +
>>  arch/x86/include/asm/mmu_context.h      | 119 ++++++++++++++++++++++++++++-
>>  arch/x86/include/asm/page.h             |  32 ++++++++
>>  arch/x86/include/asm/pgtable_32_areas.h |   9 ++-
>>  arch/x86/include/asm/pgtable_64_types.h |  12 ++-
>>  arch/x86/kernel/ldt.c                   | 130 +++++---------------------------
>
> This is too big and there's too much going on here. This is doing a few
> logical things like both introducing mm-local regions *and* making the
> LDT remap one of them.

IIRC I tried this but having both an mm-local region and a separate LDT
remap at the same time is annoying, it would mean reviewing temporary
code to deal with both existing at the same time. 

However I just had an idea: I will try creating the mm-local region but
with size 0. Then in a separate patch I'll expand it and simultaneously
move the LDT remap into it.