On 7/8/22 10:04, Nadav Amit wrote:
> On Jul 8, 2022, at 7:49 AM, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>> On 7/7/22 17:30, Nadav Amit wrote:
>> You might want to fix the clock on the system from which you sent this.
>> I was really scratching my head trying to figure out how you got this
>> patch out before Hugh's bug report.
>>
>>> From: Nadav Amit <namit@xxxxxxxxxx>
>>>
>>> Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when
>>> possible") introduced an optimization of skipping the flush if the TLB
>>> generation that is flushed (as provided in flush_tlb_info) was already
>>> flushed.
>>>
>>> However, arch_tlbbatch_flush() does not provide any generation in
>>> flush_tlb_info. As a result, try_to_unmap_one() would not perform any
>>> TLB flushes.
>>>
>>> Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is
>>> anyhow is an invalid generation value.
>>
>> It is, but the check below uses 'f->end == TLB_FLUSH_ALL' as the marker
>> for f->new_tlb_gen being invalid. Being consistent seems like a good
>> idea on this stuff.
>
> If we get a request to do a flush, regardless whether full or partial,
> that logically we have already done, there is not reason to do it.
>
> I therefore do not see a reason to look on f->end. I think that looking
> at the generation is very intuitive. If you want, I can add a constant
> such as TLB_GENERATION_INVALID.
That's a good point.
But, _my_ point was that there was only really one read site of
f->new_tlb_gen in flush_tlb_func(). That site is guarded by the "f->end
!= TLB_FLUSH_ALL" check which prevented it from making the same error
that your patch did.
Whatever we do, it would be nice to have a *single* way to check for
"does f->new_tlb_gen have an actual, valid bit of tlb gen data in it?"
Using something like TLB_GENERATION_INVALID seems reasonable to me.
