On Jul 8, 2022, at 7:49 AM, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> ⚠ External Email
>
> 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.
>
>> In addition, add the missing unlikely() and jump to get tracing right.
>
> There are currently five routes out of flush_tlb_func():
> * Three early returns
> * One 'goto done'
> * One implicit return
>
> The tracing code doesn't get run for any of the early returns, but
> that's intentional because they don't *actually* flush the TLB. We
> don't want to record that flush_tlb_func() flushed the TLB when it
> didn't. There's another tracepoint on the TLB_REMOTE_SEND_IPI side to
> tell where the flushes were requested.
>
> That said, I think the
>
> if (unlikely(local_tlb_gen == mm_tlb_gen))
> goto done;
>
> is arguably buggy, as is the 'goto done' in this hunk:
I was just trying to follow it for consistency. Will remove.
>
> We might want to (eventually) think about doing something like the
> attached patch to make the skipped flushes explicit in the tracing and
> make the return paths out of this function more consistent.
That’s fine with me. I just recommend that you have a single tracing call in
the function, since having too many ruins the generated code.
