On 16.01.23 14:47, Kirill A. Shutemov wrote:
On Mon, Jan 16, 2023 at 02:07:41PM +0100, David Hildenbrand wrote:
On 16.01.23 13:34, Kirill A. Shutemov wrote:
On Mon, Jan 16, 2023 at 01:06:59PM +0100, Jann Horn wrote:
On Sun, Jan 15, 2023 at 8:07 PM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
On Fri, Jan 13, 2023 at 08:28:59PM +0100, Jann Horn wrote:
No, that lockdep assert has to be there. Page table traversal is
allowed under any one of the mmap lock, the anon_vma lock (if the VMA
is associated with an anon_vma), and the mapping lock (if the VMA is
associated with a mapping); and so to be able to remove page tables,
we must hold all three of them.
Okay, that's fair. I agree with the patch now. Maybe adjust the commit
message a bit?
Just to make sure we're on the same page: Are you suggesting that I
add this text?
"Page table traversal is allowed under any one of the mmap lock, the
anon_vma lock (if the VMA is associated with an anon_vma), and the
mapping lock (if the VMA is associated with a mapping); and so to be
able to remove page tables, we must hold all three of them."
Or something else?
Looks good to me.
Anyway:
Acked-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Thanks!
BTW, I've noticied that you recently added tlb_remove_table_sync_one().
I'm not sure why it is needed. Why IPI in pmdp_collapse_flush() in not
good enough to serialize against GUP fast?
If that sent an IPI, it would be good enough; but
pmdp_collapse_flush() is not guaranteed to send an IPI.
It does a TLB flush, but on some architectures (including arm64 and
also virtualized x86), a remote TLB flush can be done without an IPI.
For example, arm64 has some fancy hardware support for remote TLB
invalidation without IPIs ("broadcast TLB invalidation"), and
virtualized x86 has (depending on the hypervisor) things like TLB
shootdown hypercalls (under Hyper-V, see hyperv_flush_tlb_multi) or
TLB shootdown signalling for preempted CPUs through shared memory
(under KVM, see kvm_flush_tlb_multi).
I think such architectures must provide proper pmdp_collapse_flush()
with the required serialization. Power and S390 already do that.
The plan is to eventually move away from (ab)using IPI to synchronize with
GUP-fast. Moving further into that direction a is wrong.
The flush was added as a quick fix for all architectures by Jann, until
we can do better.
Even for ppc64, see:
commit bedf03416913d88c796288f9dca109a53608c745
Author: Yang Shi <shy828301@xxxxxxxxx>
Date: Wed Sep 7 11:01:44 2022 -0700
powerpc/64s/radix: don't need to broadcast IPI for radix pmd collapse flush
The IPI broadcast is used to serialize against fast-GUP, but fast-GUP will
move to use RCU instead of disabling local interrupts in fast-GUP. Using
an IPI is the old-styled way of serializing against fast-GUP although it
still works as expected now.
And fast-GUP now fixed the potential race with THP collapse by checking
whether PMD is changed or not. So IPI broadcast in radix pmd collapse
flush is not necessary anymore. But it is still needed for hash TLB.
Okay. But I think tlb_remove_table_sync_one() belongs inside
pmdp_collapse_flush(). Collapsing pmd table into huge page without
serialization is a bug. They should not be separate.
Agreed. But I wonder if it should be moved into a generic
pmdp_collapse_flush(), that calls an arch specific __pmdp_collapse_flush().
--
Thanks,
David / dhildenb
