Re: [PATCH] x86/mm/tlb: ignore f->new_tlb_gen when zero

[Nadav Amit]

On Jul 8, 2022, at 8:13 AM, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:

> ⚠ External Email
> 
> On 7/8/22 04:40, David Hildenbrand wrote:
>>> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
>>> index d9314cc8b81f..d81b4084bb8a 100644
>>> --- a/arch/x86/mm/tlb.c
>>> +++ b/arch/x86/mm/tlb.c
>>> @@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
>>>             return;
>>>     }
>>> 
>>> -    if (f->new_tlb_gen <= local_tlb_gen) {
>>> +    if (unlikely(f->new_tlb_gen != 0 && f->new_tlb_gen <= local_tlb_gen)) {
>>>             /*
>>>              * The TLB is already up to date in respect to f->new_tlb_gen.
>>>              * While the core might be still behind mm_tlb_gen, checking
>>>              * mm_tlb_gen unnecessarily would have negative caching effects
>>>              * so avoid it.
>>>              */
>>> -            return;
>>> +            goto done;
>> Does this affect the performance numbers from aa44284960d5 ("x86/mm/tlb:
>> Avoid reading mm_tlb_gen when possible")?
> 
> It depends on how many batched flushes that workload had.  From the
> looks of it, they're all one page:
> 
>        madvise(addr + i, pgsize, MADV_DONTNEED);
> 
> so there shouldn't be *much* batching in play.  But, it wouldn't hurt to
> re-run them in either case.

Just to clarify, since these things are confusing.

There are two batching mechanisms. The common one is mmu_gather, which
MADV_DONTNEED uses. This one is *not* the one that caused the breakage.

The second one is the “unmap_batch”, which was only used by x86 until now.
(I just saw patches for ARM, but I think they just exploit the interface in
a way). The “unmap_batch” is used when you swap out. This was broken.

Since the bug was not during MADV_DONTNEED there is no reason for the
results to be any different.

Famous last words?