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

[Nadav Amit <namit@vmware.com>]

On Jul 8, 2022, at 7:49 AM, Dave Hansen <dave.hansen@intel.com> 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@vmware.com>
>> 
>> 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.