On Jan 17, 2021, at 11:25 AM, Yu Zhao [off-list ref] wrote:

On Sun, Jan 17, 2021 at 02:13:43AM -0800, Nadav Amit wrote:

quoted

quoted

On Jan 17, 2021, at 1:16 AM, Yu Zhao [off-list ref] wrote:

On Sat, Jan 16, 2021 at 11:32:22PM -0800, Nadav Amit wrote:

quoted

quoted

On Jan 16, 2021, at 8:41 PM, Yu Zhao [off-list ref] wrote:

On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:

quoted

On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:

quoted

quoted

On Jan 12, 2021, at 11:56 AM, Yu Zhao [off-list ref] wrote:
On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:

quoted

I will send an RFC soon for per-table deferred TLB flushes tracking.
The basic idea is to save a generation in the page-struct that tracks
when deferred PTE change took place, and track whenever a TLB flush
completed. In addition, other users - such as mprotect - would use
the tlb_gather interface.

Unfortunately, due to limited space in page-struct this would only
be possible for 64-bit (and my implementation is only for x86-64).

I don't want to discourage you but I don't think this would end up
well. PPC doesn't necessarily follow one-page-struct-per-table rule,
and I've run into problems with this before while trying to do
something similar.

Discourage, discourage. Better now than later.

It will be relatively easy to extend the scheme to be per-VMA instead of
per-table for architectures that prefer it this way. It does require
TLB-generation tracking though, which Andy only implemented for x86, so I
will focus on x86-64 right now.

Can you remind me of what we're missing on arm64 in this area, please? I'm
happy to help get this up and running once you have something I can build
on.

I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
Would it be something worth pursuing? Arm has been using mm_cpumask,
so it might not be too difficult I guess?

[ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]

IIUC, there are at least two bugs in x86 implementation.

First, there is a missing memory barrier in tlbbatch_add_mm() between
inc_mm_tlb_gen() and the read of mm_cpumask().

In arch_tlbbatch_add_mm()? inc_mm_tlb_gen() has builtin barrier as its
comment says -- atomic update ops that return values are also full
memory barriers.

Yes, you are correct.

quoted

quoted

Second, try_to_unmap_flush() clears flush_required after flushing. Another
thread can call set_tlb_ubc_flush_pending() after the flush and before
flush_required is cleared, and the indication that a TLB flush is pending
can be lost.

This isn't a problem either because flush_required is per thread.

Sorry, I meant mm->tlb_flush_batched . It is not per-thread.
flush_tlb_batched_pending() clears it after flush and indications that
set_tlb_ubc_flush_pending() sets in between can be lost.

Hmm, the PTL argument above flush_tlb_batched_pending() doesn't seem
to hold when USE_SPLIT_PTE_PTLOCKS is set. Do you have a reproducer?
KCSAN might be able to help in this case.

I do not have a reproducer. It is just based on my understanding of this
code.

I will give a short try for building a reproducer, although for some reason
“you guys” complain that my reproducers do not work for you (is it PTI that
I disable? idle=poll? running in a VM?). It is also not likely to be too
easy to build a reproducer that actually triggers a memory corruption.

Anyhow, apparently KCSAN has already shouted about this code, causing Qian
Cai to add "data_race()" to avoid KCSAN from shouting (9c1177b62a8c
"mm/rmap: annotate a data race at tlb_flush_batched”).

Note that Andrea asked me not to hijack this thread and have a different one
on this issue.