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?
