| Message ID | 20221022114424.515572025@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up pmd_get_atomic() and i386-PAE | expand |
On 10/22/22 04:14, Peter Zijlstra wrote: > --- a/include/linux/pgtable.h > +++ b/include/linux/pgtable.h > @@ -260,15 +260,12 @@ static inline pte_t ptep_get(pte_t *ptep > > #ifdef CONFIG_GUP_GET_PTE_LOW_HIGH > /* > - * WARNING: only to be used in the get_user_pages_fast() implementation. > - * > - * With get_user_pages_fast(), we walk down the pagetables without taking any > - * locks. For this we would like to load the pointers atomically, but sometimes > - * that is not possible (e.g. without expensive cmpxchg8b on x86_32 PAE). What > - * we do have is the guarantee that a PTE will only either go from not present > - * to present, or present to not present or both -- it will not switch to a > - * completely different present page without a TLB flush in between; something > - * that we are blocking by holding interrupts off. > + * For walking the pagetables without holding any locks. Some architectures > + * (eg x86-32 PAE) cannot load the entries atomically without using expensive > + * instructions. We are guaranteed that a PTE will only either go from not > + * present to present, or present to not present -- it will not switch to a > + * completely different present page without a TLB flush inbetween; which we > + * are blocking by holding interrupts off. This is getting interesting. My latest understanding of this story is that both the "before" and "after" versions of that comment are incorrect! Because, as Jann Horn noticed recently [1], there might not be any IPIs involved in a TLB flush, if x86 is running under a hypervisor, and that breaks the chain of reasoning here. [1] https://lore.kernel.org/all/CAG48ez3h-mnp9ZFC10v+-BW_8NQvxbwBsMYJFP8JX31o0B17Pg@mail.gmail.com/ thanks,
On Sun, Oct 23, 2022 at 10:42:49PM -0700, John Hubbard wrote: > On 10/22/22 04:14, Peter Zijlstra wrote: > > --- a/include/linux/pgtable.h > > +++ b/include/linux/pgtable.h > > @@ -260,15 +260,12 @@ static inline pte_t ptep_get(pte_t *ptep > > > > #ifdef CONFIG_GUP_GET_PTE_LOW_HIGH > > /* > > - * WARNING: only to be used in the get_user_pages_fast() implementation. > > - * > > - * With get_user_pages_fast(), we walk down the pagetables without taking any > > - * locks. For this we would like to load the pointers atomically, but sometimes > > - * that is not possible (e.g. without expensive cmpxchg8b on x86_32 PAE). What > > - * we do have is the guarantee that a PTE will only either go from not present > > - * to present, or present to not present or both -- it will not switch to a > > - * completely different present page without a TLB flush in between; something > > - * that we are blocking by holding interrupts off. > > + * For walking the pagetables without holding any locks. Some architectures > > + * (eg x86-32 PAE) cannot load the entries atomically without using expensive > > + * instructions. We are guaranteed that a PTE will only either go from not > > + * present to present, or present to not present -- it will not switch to a > > + * completely different present page without a TLB flush inbetween; which we > > + * are blocking by holding interrupts off. > > > This is getting interesting. My latest understanding of this story is > that both the "before" and "after" versions of that comment are > incorrect! Because, as Jann Horn noticed recently [1], there might not > be any IPIs involved in a TLB flush, if x86 is running under a > hypervisor, and that breaks the chain of reasoning here. That mail doesn't really include enough detail. The way x86 HV TLB flushing is supposed to work is by making use of MMU_GATHER_RCU_TABLE_FREE. Specifically, something like: vCPU0 vCPU1 tlb_gather_mmut(&tlb, mm); .... local_irq_disable(); ... starts page-table walk ... <schedules out; sets KVM_VCPU_PREEMPTED> tlb_finish_mmu(&tlb) ... kvm_flush_tlb_multi() if (state & KVM_VCPU_PREEMPTED) if (try_cmpxchg(,&state, state | KVM_VCPU_FLUSH_TLB)) __cpumask_clear_cpu(cpu, flushmask); tlb_remove_table_sync_one() / call_rcu() <schedules back in> ... continues page-table walk ... local_irq_enable(); If mmu gather is forced into tlb_remove_talbe_sync_one() (by memory pressure), then you've got your IPI back, otherwise it does call_rcu() and RCU itself will need vCPU0 to enable IRQs in order to make progress. Either way around, the actual freeing of the pages is delayed until the page-table walk is finished. What am I missing?
On Mon, Oct 24, 2022 at 10:01 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Sun, Oct 23, 2022 at 10:42:49PM -0700, John Hubbard wrote: > > On 10/22/22 04:14, Peter Zijlstra wrote: > > > --- a/include/linux/pgtable.h > > > +++ b/include/linux/pgtable.h > > > @@ -260,15 +260,12 @@ static inline pte_t ptep_get(pte_t *ptep > > > > > > #ifdef CONFIG_GUP_GET_PTE_LOW_HIGH > > > /* > > > - * WARNING: only to be used in the get_user_pages_fast() implementation. > > > - * > > > - * With get_user_pages_fast(), we walk down the pagetables without taking any > > > - * locks. For this we would like to load the pointers atomically, but sometimes > > > - * that is not possible (e.g. without expensive cmpxchg8b on x86_32 PAE). What > > > - * we do have is the guarantee that a PTE will only either go from not present > > > - * to present, or present to not present or both -- it will not switch to a > > > - * completely different present page without a TLB flush in between; something > > > - * that we are blocking by holding interrupts off. > > > + * For walking the pagetables without holding any locks. Some architectures > > > + * (eg x86-32 PAE) cannot load the entries atomically without using expensive > > > + * instructions. We are guaranteed that a PTE will only either go from not > > > + * present to present, or present to not present -- it will not switch to a > > > + * completely different present page without a TLB flush inbetween; which we > > > + * are blocking by holding interrupts off. > > > > > > This is getting interesting. My latest understanding of this story is > > that both the "before" and "after" versions of that comment are > > incorrect! Because, as Jann Horn noticed recently [1], there might not > > be any IPIs involved in a TLB flush, if x86 is running under a > > hypervisor, and that breaks the chain of reasoning here. > > That mail doesn't really include enough detail. The way x86 HV TLB > flushing is supposed to work is by making use of > MMU_GATHER_RCU_TABLE_FREE. Specifically, something like: > > > vCPU0 vCPU1 > > tlb_gather_mmut(&tlb, mm); > > .... > > local_irq_disable(); > ... starts page-table walk ... > > <schedules out; sets KVM_VCPU_PREEMPTED> > > tlb_finish_mmu(&tlb) > ... > kvm_flush_tlb_multi() > if (state & KVM_VCPU_PREEMPTED) > if (try_cmpxchg(,&state, state | KVM_VCPU_FLUSH_TLB)) > __cpumask_clear_cpu(cpu, flushmask); > > > tlb_remove_table_sync_one() / call_rcu() > > > <schedules back in> > > ... continues page-table walk ... > local_irq_enable(); > > If mmu gather is forced into tlb_remove_talbe_sync_one() (by memory > pressure), then you've got your IPI back, otherwise it does call_rcu() > and RCU itself will need vCPU0 to enable IRQs in order to make progress. > > Either way around, the actual freeing of the pages is delayed until the > page-table walk is finished. > > What am I missing? Unless I'm completely misunderstanding what's going on here, the whole "remove_table" thing only happens when you "remove a table", meaning you free an entire *pagetable*. Just zapping PTEs doesn't trigger that logic.
On Mon, Oct 24, 2022 at 12:58 PM Jann Horn <jannh@google.com> wrote: > > Unless I'm completely misunderstanding what's going on here, the whole > "remove_table" thing only happens when you "remove a table", meaning > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > logic. I do have to admit that I'd be happier if this code - and the GUP code that also relies on "interrupts off" behavior - would just use a sequence counter instead. Relying on blocking IPI's is clever, but also clearly very subtle and somewhat dangerous. I think our GUP code is a *lot* more important than some "legacy x86-32 has problems in case you have an incredibly unlikely race that re-populates the page table with a different page that just happens to be exactly the same MOD-4GB", so honestly, I don't think the load-tearing is even worth worrying about - if you have hardware that is good enough at virtualizing things, it's almost certainly already 64-bit, and running 32-bit virtual machines with PAE you really only have yourself to blame. So I can't find it in myself to care about the 32-bit tearing thing, but this discussion makes me worried about Fast GUP. Note that even with proper atomic pte_t pte = ptep_get_lockless(ptep); in gup_pte_range(), and even if the page tables are RCU-free'd, that just means that the 'ptep' access itself is safe. But then you have the whole "the lookup of the page pointer is not atomic" wrt that. And right now that GUP code does rely on the "block IPI" to make it basically valid. I don't think it matters if GUP races with munmap or madvise() or something like that - if you get the old page, that's still a valid page, and the user only has himself to blame. But if we have memory pressure that causes vmscan to push out a page, and it gets replaced with a new page, and GUP gets the old page with no serialization, that sounds like a possible source of data inconsistency. I don't know if this can happen, but the whole "interrupts disabled doesn't actually block IPI's and synchronize with TLB flushes" really sounds like it would affect GUP too. And be much more serious there than on some x86-32 platform that nobody should be using anyway. Linus
On Mon, Oct 24, 2022 at 10:19 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Mon, Oct 24, 2022 at 12:58 PM Jann Horn <jannh@google.com> wrote: > > > > Unless I'm completely misunderstanding what's going on here, the whole > > "remove_table" thing only happens when you "remove a table", meaning > > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > > logic. > > I do have to admit that I'd be happier if this code - and the GUP code > that also relies on "interrupts off" behavior - would just use a > sequence counter instead. > > Relying on blocking IPI's is clever, but also clearly very subtle and > somewhat dangerous. > > I think our GUP code is a *lot* more important than some "legacy > x86-32 has problems in case you have an incredibly unlikely race that > re-populates the page table with a different page that just happens to > be exactly the same MOD-4GB", so honestly, I don't think the > load-tearing is even worth worrying about - if you have hardware that > is good enough at virtualizing things, it's almost certainly already > 64-bit, and running 32-bit virtual machines with PAE you really only > have yourself to blame. > > So I can't find it in myself to care about the 32-bit tearing thing, > but this discussion makes me worried about Fast GUP. > > Note that even with proper atomic > > pte_t pte = ptep_get_lockless(ptep); > > in gup_pte_range(), and even if the page tables are RCU-free'd, that > just means that the 'ptep' access itself is safe. > > But then you have the whole "the lookup of the page pointer is not > atomic" wrt that. And right now that GUP code does rely on the "block > IPI" to make it basically valid. > > I don't think it matters if GUP races with munmap or madvise() or > something like that - if you get the old page, that's still a valid > page, and the user only has himself to blame. > > But if we have memory pressure that causes vmscan to push out a page, > and it gets replaced with a new page, and GUP gets the old page with > no serialization, that sounds like a possible source of data > inconsistency. > > I don't know if this can happen, but the whole "interrupts disabled > doesn't actually block IPI's and synchronize with TLB flushes" really > sounds like it would affect GUP too. And be much more serious there > than on some x86-32 platform that nobody should be using anyway. That's why GUP-fast re-checks the PTE after it has grabbed a reference to the page. Pasting from a longer writeup that I plan to publish soon, describing how gup_pte_range() works: """ This guarantees that the page tables that are being walked aren't freed concurrently, but at the end of the walk, we have to grab a stable reference to the referenced page. For this we use the grab-reference-and-revalidate trick from above again: First we (locklessly) load the page table entry, then we grab a reference to the page that it points to (which can fail if the refcount is zero, in that case we bail), then we recheck that the page table entry is still the same, and if it changed in between, we drop the page reference and bail. This can, again, grab a reference to a page after it has already been freed and reallocated. The reason why this is fine is that the metadata structure that holds this refcount, `struct folio` (or `struct page`, depending on which kernel version you're looking at; in current kernels it's `folio` but `struct page` and `struct folio` are actually aliases for the same memory, basically, though that is supposed to maybe change at some point) is never freed; even when a page is freed and reallocated, the corresponding `struct folio` stays. This does have the fun consequence that whenever a page/folio has a non-zero refcount, the refcount can spuriously go up and then back down for a little bit. (Also it's technically not as simple as I just described it, because the `struct page` that the PTE points to might be a "tail page" of a `struct folio`. So actually we first read the PTE, the PTE gives us the `page*`, then from that we go to the `folio*`, then we try to grab a reference to the `folio`, then if that worked we check that the `page` still points to the same `folio`, and then we recheck that the PTE is still the same.) """
On Mon, Oct 24, 2022 at 1:24 PM Jann Horn <jannh@google.com> wrote: > > That's why GUP-fast re-checks the PTE after it has grabbed a reference > to the page. Bah. I should have known that. We got that through the PPC version that never had the whole IPI serialization thing (just the RCU freeing). I haven't looked at that code in much too long. Actually, by "much too long" I really mean "thankfully I haven't needed to" ;) Linus
On Mon, Oct 24, 2022 at 10:23:51PM +0200, Jann Horn wrote: > """ > This guarantees that the page tables that are being walked > aren't freed concurrently, but at the end of the walk, we > have to grab a stable reference to the referenced page. > For this we use the grab-reference-and-revalidate trick > from above again: > First we (locklessly) load the page > table entry, then we grab a reference to the page that it > points to (which can fail if the refcount is zero, in that > case we bail), then we recheck that the page table entry > is still the same, and if it changed in between, we drop the > page reference and bail. > This can, again, grab a reference to a page after it has > already been freed and reallocated. The reason why this is > fine is that the metadata structure that holds this refcount, > `struct folio` (or `struct page`, depending on which kernel > version you're looking at; in current kernels it's `folio` > but `struct page` and `struct folio` are actually aliases for > the same memory, basically, though that is supposed to maybe > change at some point) is never freed; even when a page is > freed and reallocated, the corresponding `struct folio` > stays. This does have the fun consequence that whenever a > page/folio has a non-zero refcount, the refcount can > spuriously go up and then back down for a little bit. > (Also it's technically not as simple as I just described it, > because the `struct page` that the PTE points to might be > a "tail page" of a `struct folio`. > So actually we first read the PTE, the PTE gives us the > `page*`, then from that we go to the `folio*`, then we > try to grab a reference to the `folio`, then if that worked > we check that the `page` still points to the same `folio`, > and then we recheck that the PTE is still the same.) > """ Nngh. In trying to make this description fit all kernels (with both pages and folios), you've complicated it maximally. Let's try a more simple explanation: First we (locklessly) load the page table entry, then we grab a reference to the folio that contains it (which can fail if the refcount is zero, in that case we bail), then we recheck that the page table entry is still the same, and if it changed in between, we drop the folio reference and bail. This can, again, grab a reference to a folio after it has already been freed and reallocated. The reason why this is fine is that the metadata structure that holds this refcount, `struct folio` is never freed; even when a folio is freed and reallocated, the corresponding `struct folio` stays. This does have the fun consequence that whenever a folio has a non-zero refcount, the refcount can spuriously go up and then back down for a little bit. (Also it's slightly more complex than I just described, because the PTE that we just loaded might be in the middle of being reallocated into a different folio. So actually we first read the PTE, translate the PTE into the `page*`, then from that we go to the `folio*`, then we try to grab a reference to the `folio`, then if that worked we check that the `page` is still in the same `folio`, and then we recheck that the PTE is still the same. Older kernels did not make a clear distinction between pages and folios, so it was even more confusing.) Better?
Matthew Wilcox <willy@infradead.org> writes: > On Mon, Oct 24, 2022 at 10:23:51PM +0200, Jann Horn wrote: >> """ >> This guarantees that the page tables that are being walked >> aren't freed concurrently, but at the end of the walk, we >> have to grab a stable reference to the referenced page. >> For this we use the grab-reference-and-revalidate trick >> from above again: >> First we (locklessly) load the page >> table entry, then we grab a reference to the page that it >> points to (which can fail if the refcount is zero, in that >> case we bail), then we recheck that the page table entry >> is still the same, and if it changed in between, we drop the >> page reference and bail. >> This can, again, grab a reference to a page after it has >> already been freed and reallocated. The reason why this is >> fine is that the metadata structure that holds this refcount, >> `struct folio` (or `struct page`, depending on which kernel >> version you're looking at; in current kernels it's `folio` >> but `struct page` and `struct folio` are actually aliases for >> the same memory, basically, though that is supposed to maybe >> change at some point) is never freed; even when a page is >> freed and reallocated, the corresponding `struct folio` >> stays. This does have the fun consequence that whenever a >> page/folio has a non-zero refcount, the refcount can >> spuriously go up and then back down for a little bit. >> (Also it's technically not as simple as I just described it, >> because the `struct page` that the PTE points to might be >> a "tail page" of a `struct folio`. >> So actually we first read the PTE, the PTE gives us the >> `page*`, then from that we go to the `folio*`, then we >> try to grab a reference to the `folio`, then if that worked >> we check that the `page` still points to the same `folio`, >> and then we recheck that the PTE is still the same.) >> """ > > Nngh. In trying to make this description fit all kernels (with > both pages and folios), you've complicated it maximally. Let's > try a more simple explanation: > > First we (locklessly) load the page table entry, then we grab a > reference to the folio that contains it (which can fail if the > refcount is zero, in that case we bail), then we recheck that the > page table entry is still the same, and if it changed in between, > we drop the folio reference and bail. > This can, again, grab a reference to a folio after it has > already been freed and reallocated. The reason why this is > fine is that the metadata structure that holds this refcount, > `struct folio` is never freed; even when a folio is > freed and reallocated, the corresponding `struct folio` > stays. I'm probably missing something obvious but how is that synchronised against memory hotplug? AFAICT if it isn't couldn't the pages be freed and memory removed? In that case the above would no longer hold because (I think) the metadata structure could have been freed. > This does have the fun consequence that whenever a > folio has a non-zero refcount, the refcount can > spuriously go up and then back down for a little bit. > (Also it's slightly more complex than I just described, > because the PTE that we just loaded might be in the middle of > being reallocated into a different folio. > So actually we first read the PTE, translate the PTE into the > `page*`, then from that we go to the `folio*`, then we > try to grab a reference to the `folio`, then if that worked > we check that the `page` is still in the same `folio`, > and then we recheck that the PTE is still the same. Older kernels > did not make a clear distinction between pages and folios, so > it was even more confusing.) > > > Better?
On Tue, Oct 25, 2022 at 06:54:10PM +1100, Alistair Popple wrote: > > First we (locklessly) load the page table entry, then we grab a > > reference to the folio that contains it (which can fail if the > > refcount is zero, in that case we bail), then we recheck that the > > page table entry is still the same, and if it changed in between, > > we drop the folio reference and bail. > > This can, again, grab a reference to a folio after it has > > already been freed and reallocated. The reason why this is > > fine is that the metadata structure that holds this refcount, > > `struct folio` is never freed; even when a folio is > > freed and reallocated, the corresponding `struct folio` > > stays. > > I'm probably missing something obvious but how is that synchronised > against memory hotplug? AFAICT if it isn't couldn't the pages be freed > and memory removed? In that case the above would no longer hold because > (I think) the metadata structure could have been freed. Note, this scheme is older than memory hot-plug, so if anybody is to blame it's the memory hotplug code. Anyway, since all that is done with IRQs disabled, all the hotplug stuff needs to do is rcu_synchronize() in order to ensure all active IRQ-disabled regions are finshed (between ensuring the memory is unused and taking out the struct page).
On Tue, Oct 25, 2022 at 10:11 AM Alistair Popple <apopple@nvidia.com> wrote: > > > Matthew Wilcox <willy@infradead.org> writes: > > > On Mon, Oct 24, 2022 at 10:23:51PM +0200, Jann Horn wrote: > >> """ > >> This guarantees that the page tables that are being walked > >> aren't freed concurrently, but at the end of the walk, we > >> have to grab a stable reference to the referenced page. > >> For this we use the grab-reference-and-revalidate trick > >> from above again: > >> First we (locklessly) load the page > >> table entry, then we grab a reference to the page that it > >> points to (which can fail if the refcount is zero, in that > >> case we bail), then we recheck that the page table entry > >> is still the same, and if it changed in between, we drop the > >> page reference and bail. > >> This can, again, grab a reference to a page after it has > >> already been freed and reallocated. The reason why this is > >> fine is that the metadata structure that holds this refcount, > >> `struct folio` (or `struct page`, depending on which kernel > >> version you're looking at; in current kernels it's `folio` > >> but `struct page` and `struct folio` are actually aliases for > >> the same memory, basically, though that is supposed to maybe > >> change at some point) is never freed; even when a page is > >> freed and reallocated, the corresponding `struct folio` > >> stays. This does have the fun consequence that whenever a > >> page/folio has a non-zero refcount, the refcount can > >> spuriously go up and then back down for a little bit. > >> (Also it's technically not as simple as I just described it, > >> because the `struct page` that the PTE points to might be > >> a "tail page" of a `struct folio`. > >> So actually we first read the PTE, the PTE gives us the > >> `page*`, then from that we go to the `folio*`, then we > >> try to grab a reference to the `folio`, then if that worked > >> we check that the `page` still points to the same `folio`, > >> and then we recheck that the PTE is still the same.) > >> """ > > > > Nngh. In trying to make this description fit all kernels (with > > both pages and folios), you've complicated it maximally. Let's > > try a more simple explanation: > > > > First we (locklessly) load the page table entry, then we grab a > > reference to the folio that contains it (which can fail if the > > refcount is zero, in that case we bail), then we recheck that the > > page table entry is still the same, and if it changed in between, > > we drop the folio reference and bail. > > This can, again, grab a reference to a folio after it has > > already been freed and reallocated. The reason why this is > > fine is that the metadata structure that holds this refcount, > > `struct folio` is never freed; even when a folio is > > freed and reallocated, the corresponding `struct folio` > > stays. Oh, thanks. You're right, trying to talk about kernels with folios made it unnecessarily complicated... > I'm probably missing something obvious but how is that synchronised > against memory hotplug? AFAICT if it isn't couldn't the pages be freed > and memory removed? In that case the above would no longer hold because > (I think) the metadata structure could have been freed. Hm... that's this codepath? arch_remove_memory -> __remove_pages -> __remove_section -> sparse_remove_section -> section_deactivate -> depopulate_section_memmap -> vmemmap_free -> remove_pagetable which then walks down the page tables and ends up freeing individual pages in remove_pte_table() using the confusingly-named free_pagetable()? I'm not sure what the synchronization against hotplug is - GUP-fast is running with IRQs disabled, but other codepaths might not, like get_ksm_page()? I don't know if that's holding something else for protection...
On Mon, Oct 24, 2022 at 09:58:07PM +0200, Jann Horn wrote: > Unless I'm completely misunderstanding what's going on here, the whole > "remove_table" thing only happens when you "remove a table", meaning > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > logic. Aah; yes true. OTOH even it that were not so, I think it would still be broken because the current code relies on the TLB flush to have completed, whereas the RCU scheme is effectively async and can be considered pending until the callback runs. Hurmph... easiest fix is probably to dis-allow kvm_flush_tlb_multi() for i386-pae builds. Something like so... nobody in his right mind should care about i386-pae virt performance much. --- diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 95fb85bea111..cbfb84e88251 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -473,6 +473,12 @@ static DEFINE_PER_CPU(cpumask_var_t, __pv_cpu_mask); static bool pv_tlb_flush_supported(void) { + /* + * i386-PAE split loads are incompatible with optimized TLB flushes. + */ + if (IS_ENABLED(CONFIG_GUP_GET_PTE_LOW_HIGH)) + return false; + return (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) && !kvm_para_has_hint(KVM_HINTS_REALTIME) && kvm_para_has_feature(KVM_FEATURE_STEAL_TIME) &&
On Tue, Oct 25, 2022 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Oct 24, 2022 at 09:58:07PM +0200, Jann Horn wrote: > > > Unless I'm completely misunderstanding what's going on here, the whole > > "remove_table" thing only happens when you "remove a table", meaning > > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > > logic. > > Aah; yes true. OTOH even it that were not so, I think it would still be > broken because the current code relies on the TLB flush to have > completed, whereas the RCU scheme is effectively async and can be > considered pending until the callback runs. > > Hurmph... easiest fix is probably to dis-allow kvm_flush_tlb_multi() > for i386-pae builds. > > Something like so... nobody in his right mind should care about i386-pae > virt performance much. I think Xen and HyperV have similar codepaths. hyperv_flush_tlb_multi() looks like it uses remote flush hypercalls, xen_flush_tlb_multi() too. On top of that, I think that theoretically, Linux doesn't even ensure that you have a TLB flush in between tearing down one PTE and installing another PTE (see https://lore.kernel.org/all/CAG48ez1Oz4tT-N2Y=Zs6jumu=zOp7SQRZ=V2c+b5bT9P4retJA@mail.gmail.com/), but I haven't tested that, and if it is true, I'm also not entirely sure if it's correct (in the sense that it only creates incoherent-TLB states when userspace is doing something stupid like racing MADV_DONTNEED and page faults on the same region). I think the more clearly correct fix would be to get rid of the split loads and use CMPXCHG16B instead (probably destroying the performance of GUP-fast completely), but that's complicated because some of the architectures that use the split loads path don't have cmpxchg_double (or at least don't have it wired up).
On Tue, Oct 25, 2022 at 04:18:20PM +0200, Jann Horn wrote: > On Tue, Oct 25, 2022 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Oct 24, 2022 at 09:58:07PM +0200, Jann Horn wrote: > > > > > Unless I'm completely misunderstanding what's going on here, the whole > > > "remove_table" thing only happens when you "remove a table", meaning > > > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > > > logic. > > > > Aah; yes true. OTOH even it that were not so, I think it would still be > > broken because the current code relies on the TLB flush to have > > completed, whereas the RCU scheme is effectively async and can be > > considered pending until the callback runs. > > > > Hurmph... easiest fix is probably to dis-allow kvm_flush_tlb_multi() > > for i386-pae builds. > > > > Something like so... nobody in his right mind should care about i386-pae > > virt performance much. > > I think Xen and HyperV have similar codepaths. > hyperv_flush_tlb_multi() looks like it uses remote flush hypercalls, > xen_flush_tlb_multi() too. Sure (not updated). > On top of that, I think that theoretically, Linux doesn't even ensure > that you have a TLB flush in between tearing down one PTE and > installing another PTE (see > https://lore.kernel.org/all/CAG48ez1Oz4tT-N2Y=Zs6jumu=zOp7SQRZ=V2c+b5bT9P4retJA@mail.gmail.com/), > but I haven't tested that, and if it is true, I'm also not entirely > sure if it's correct (in the sense that it only creates incoherent-TLB > states when userspace is doing something stupid like racing > MADV_DONTNEED and page faults on the same region). > > I think the more clearly correct fix would be to get rid of the split > loads and use CMPXCHG16B instead (probably destroying the performance > of GUP-fast completely), but that's complicated because some of the > architectures that use the split loads path don't have cmpxchg_double > (or at least don't have it wired up). cmpxchg8b; but no, I think we want to fix MADV_DONTNEED, incoherent TLB states are a pain nobody needs. Something like so should force TLB flushes before dropping pte_lock (not looked at the various pmd level things yet). diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 95fb85bea111..cbfb84e88251 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -473,6 +473,12 @@ static DEFINE_PER_CPU(cpumask_var_t, __pv_cpu_mask); static bool pv_tlb_flush_supported(void) { + /* + * i386-PAE split loads are incompatible with optimized TLB flushes. + */ + if (IS_ENABLED(CONFIG_GUP_GET_PTE_LOW_HIGH)) + return false; + return (kvm_para_has_feature(KVM_FEATURE_PV_TLB_FLUSH) && !kvm_para_has_hint(KVM_HINTS_REALTIME) && kvm_para_has_feature(KVM_FEATURE_STEAL_TIME) && diff --git a/include/linux/mm.h b/include/linux/mm.h index 8bbcccbc5565..397bc04e2d82 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3474,5 +3474,6 @@ madvise_set_anon_name(struct mm_struct *mm, unsigned long start, * default, the flag is not set. */ #define ZAP_FLAG_DROP_MARKER ((__force zap_flags_t) BIT(0)) +#define ZAP_FLAG_FORCE_FLUSH ((__force zap_flags_t) BIT(1)) #endif /* _LINUX_MM_H */ diff --git a/mm/memory.c b/mm/memory.c index f88c351aecd4..9bb63b3fbee1 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1440,6 +1440,11 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, tlb_remove_tlb_entry(tlb, pte, addr); zap_install_uffd_wp_if_needed(vma, addr, pte, details, ptent); + + if (!force_flush && !tlb->fullmm && details && + details->zap_flags & ZAP_FLAG_FORCE_FLUSH) + force_flush = 1; + if (unlikely(!page)) continue; @@ -1749,6 +1754,9 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start, struct maple_tree *mt = &vma->vm_mm->mm_mt; unsigned long end = start + size; struct mmu_notifier_range range; + struct zap_details details = { + .zap_flags = ZAP_FLAG_FORCE_FLUSH, + }; struct mmu_gather tlb; MA_STATE(mas, mt, vma->vm_end, vma->vm_end); @@ -1759,7 +1767,7 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start, update_hiwater_rss(vma->vm_mm); mmu_notifier_invalidate_range_start(&range); do { - unmap_single_vma(&tlb, vma, start, range.end, NULL); + unmap_single_vma(&tlb, vma, start, range.end, &details); } while ((vma = mas_find(&mas, end - 1)) != NULL); mmu_notifier_invalidate_range_end(&range); tlb_finish_mmu(&tlb); @@ -1806,7 +1814,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address, unsigned long size) { if (!range_in_vma(vma, address, address + size) || - !(vma->vm_flags & VM_PFNMAP)) + !(vma->vm_flags & VM_PFNMAP)) return; zap_page_range_single(vma, address, size, NULL);
Jann Horn <jannh@google.com> writes: > On Tue, Oct 25, 2022 at 10:11 AM Alistair Popple <apopple@nvidia.com> wrote: >> >> >> Matthew Wilcox <willy@infradead.org> writes: >> >> > On Mon, Oct 24, 2022 at 10:23:51PM +0200, Jann Horn wrote: >> >> """ >> >> This guarantees that the page tables that are being walked >> >> aren't freed concurrently, but at the end of the walk, we >> >> have to grab a stable reference to the referenced page. >> >> For this we use the grab-reference-and-revalidate trick >> >> from above again: >> >> First we (locklessly) load the page >> >> table entry, then we grab a reference to the page that it >> >> points to (which can fail if the refcount is zero, in that >> >> case we bail), then we recheck that the page table entry >> >> is still the same, and if it changed in between, we drop the >> >> page reference and bail. >> >> This can, again, grab a reference to a page after it has >> >> already been freed and reallocated. The reason why this is >> >> fine is that the metadata structure that holds this refcount, >> >> `struct folio` (or `struct page`, depending on which kernel >> >> version you're looking at; in current kernels it's `folio` >> >> but `struct page` and `struct folio` are actually aliases for >> >> the same memory, basically, though that is supposed to maybe >> >> change at some point) is never freed; even when a page is >> >> freed and reallocated, the corresponding `struct folio` >> >> stays. This does have the fun consequence that whenever a >> >> page/folio has a non-zero refcount, the refcount can >> >> spuriously go up and then back down for a little bit. >> >> (Also it's technically not as simple as I just described it, >> >> because the `struct page` that the PTE points to might be >> >> a "tail page" of a `struct folio`. >> >> So actually we first read the PTE, the PTE gives us the >> >> `page*`, then from that we go to the `folio*`, then we >> >> try to grab a reference to the `folio`, then if that worked >> >> we check that the `page` still points to the same `folio`, >> >> and then we recheck that the PTE is still the same.) >> >> """ >> > >> > Nngh. In trying to make this description fit all kernels (with >> > both pages and folios), you've complicated it maximally. Let's >> > try a more simple explanation: >> > >> > First we (locklessly) load the page table entry, then we grab a >> > reference to the folio that contains it (which can fail if the >> > refcount is zero, in that case we bail), then we recheck that the >> > page table entry is still the same, and if it changed in between, >> > we drop the folio reference and bail. >> > This can, again, grab a reference to a folio after it has >> > already been freed and reallocated. The reason why this is >> > fine is that the metadata structure that holds this refcount, >> > `struct folio` is never freed; even when a folio is >> > freed and reallocated, the corresponding `struct folio` >> > stays. > > Oh, thanks. You're right, trying to talk about kernels with folios > made it unnecessarily complicated... > >> I'm probably missing something obvious but how is that synchronised >> against memory hotplug? AFAICT if it isn't couldn't the pages be freed >> and memory removed? In that case the above would no longer hold because >> (I think) the metadata structure could have been freed. > > Hm... that's this codepath? > > arch_remove_memory -> __remove_pages -> __remove_section -> > sparse_remove_section -> section_deactivate -> > depopulate_section_memmap -> vmemmap_free -> remove_pagetable > which then walks down the page tables and ends up freeing individual > pages in remove_pte_table() using the confusingly-named > free_pagetable()? Right. section_deactivate() will also clear SECTION_HAS_MEM_MAP which would trigger VM_BUG_ON(!pfn_valid(pte_pfn(pte))) in gup_pte_range(). > I'm not sure what the synchronization against hotplug is - GUP-fast is > running with IRQs disabled, but other codepaths might not, like > get_ksm_page()? I don't know if that's holding something else for protection... I was thinking about this from the ZONE_DEVICE perspective (ie. memunmap_pages -> pageunmap_range -> arch_remove_memory -> ...). That runs with IRQs enabled, and I couldn't see any other synchronization. pageunmap_range() does call mem_hotplug_begin() which takes hotplug locks but GUP-fast doesn't take those locks. So based on Peter's response I think I need to add a rcu_synchronize() call to pageunmap_range() right after calling mem_hotplug_begin(). I could just add it to mem_hotplug_begin() but offline_pages() calls that too early (before pages have been isolated) so will need a separate change.
On Tue, Oct 25, 2022 at 5:06 PM Peter Zijlstra <peterz@infradead.org> wrote: > On Tue, Oct 25, 2022 at 04:18:20PM +0200, Jann Horn wrote: > > On Tue, Oct 25, 2022 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > On Mon, Oct 24, 2022 at 09:58:07PM +0200, Jann Horn wrote: > > > > > > > Unless I'm completely misunderstanding what's going on here, the whole > > > > "remove_table" thing only happens when you "remove a table", meaning > > > > you free an entire *pagetable*. Just zapping PTEs doesn't trigger that > > > > logic. > > > > > > Aah; yes true. OTOH even it that were not so, I think it would still be > > > broken because the current code relies on the TLB flush to have > > > completed, whereas the RCU scheme is effectively async and can be > > > considered pending until the callback runs. > > > > > > Hurmph... easiest fix is probably to dis-allow kvm_flush_tlb_multi() > > > for i386-pae builds. > > > > > > Something like so... nobody in his right mind should care about i386-pae > > > virt performance much. > > > > I think Xen and HyperV have similar codepaths. > > hyperv_flush_tlb_multi() looks like it uses remote flush hypercalls, > > xen_flush_tlb_multi() too. > > Sure (not updated). > > > On top of that, I think that theoretically, Linux doesn't even ensure > > that you have a TLB flush in between tearing down one PTE and > > installing another PTE (see > > https://lore.kernel.org/all/CAG48ez1Oz4tT-N2Y=Zs6jumu=zOp7SQRZ=V2c+b5bT9P4retJA@mail.gmail.com/), > > but I haven't tested that, and if it is true, I'm also not entirely > > sure if it's correct (in the sense that it only creates incoherent-TLB > > states when userspace is doing something stupid like racing > > MADV_DONTNEED and page faults on the same region). > > > > I think the more clearly correct fix would be to get rid of the split > > loads and use CMPXCHG16B instead (probably destroying the performance > > of GUP-fast completely), but that's complicated because some of the > > architectures that use the split loads path don't have cmpxchg_double > > (or at least don't have it wired up). > > cmpxchg8b; but no, I think we want to fix MADV_DONTNEED, incoherent TLB > states are a pain nobody needs. > > Something like so should force TLB flushes before dropping pte_lock (not > looked at the various pmd level things yet). [...] > #endif /* _LINUX_MM_H */ > diff --git a/mm/memory.c b/mm/memory.c > index f88c351aecd4..9bb63b3fbee1 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1440,6 +1440,11 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > tlb_remove_tlb_entry(tlb, pte, addr); > zap_install_uffd_wp_if_needed(vma, addr, pte, details, > ptent); > + > + if (!force_flush && !tlb->fullmm && details && > + details->zap_flags & ZAP_FLAG_FORCE_FLUSH) > + force_flush = 1; > + Hmm... I guess that might work, assuming that there is no other codepath we might race with that first turns the present PTE into a non-present PTE but keeps the flush queued for later. At least codepaths that use the tlb_batched infrastructure are unproblematic...
On Oct 25, 2022, at 6:06 PM, Peter Zijlstra <peterz@infradead.org> wrote: > if (!force_flush && !tlb->fullmm && details && > + details->zap_flags & ZAP_FLAG_FORCE_FLUSH) > + force_flush = 1; Isn’t it too big of a hammer? At the same time, the whole reasoning about TLB flushes is not getting any simpler. We had cases in which MADV_DONTNEED and another concurrent operation that effectively zapped PTEs (e.g., another MADV_DONTNEED) caused the zap_pte_range() to skip entries since pte_none() was true. To resolve these cases we relied on tlb_finish_mmu() to flush the range when needed (i.e., flush the whole range when mm_tlb_flush_nested()). Now, I do not have a specific broken scenario in mind following this change, but it is all sounds to me a bit dangerous and at same time can potentially introduce new overheads. One alternative may be using mm_tlb_flush_pending() when setting a new PTE to check for pending flushes and flushing the TLB if that is the case. This is somewhat similar to what ptep_clear_flush() does. Anyhow, I guess this might induce some overheads. As noted before, it is possible to track pending TLB flushes in VMA/page-table granularity, with different tradeoffs of overheads.
On Wed, Oct 26, 2022 at 06:45:16PM +0200, Jann Horn wrote: > > #endif /* _LINUX_MM_H */ > > diff --git a/mm/memory.c b/mm/memory.c > > index f88c351aecd4..9bb63b3fbee1 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -1440,6 +1440,11 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > > tlb_remove_tlb_entry(tlb, pte, addr); > > zap_install_uffd_wp_if_needed(vma, addr, pte, details, > > ptent); > > + > > + if (!force_flush && !tlb->fullmm && details && > > + details->zap_flags & ZAP_FLAG_FORCE_FLUSH) > > + force_flush = 1; > > + > > Hmm... I guess that might work, assuming that there is no other > codepath we might race with that first turns the present PTE into a > non-present PTE but keeps the flush queued for later. At least > codepaths that use the tlb_batched infrastructure are unproblematic... So I thought the general rule was that if you modify a PTE and have not unmapped things -- IOW, there's actual concurrency possible on the thing, then the TLB invalidate needs to happen under pte_lock, since that is what controls concurrency at the pte level. As it stands MADV_DONTNEED seems to blatatly violate that general rule. Then again; I could've missed something and the rules changed?
On Wed, Oct 26, 2022 at 10:43:21PM +0300, Nadav Amit wrote: > On Oct 25, 2022, at 6:06 PM, Peter Zijlstra <peterz@infradead.org> wrote: > > > if (!force_flush && !tlb->fullmm && details && > > + details->zap_flags & ZAP_FLAG_FORCE_FLUSH) > > + force_flush = 1; > > Isn’t it too big of a hammer? It is the obvious hammer :-) TLB invalidate under pte_lock when clearing. > At the same time, the whole reasoning about TLB flushes is not getting any > simpler. We had cases in which MADV_DONTNEED and another concurrent > operation that effectively zapped PTEs (e.g., another MADV_DONTNEED) caused > the zap_pte_range() to skip entries since pte_none() was true. To resolve > these cases we relied on tlb_finish_mmu() to flush the range when needed > (i.e., flush the whole range when mm_tlb_flush_nested()). Yeah, whoever thought that allowing concurrency there was a great idea :/ And I must admit to hating the pending thing with a passion. And that mm_tlb_flush_nested() thing in tlb_finish_mmu() is a giant hack at the best of times. Also; I feel it's part of the problem here; it violates the basic rules we've had for a very long time. > Now, I do not have a specific broken scenario in mind following this change, > but it is all sounds to me a bit dangerous and at same time can potentially > introduce new overheads. I'll take correctness over being fast. As you say, this whole TLB thing is getting out of hand. > One alternative may be using mm_tlb_flush_pending() when setting a new PTE > to check for pending flushes and flushing the TLB if that is the case. This > is somewhat similar to what ptep_clear_flush() does. Anyhow, I guess this > might induce some overheads. As noted before, it is possible to track > pending TLB flushes in VMA/page-table granularity, with different tradeoffs > of overheads. Right; I just don't believe in VMAs for this, they're *waaay* to big.
On Oct 27, 2022, at 12:27 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> One alternative may be using mm_tlb_flush_pending() when setting a new PTE >> to check for pending flushes and flushing the TLB if that is the case. This >> is somewhat similar to what ptep_clear_flush() does. Anyhow, I guess this >> might induce some overheads. As noted before, it is possible to track >> pending TLB flushes in VMA/page-table granularity, with different tradeoffs >> of overheads. > > Right; I just don't believe in VMAs for this, they're *waaay* to big. Well, I did it for VMA in an RFC only because I was pushed. I thought and do think that page-table granularity is the right one.
On Thu, Oct 27, 2022 at 12:08 AM Peter Zijlstra <peterz@infradead.org> wrote: > > So I thought the general rule was that if you modify a PTE and have not > unmapped things -- IOW, there's actual concurrency possible on the > thing, then the TLB invalidate needs to happen under pte_lock, since > that is what controls concurrency at the pte level. Yeah, I think we should really think about the TLB issue and make the rules very clear. A lot of our thinking behind "fix TLB races" have been about "use-after-free wrt TLB on another CPU", and the design in zap_page_ranges() is almost entirely about that: making sure that the TLB flush happens before the underlying pages are free'd. I say "almost entirely", because you can see the effects of the *other* kind of race in if (!PageAnon(page)) { if (pte_dirty(ptent)) { force_flush = 1; set_page_dirty(page); } where it isn't about the lifetime of the page, but about the state coherency wrt other users. But I think even that code is quite questionable, and we really should write down the rules much more strictly somewhere. For example, wrt that pte_dirty() causing force_flush, I'd love to have some core set of rules that would explain (a) why it is only relevant for shared pages (b) do even shared pages need it for the "fullmm" case when we tear down the whole VM? (c) what are the force_flush interactions wrt holding the mmap lock for writing vs reading? In the above, I think (b)/(c) are related - I suspect "fullmm" is mostly equivalent to "mmap held for writing", in that it guarantees that there should be no concurrent faults or other active sw ops on the table. But "fullmm" is probably even stronger than "mmap write-lock" in that it should also mean "no other CPU can be actively using this" - either for hardware page table walking, or for GUP. Both both have the vmscan code and rmap that can still get to the pages - and the vmscan code clearly only cares about the page table lock. But the vmscan code itself also shouldn't care about some stale TLB value, so ... > As it stands MADV_DONTNEED seems to blatatly violate that general rule. So let's talk about exactly what and why the TLB would need to be flushed before dropping the page table lock. For example, MADV_DONTNEED does this all with just the mmap lock held for reading, so we *unless* we have that 'force_flush', we can (a) have another CPU continue to use the old stale TLB entry for quite a while (b) yet another CPU (that didn't have a TLB entry, or wanted to write to a read-only one ) could take a page fault, and install a *new* PTE entry in the same slot, all at the same time. Now, that's clearly *very* confusing. But being confusing may not mean "wrong" - we're still delaying the free of the old entry, so there's no use-after-free. The biggest problem I can see is that this means that user space memory ordering might be screwed up: the CPU in (a) will see not just an old TLB entry, but simply old *data*, when the CPU in (b) may be writing to that same address with new data. So I think we clearly do *NOT* serialize as much as we could for MADV_DONTNEED, and I think the above is a real semantic result of that. But equally arguably if you do this kind of unserialized MADV_DONTNEED in user space, that's a case of "you asked for the breakage - you get to keep both pieces". So is that ever an actual problem? If the worst issue is that some CPU's may see old ("pre-DONTNEED") data, while other CPU's then see new data while the MADV_DONTNEED is executing, I think maybe *THAT* part is fine. But because it's so very confusing, maybe we have other problems in this area, which is why I think it would be really good to have an actual set of real hard documented rules about this all, and exactly when we need to flush TLBs synchronously with the page table lock, and when we do not. Anybody willing to try to write up the rules (and have each rule document *why* it's a rule - not just "by fiat", but an actual "these are the rules and this is *why* they are the rules"). Because right now I think all of our rules are almost entirely just encoded in the code, with a couple of comments, and a few people who just remember why we do what we do. Linus
Just two quick remarks; it's far to late to really think :-) On Thu, Oct 27, 2022 at 11:13:55AM -0700, Linus Torvalds wrote: > But "fullmm" is probably even stronger than "mmap write-lock" in that > it should also mean "no other CPU can be actively using this" - either > for hardware page table walking, or for GUP. IIRC fullmm is really: this is the last user and we're taking the whole mm down -- IOW exit(). > For example, MADV_DONTNEED does this all with just the mmap lock held > for reading, so we *unless* we have that 'force_flush', we can > > (a) have another CPU continue to use the old stale TLB entry for quite a while > > (b) yet another CPU (that didn't have a TLB entry, or wanted to write > to a read-only one ) could take a page fault, and install a *new* PTE > entry in the same slot, all at the same time. > > Now, that's clearly *very* confusing. But being confusing may not mean > "wrong" - we're still delaying the free of the old entry, so there's > no use-after-free. Do we worry about CPU errata where things go side-ways if multiple CPUs have inconsistent TLB state?
On Thu, Oct 27, 2022 at 12:35 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Oct 27, 2022 at 11:13:55AM -0700, Linus Torvalds wrote: > > > But "fullmm" is probably even stronger than "mmap write-lock" in that > > it should also mean "no other CPU can be actively using this" - either > > for hardware page table walking, or for GUP. > > IIRC fullmm is really: this is the last user and we're taking the whole > mm down -- IOW exit(). Yes. But that doesn't mean that it's entirely "just ours" - vmscan can still see the entries due to rmap, I think. So there can still be some concurrency concerns, but it's limited. > Do we worry about CPU errata where things go side-ways if multiple CPUs > have inconsistent TLB state? Yeah, we should definitely worry about those, since I think they have been known to cause machine checks etc, which then crashes the machine because the machine check architecture is broken garbage. "User gets the odd memory ordering they asked for" is different from "user can crash machine because of bad machine check architecture" ;) That said, I don't think this is a real worry here. Because if I recall the errata correctly, they are not about "different TLB contents", but "different cacheability for the same physical page". Because different TLB contents are normal and even expected, I think. Things like kmap_local etc already end up doing some lazy TLB flushing. No? I think it's only "somebody did an UC access to a cacheline I have" that ends up being bad. Note the *WILD HANDWAVING* above - I didn't actually look up the errata. The above is from my dim memories of the issues we had, and I might just be wrong. Linus
On Oct 27, 2022, at 11:13 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > Anybody willing to try to write up the rules (and have each rule > document *why* it's a rule - not just "by fiat", but an actual "these > are the rules and this is *why* they are the rules"). > > Because right now I think all of our rules are almost entirely just > encoded in the code, with a couple of comments, and a few people who > just remember why we do what we do. I think it might be easier to come up with new rules instead of phrasing the existing ones. The approach I suggested before [1] is something like: 1. Turn x86’s TLB-generation mechanism to be generic. Turn the TLB-generation into “pending TLB-generation”. 2. For each mm track “completed TLB-generation”, whenever an actual flush takes place. 3. When you defer a TLB-flush, while holding the PTL: a. Increase the TLB-generation. b. Save the updated “table generation" in a new field in the page-table’s page-struct. 4. When you are about to rely on a PTE value that is read from a page-table, first check if a TLB flush is needed. The check is performed by comparing the “table generation” with the “completed generation”. If the “table generation” is behind, a TLB flush is needed. [ You rely on the PTE value when you install new PTEs or change them ] That’s about it. I might have not covered some issues with fast-GUP. But in general I think it is a simple scheme. The thing I like about this scheme the most is that it avoids relying on almost all the OS data-structures (e.g., PageAnon()), making it much easier to grasp. I can revive the patch-set if the overall approach is agreeable. [1] https://lore.kernel.org/lkml/20210131001132.3368247-1-namit@vmware.com/
On Thu, Oct 27, 2022 at 1:15 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > I think it might be easier to come up with new rules instead of phrasing the > existing ones. I'm ok with that, but I think you are missing a very important issue: all the cases where we can short-circuit TLB invalidations *entirely*. You don't mention those at all. Those optimizations are *very* important. Process exit is one of the most performance-critical pieces of code in the kernel on some loads, because a lot of traditional unix loads have a *ton* of small fork/exec/exit sequences, and the whole "do just one TLB flush" was at least historically quite a big deal. So one very big issue here is when zap_page_tables() can end up skipping TLB flushes entirely, because nobody cares. And no, the fix is not to turn it into some "just increment a generation number". We want to avoid *even that* cost for the whole "we don't actually need a TLB flush at all, until we actually free the pages". So there are two levels of tlb flush optimizations (a) avoiding them entirely in the first place (b) the whole "once you have to flush, keep track of lazy modes and TLB generations, and flush ranges" And honestly, I think you ignored (a), and that's where we do exactly those kinds of "this case doesn't need to flush AT ALL" things. So when you say > The thing I like about this scheme > the most is that it avoids relying on almost all the OS data-structures > (e.g., PageAnon()), making it much easier to grasp. I think it's because you've ignored a big part of the whole issue. Linus
On Oct 27, 2022, at 1:31 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > So there are two levels of tlb flush optimizations > > (a) avoiding them entirely in the first place > > (b) the whole "once you have to flush, keep track of lazy modes and > TLB generations, and flush ranges" > > And honestly, I think you ignored (a), and that's where we do exactly > those kinds of "this case doesn't need to flush AT ALL" things. I did try to avoid TLB flushes by introducing pte_needs_flush() and avoiding flushes based on the architectural PTE changes. There are even more x86 arch-based opportunities to further avoid TLB flushes (and then only flush the TLB if spurious #PF occurs). Personally, I still think that making decisions on flushes based on (mostly) only the arch state makes the code more robust against misuse (e.g., see various confusions between mmu_gather’s fullmm and need_flush_all). Having said that, I will follow your feedback that the extra complexity worth the extra performance. Anyhow, admittedly, I need to give it more thought. For instance, in respect to the code that you mentioned (in zap_pte_range), after reading it again, it seems strange: how is ok to defer the TLB flush after the rmap was removed, even if it is done while the PTL is held. folio_clear_dirty_for_io() would not sync on the PTL afterwards, so the page might be later re-dirtied using a stale cached PTE. Oh well.
On Oct 27, 2022, at 2:44 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > Anyhow, admittedly, I need to give it more thought. For instance, in respect > to the code that you mentioned (in zap_pte_range), after reading it again, > it seems strange: how is ok to defer the TLB flush after the rmap was > removed, even if it is done while the PTL is held. > folio_clear_dirty_for_io() would not sync on the PTL afterwards, so the page > might be later re-dirtied using a stale cached PTE. Oh well. Peter, So it appears to be a problem - flushing after removing the rmap. I attach a PoC that shows the problem. The problem is in the following code of zap_pte_range(): if (!PageAnon(page)) { if (pte_dirty(ptent)) { force_flush = 1; set_page_dirty(page); } … } page_remove_rmap(page, vma, false); Once we remove the rmap, rmap_walk() would not acquire the page-table lock anymore. As a result, nothing prevents the kernel from performing writeback and cleaning the page-struct dirty-bit before the TLB is actually flushed. As a result, if there is an additional thread that has the dirty-PTE cached in the TLB, it can keep writing to the page and nothing (PTE/page-struct) will keep track that the page has been dirtied. In other words, writes to the memory mapped file after munmap()/MADV_DONTNEED started can be lost. This affects both munmap() and MADV_DONTNEED. One might argue that if you don’t keep writing after munmap()/MADV_DONTNEED it’s your fault (questionable), but if that’s the case why do we bother with force_flush at all? If we want it to behave correctly - i.e., writes after munmap/MADV_DONTNEED to propagate to the file - we need to collect dirty pages and remove their rmap only after we flush the TLB (and before we release the ptl). mmu_gather would probably need to be changed for this matter. Thoughts? --- Some details about the PoC (below): We got 3 threads that use 2MB range: 1. Maintains a counter in the first 4KB of the 2MB range and checks it actually updates the memory. 2. Dirties pages in 2MB range (just to make the race more likely). 3. Either (i) maps the same mapping at the first 4KB (to test munmap indirectly); or (ii) runs MADV_DONTNEED on the first 4KB. In addition, a child process runs msync and fdatasync to writeback the first 4KB. The PoC should be run with a file on a block RAM device. It manages to trigger the issue relatively reliably (within 60 seconds) with munmap() and slightly less reliably with MADV_DONTNEED. I have no idea if it works in VM, deep C-states, etc.. -- >8 -- #define _GNU_SOURCE #include <signal.h> #include <stdio.h> #include <stdlib.h> #include <pthread.h> #include <sys/mman.h> #include <sys/stat.h> #include <fcntl.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <time.h> #define handle_error(msg) \ do { perror(msg); exit(EXIT_FAILURE); } while (0) void *p; volatile bool stop = false; pid_t flusher_pid; int fd; #define PAGE_SIZE (4096ul) #define PAGES_PER_PMD (512) #define HPAGE_SIZE (PAGE_SIZE * PAGES_PER_PMD) // Comment MUNMAP_TEST for MADV_DONTNEED test #define MUNMAP_TEST void *dirtying_thread(void *arg) { int i; while (!stop) { for (i = 1; i < PAGES_PER_PMD; i++) { *(volatile char *)(p + (i * PAGE_SIZE) + 64) = 5; } } return NULL; } void *checking_thread(void *arg) { volatile unsigned long *ul_p = (volatile unsigned long*)p; unsigned long cnt = 0; while (!stop) { *ul_p = cnt; if (*ul_p != cnt) { printf("FAILED: expected %ld, got %ld\n", cnt, *ul_p); kill(flusher_pid, SIGTERM); exit(0); } cnt++; } return NULL; } void *remap_thread(void *arg) { void *ptr; struct timespec t = { .tv_nsec = 10000, }; while (!stop) { #ifdef MUNMAP_TEST ptr = mmap(p, PAGE_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED|MAP_POPULATE, fd, 0); if (ptr == MAP_FAILED) handle_error("remap_thread"); #else if (madvise(p, PAGE_SIZE, MADV_DONTNEED) < 0) handle_error("MADV_DONTNEED"); nanosleep(&t, NULL); #endif } return NULL; } void flushing_process(void) { // Remove the pages to speed up rmap_walk and allow to drop caches. if (madvise(p, HPAGE_SIZE, MADV_DONTNEED) < 0) handle_error("MADV_DONTNEED"); while (true) { if (msync(p, PAGE_SIZE, MS_SYNC)) handle_error("msync"); if (posix_fadvise(fd, 0, PAGE_SIZE, POSIX_FADV_DONTNEED)) handle_error("posix_fadvise"); } } int main(int argc, char *argv[]) { void *(*thread_funcs[])(void*) = { &dirtying_thread, &checking_thread, &remap_thread, }; int r, i; int rc1, rc2; unsigned long addr; void *ptr; char *page = malloc(PAGE_SIZE); int n_threads = sizeof(thread_funcs) / sizeof(*thread_funcs); pthread_t *threads = malloc(sizeof(pthread_t) * n_threads); pid_t pid; if (argc < 2) { fprintf(stderr, "usages: %s [filename]\n", argv[0]); exit(EXIT_FAILURE); } fd = open(argv[1], O_RDWR|O_CREAT, 0666); if (fd == -1) handle_error("open fd"); for (i = 0; i < PAGES_PER_PMD; i++) { if (write(fd, page, PAGE_SIZE) != PAGE_SIZE) handle_error("write"); } free(page); ptr = mmap(NULL, HPAGE_SIZE * 2, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0); if (ptr == MAP_FAILED) handle_error("mmap anon"); addr = (unsigned long)(ptr + HPAGE_SIZE - 1) & ~(HPAGE_SIZE - 1); printf("starting...\n"); ptr = mmap((void *)addr, HPAGE_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED|MAP_POPULATE, fd, 0); if (ptr == MAP_FAILED) handle_error("mmap file - start"); p = ptr; for (i = 0; i < n_threads; i++) { r = pthread_create(&threads[i], NULL, thread_funcs[i], NULL); if (r) handle_error("pthread_create"); } // Run the flushing process in a different process, so msync() would // not require mmap_lock. pid = fork(); if (pid == 0) flushing_process(); flusher_pid = pid; sleep(60); stop = true; for (i = 0; i < n_threads; i++) pthread_join(threads[i], NULL); kill(flusher_pid, SIGTERM); printf("Finished without an error"); exit(0); }
On Fri, Oct 28, 2022 at 4:57 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > The problem is in the following code of zap_pte_range(): > > if (!PageAnon(page)) { > if (pte_dirty(ptent)) { > force_flush = 1; > set_page_dirty(page); > } > … > } > page_remove_rmap(page, vma, false); > > Once we remove the rmap, rmap_walk() would not acquire the page-table lock > anymore. As a result, nothing prevents the kernel from performing writeback > and cleaning the page-struct dirty-bit before the TLB is actually flushed. Hah. The original reason for force_flush there was similar, with a race wrt page_mkclean() because this code doesn't take the page lock that would normally serialize things, because the page lock is so painful and ends up having nasty nasty interactions with slow IO operations. So all the page-dirty handling really *wants* to use the page lock, and for the IO side (writeback) that ends up being acceptable and works well, but from that "serialize VM state" it's horrendous. So instead the code intentionally serialized on the rmap data structures which page_mkclean() also walks, and as you point out, that's broken. It's not broken at the point where we do set_page_dirty(), but it *comes* broken when we drop the rmap, and the problem is exactly that "we still have the dirty bit hidden in the TLB state" issue that you pinpoint. I think the proper fix (or at least _a_ proper fix) would be to actually carry the dirty bit along to the __tlb_remove_page() point, and actually treat it exactly the same way as the page pointer itself - set the page dirty after the TLB flush, the same way we can free the page after the TLB flush. We could easiy hide said dirty bit in the low bits of the "batch->pages[]" array or something like that. We'd just have to add the 'dirty' argument to __tlb_remove_page_size() and friends. Hmm? Your idea of "do the page_remove_rmap() late instead" would also work, but the reason I think just squirrelling away the dirty bit is the "proper" fix is that it would get rid of the whole need for 'force_flush' in this area entirely. So we'd not only fix that race you noticed, we'd actually do so and reduce the number of TLB flushes too. I don't know. Maybe I'm missing something fundamental, and my idea is just stupid. Linus
On Oct 28, 2022, at 5:42 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > I think the proper fix (or at least _a_ proper fix) would be to > actually carry the dirty bit along to the __tlb_remove_page() point, > and actually treat it exactly the same way as the page pointer itself > - set the page dirty after the TLB flush, the same way we can free the > page after the TLB flush. > > We could easiy hide said dirty bit in the low bits of the > "batch->pages[]" array or something like that. We'd just have to add > the 'dirty' argument to __tlb_remove_page_size() and friends. Thank you for your quick response. I was slow to respond due to a jet lag. Anyhow, I am not sure whether the solution that you propose would work. Please let me know if my understanding makes sense. Let’s assume that we do not call set_page_dirty() before we remove the rmap but only after we invalidate the page [*]. Let’s assume that shrink_page_list() is called after the page’s rmap is removed and the page is no longer mapped, but before set_page_dirty() was actually called. In such a case, shrink_page_list() would consider the page clean, and would indeed keep the page (since __remove_mapping() would find elevated page refcount), which appears to give us a chance to mark the page as dirty later. However, IIUC, in this case shrink_page_list() might still call filemap_release_folio() and release the buffers, so calling set_page_dirty() afterwards - after the actual TLB invalidation took place - would fail. > Your idea of "do the page_remove_rmap() late instead" would also work, > but the reason I think just squirrelling away the dirty bit is the > "proper" fix is that it would get rid of the whole need for > 'force_flush' in this area entirely. So we'd not only fix that race > you noticed, we'd actually do so and reduce the number of TLB flushes > too. I’m all for reducing the number of TLB flushes, and your solution does sound better in general. I proposed something that I considered having the path of least resistance (i.e., least chance of breaking something). I can do what you propsosed, but I am not sure how to deal with the buffers being removed. One more note: This issue, I think, also affects migrate_vma_collect_pmd(). Alistair recently addressed an issue there, but in my prior feedback to him I missed this issue. [*] Note that this would be for our scenario pretty much the same if we also called set_page_dirty() before removing the rmap, but the page was cleaned while the TLB invalidation has still not been performed.
On Sat, Oct 29, 2022 at 11:05 AM Nadav Amit <nadav.amit@gmail.com> wrote: > > Anyhow, I am not sure whether the solution that you propose would work. > Please let me know if my understanding makes sense. Let me include my (UNTESTED! PROBABLY GARBAGE!) patch here just as a "I meant something like this". Note that it's untested, but I tried to make it intentionally use different names and an opaque type in the 'mmu_gather' data structure so that at least these bit games end up being type-safe and more likely to be correct if it compiles. And I will say that it does compile for me - but only in my normal x86-64 config. I haven't tested anything else, and I guarantee it won't build on s390, for example, because s390 has its own mmu_gather functions. > Let’s assume that we do not call set_page_dirty() before we remove the rmap > but only after we invalidate the page [*]. Let’s assume that > shrink_page_list() is called after the page’s rmap is removed and the page > is no longer mapped, but before set_page_dirty() was actually called. > > In such a case, shrink_page_list() would consider the page clean, and would > indeed keep the page (since __remove_mapping() would find elevated page > refcount), which appears to give us a chance to mark the page as dirty > later. Right. That is not different to any other function (like "write()" having looked up the page. > However, IIUC, in this case shrink_page_list() might still call > filemap_release_folio() and release the buffers, so calling set_page_dirty() > afterwards - after the actual TLB invalidation took place - would fail. I'm not seeing why. That would imply that any "look up page, do set_page_dirty()" is broken. They don't have rmap either. And we have a number of them all over (eg think "GUP users" etc). But hey, you were right about the stale TLB case, so I may just be missing something. I *think* the important thing is just that we need to mark the page dirtty from the page tables _after_ we've flushed the TLB, just to make sure that there can be no subsequent dirtiers that then get lost. Anyway, I think the best documentation for "this is what I meant" is simply the patch. Does this affect your PoC on your setup? I tried to run your program on my machine (WITHOUT this patch - I have compiled this patch, but I haven't even booted it - it really could be horrible broken). But it doesn't fail for me even without the patch and I just get "Finished without an error" over and over again - but you said it had to be run on a RAM block device, which I didn't do, so my testing is very suspect,. Again: THIS PATCH IS UNTESTED. I feel like it might actually work, if only because I tried to be so careful with the type system. But that "might actually work" is probably me being wildly optimistic, and also depends on the whole concept being ok in the first place. So realistically, think of this patch more as a "document in code what Linus meant with his incoherent ramblings" rather than anything else. Hmm? Linus
On Sat, Oct 29, 2022 at 11:36 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Anyway, I think the best documentation for "this is what I meant" is > simply the patch. Does this affect your PoC on your setup? Here's a slightly cleaned up set with preliminary commit messages, and an explanation for why some of the 'struct page' declarations were moved around a bit in case you wondered about that part of the change in the full patch. The end result should be the same, so if you already looked at the previous unified patch, never mind. But this one tries to make for a better patch series. Still not tested in any way, shape, or form. I decided I wanted to send this one before booting into this and possibly blowing up ;^) Linus
On Sat, Oct 29, 2022 at 11:58 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Still not tested in any way, shape, or form. I decided I wanted to > send this one before booting into this and possibly blowing up ;^) Well, it boots, and I see no difference with your PoC code. It didn't fail for me before, it doesn't fail for me with those patches. Again, the "it doesn't fail for me" is probably because I'm running it incorrectly, although for all I know there can also be hardware differences. I'm testing on an older AMD threadripper, and as I'm sure you are very aware, some AMD cores used to have special support for keeping the TLB coherent with the actual page table contents in order to then avoid TLB flushes entirely. Those things ended up being buggy and disabled, but my point is that hardware differences can obviously actively hide this issue by making the TLB contents track page table changes. So even if I were to run it the same way you do, I might not see the failure due to just running it on different hardware with different TLB and timing. Anyway, the patches don't seem to cause any *obvious* problems. That's not to say that they are correct, or that they fix anything, but it's certainly a fairly simple and straightforward patch, and it "feels right" to me. Sadly, reality doesn't always agree with my feelings. Damn. Linus
On Oct 29, 2022, at 12:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Oct 29, 2022 at 11:58 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> Still not tested in any way, shape, or form. I decided I wanted to >> send this one before booting into this and possibly blowing up ;^) > > Well, it boots, and I see no difference with your PoC code. > > It didn't fail for me before, it doesn't fail for me with those patches. > > Again, the "it doesn't fail for me" is probably because I'm running it > incorrectly, although for all I know there can also be hardware > differences. Please give me some time to test it. I presume you ran it with block ram device (not tmpfs) and not on a virtual machine (which can affect besides Intel/AMD implementation differences). But even if your patches work and the tests pass, I am not sure it means that everything is fine. I did not try to trigger a race with shrink_page_list(), and doing that might be harder than the race I tried to create before. I need to do some tracing to understand what I was missing in my understanding of the shrink_page_list() - assuming that I am mistaken about the buffers being potentially released. I would note that my concern about releasing the buffers is partially driven by to issues that were reported before [1]. I am actually not sure how they were resolved. [1] https://lore.kernel.org/all/20180103100430.GE4911@quack2.suse.cz/
On 10/29/22 11:36, Linus Torvalds wrote: >> In such a case, shrink_page_list() would consider the page clean, and would >> indeed keep the page (since __remove_mapping() would find elevated page >> refcount), which appears to give us a chance to mark the page as dirty >> later. > > Right. That is not different to any other function (like "write()" > having looked up the page. > >> However, IIUC, in this case shrink_page_list() might still call >> filemap_release_folio() and release the buffers, so calling set_page_dirty() >> afterwards - after the actual TLB invalidation took place - would fail. > > I'm not seeing why. > > That would imply that any "look up page, do set_page_dirty()" is > broken. They don't have rmap either. And we have a number of them all > over (eg think "GUP users" etc). Yes, we do have a bunch of "look up page, do set_page_dirty()" cases. And I think that many (most?) of them are in fact broken! Because: the dirtiness of a page is something that the filesystem believes that it is managing, and so filesystem coordination is, in general, required in order to mark a page as dirty. Jan Kara's 2018 analysis [1] (which launched the pin_user_pages() effort) shows a nice clear example. And since then, I've come to believe that most of the gup/pup call sites have it wrong: a) pin_user_pages() b) /* access page contents */ c) set_page_dirty() or set_page_dirty_lock() // PROBLEM HERE d) unpin_user_page() ext4 has since papered over the problem, by soldiering on if it finds a page without writeback buffers when it expected to be able to writeback a dirty page. But you get the idea. And I think that applies beyond the gup/pup situation. [1] https://lore.kernel.org/linux-mm/20180103100430.GE4911@quack2.suse.cz/T/#u thanks,
On Sat, Oct 29, 2022 at 12:39 PM John Hubbard <jhubbard@nvidia.com> wrote: > > ext4 has since papered over the problem, by soldiering on if it finds a > page without writeback buffers when it expected to be able to writeback > a dirty page. But you get the idea. I suspect that "soldiering on" is the right thing to do, but yes, our 'mkdirty' vs 'mkclean' thing has always been problematic. I think we always needed a page lock for it, but PG_lock itself doesn't work (as mentioned earlier) because the VM can't serialize with IO, and needs the lock to basically be a spinlock. The page table lock kind of took its place, and then the rmap removal makes for problems (since it is what basically ends up being the shared place to look it upo). I can think of three options: (a) filesystems just deal with it (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too (c) we could actually add a spinlock (hashed on the page?) for this I think (a) is basically our current expectation. And (b) would be fairly easy - same model as that dirty bit patch, just a 'do page_remove_rmap too' - except page_remove_rmap() wants the vma as well (and we delay the TLB flush over multiple vma's, so it's not just a "save vma in mmu_gather"). Doing (c) doesn't look hard, except for the "new lock" thing, which is always a potential huge disaster. If it's only across set_page_dirty() and page_mkclean(), though, and uses some simple page-based hash, it sounds fairly benign. Linus Linus
On Sat, Oct 29, 2022 at 1:15 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I can think of three options: > > (a) filesystems just deal with it > > (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too > > (c) we could actually add a spinlock (hashed on the page?) for this > > I think (a) is basically our current expectation. Side note: anybody doing gup + set_page_dirty() won't be fixed by b/c anyway, so I think (a) is basically the only thing. And that's true even if you do a page pinning gup, since the source of the gup may be actively unmapped after the gup. So a filesystem that thinks that only write, or a rmap-accessible mmap can turn the page dirty really seems to be fundamentally broken. And I think that has always been the case, it's just that filesystem writers may not have been happy with it, and may not have had test-cases for it. It's not surprising that the filesystem people then try to blame users. Linus
On 10/29/22 13:30, Linus Torvalds wrote: >> I can think of three options: >> >> (a) filesystems just deal with it >> >> (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too >> >> (c) we could actually add a spinlock (hashed on the page?) for this >> >> I think (a) is basically our current expectation. > > Side note: anybody doing gup + set_page_dirty() won't be fixed by b/c > anyway, so I think (a) is basically the only thing. > > And that's true even if you do a page pinning gup, since the source of > the gup may be actively unmapped after the gup. I was just now writing a response that favored (c) over (b), precisely because of that, yes. :) > > So a filesystem that thinks that only write, or a rmap-accessible mmap > can turn the page dirty really seems to be fundamentally broken. > > And I think that has always been the case, it's just that filesystem > writers may not have been happy with it, and may not have had > test-cases for it. > > It's not surprising that the filesystem people then try to blame users. > > Linus Yes, lots of unhappy debates about this over the years. However, I remain intrigued by (c), because if we had a "dirty page lock" that is looked up by page (much like looking up the ptl), it seems like a building block that would potentially help solve the whole thing. The above points about "file system needs to coordinate with mm about what's allowed to be dirtied, including gup/dma cases", those are still true and not yet solved, yes. But having a solid point of synchronization for this, definitely looks interesting. Of course, without working through this more thoroughly, it's not fair to impose this constraint on the current discussion, understood. :) thanks,
On Oct 29, 2022, at 1:15 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too > > And (b) would be fairly easy - same model as that dirty bit patch, > just a 'do page_remove_rmap too' - except page_remove_rmap() wants the > vma as well (and we delay the TLB flush over multiple vma's, so it's > not just a "save vma in mmu_gather”). (b) sounds reasonable and may potentially allow future performance improvements (batching, doing stuff without locks). It does appear to break a potential hidden assumption that rmap is removed while the ptl is acquired (at least in the several instances I samples). Yet, anyhow page_vma_mapped_walk() checks the PTE before calling the function, so it should be fine. I’ll give it a try.
On Sat, Oct 29, 2022 at 01:15:26PM -0700, Linus Torvalds wrote: > On Sat, Oct 29, 2022 at 12:39 PM John Hubbard <jhubbard@nvidia.com> wrote: > > > > ext4 has since papered over the problem, by soldiering on if it finds a > > page without writeback buffers when it expected to be able to writeback > > a dirty page. But you get the idea. > > I suspect that "soldiering on" is the right thing to do, but yes, our > 'mkdirty' vs 'mkclean' thing has always been problematic. > > ... > > (a) filesystems just deal with it It should be noted that "soldiering on" just means that the kernel will not crash or BUG. It may mean that the dirty page will not gotten written back (since at the time when it is discovered we are in a context we may not allocate memory or block if there is a need to allocate blocks if the file system uses delayed allocation). Furthermore, since the file system does not know that one or more pages have dirtied behind it's back, if the file system is almost full, some writes may silently fail --- including writes where the usesrspace application was implicitly promised that the write would succeed by having the write(2) system call return without errors. If people are OK with that, it's fine. Just don't complain to the file system maintainers. :-) - Ted P.S. The reason why this isn't an utter disaster is because normally users of remote RMA use preallocated and pre-written/initialized files. And there aren't _that_ many other users of gup. So long as this remains the case, we might be happy to let sleeping canines lie. Just please dear $DEITY, let's not have any additional users of gup until we have a better solution.
On Oct 29, 2022, at 1:56 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > On Oct 29, 2022, at 1:15 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too >> >> And (b) would be fairly easy - same model as that dirty bit patch, >> just a 'do page_remove_rmap too' - except page_remove_rmap() wants the >> vma as well (and we delay the TLB flush over multiple vma's, so it's >> not just a "save vma in mmu_gather”). > > (b) sounds reasonable and may potentially allow future performance > improvements (batching, doing stuff without locks). > > It does appear to break a potential hidden assumption that rmap is removed > while the ptl is acquired (at least in the several instances I samples). > Yet, anyhow page_vma_mapped_walk() checks the PTE before calling the > function, so it should be fine. > > I’ll give it a try. I have just seen John’s and your emails. It seems (b) fell off. (a) is out of my “zone”, and anyhow assuming it would not be solved soon, deferring page_remove_rmap() might cause regressions. (c) might be more intrusive and potentially induce overheads. If we need a small backportable solution, I think the approach that I proposed (marking the page dirty after the invalidation, before the PTL is released) is the simplest one. Please advise how to proceed.
On Sat, Oct 29, 2022 at 1:56 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > On Oct 29, 2022, at 1:15 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > (b) we could move the "page_remove_rmap()" into the "flush-and-free" path too > > > > And (b) would be fairly easy - same model as that dirty bit patch, > > just a 'do page_remove_rmap too' - except page_remove_rmap() wants the > > vma as well (and we delay the TLB flush over multiple vma's, so it's > > not just a "save vma in mmu_gather”). > > (b) sounds reasonable and may potentially allow future performance > improvements (batching, doing stuff without locks). So the thing is, I think (b) makes sense from a TLB flush standpoint, but as mentioned, if filesystems _really_ want to feel in control, none of these locks fundamentally help, because the whole "gup + set_page_dirty()" situation still exists. And that fundamentally does not hold any locks between the lookup and the set_page_dirty(), and fundamentally doesn't stop an rmap from happening in between, so while the zap_page_range() and dirty TLB case case be dealt with that way, you don't actually fix any fundamental issues. Now, neither does my (c) case on its own, but as John Hubbard alluded to, *if* we had some sane serialization for a struct page, maybe that could then be used to at least avoid the issue with "rmap no longer exists", and make the filesystem handling case a bit better. IOW, I really do think that not only is (a) the current solution, it's the *correct* solution. But it is possible that (c) could then be used as a way to make (a) more palatable to filesystems, in that at least then there would be some way to serialize with "set_page_dirty()". But (b) cannot be used for that - because GUP fundamentally breaks that rmap association. It's all nasty. Linus
On Oct 29, 2022, at 12:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> It didn't fail for me before, it doesn't fail for me with those patches.
For the record, I tried to run the PoC on another machine, and it indeed did
not fail.
Turns out I had a small bug in one of the mechanisms that were intended to
make the failure more likely (I should have mapped again or madvised
HPAGE_SIZE to increase the time zap_pte_range spends to increase the
probability of the race).
I am still trying to figure out how to address this issue, and whether the
fact that some rmap_walk(), which do not use PVMW_SYNC are of an issue.
---
#define _GNU_SOURCE
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <time.h>
#define handle_error(msg) \
do { perror(msg); exit(EXIT_FAILURE); } while (0)
void *p;
volatile bool stop = false;
pid_t flusher_pid;
int fd;
#define PAGE_SIZE (4096ul)
#define PAGES_PER_PMD (512)
#define HPAGE_SIZE (PAGE_SIZE * PAGES_PER_PMD)
// Comment MUNMAP_TEST for MADV_DONTNEED test
#define MUNMAP_TEST
void *dirtying_thread(void *arg)
{
int i;
while (!stop) {
for (i = 1; i < PAGES_PER_PMD; i++) {
*(volatile char *)(p + (i * PAGE_SIZE) + 64) = 5;
}
}
return NULL;
}
void *checking_thread(void *arg)
{
volatile unsigned long *ul_p = (volatile unsigned long*)p;
unsigned long cnt = 0;
while (!stop) {
*ul_p = cnt;
if (*ul_p != cnt) {
printf("FAILED: expected %ld, got %ld\n", cnt, *ul_p);
kill(flusher_pid, SIGTERM);
exit(0);
}
cnt++;
}
return NULL;
}
void *remap_thread(void *arg)
{
void *ptr;
struct timespec t = {
.tv_nsec = 10000,
};
while (!stop) {
#ifdef MUNMAP_TEST
ptr = mmap(p, HPAGE_SIZE, PROT_READ|PROT_WRITE,
MAP_SHARED|MAP_FIXED|MAP_POPULATE, fd, 0);
if (ptr == MAP_FAILED)
handle_error("remap_thread");
#else
if (madvise(p, HPAGE_SIZE, MADV_DONTNEED) < 0)
handle_error("MADV_DONTNEED");
nanosleep(&t, NULL);
#endif
}
return NULL;
}
void flushing_process(void)
{
// Remove the pages to speed up rmap_walk and allow to drop caches.
if (madvise(p, HPAGE_SIZE, MADV_DONTNEED) < 0)
handle_error("MADV_DONTNEED");
while (true) {
if (msync(p, PAGE_SIZE, MS_SYNC))
handle_error("msync");
if (posix_fadvise(fd, 0, PAGE_SIZE, POSIX_FADV_DONTNEED))
handle_error("posix_fadvise");
}
}
int main(int argc, char *argv[])
{
void *(*thread_funcs[])(void*) = {
&dirtying_thread,
&checking_thread,
&remap_thread,
};
int r, i;
int rc1, rc2;
unsigned long addr;
void *ptr;
char *page = malloc(PAGE_SIZE);
int n_threads = sizeof(thread_funcs) / sizeof(*thread_funcs);
pthread_t *threads = malloc(sizeof(pthread_t) * n_threads);
pid_t pid;
if (argc < 2) {
fprintf(stderr, "usages: %s [filename]\n", argv[0]);
exit(EXIT_FAILURE);
}
fd = open(argv[1], O_RDWR|O_CREAT, 0666);
if (fd == -1)
handle_error("open fd");
for (i = 0; i < PAGES_PER_PMD; i++) {
if (write(fd, page, PAGE_SIZE) != PAGE_SIZE)
handle_error("write");
}
free(page);
ptr = mmap(NULL, HPAGE_SIZE * 2, PROT_NONE, MAP_PRIVATE|MAP_ANON,
-1, 0);
if (ptr == MAP_FAILED)
handle_error("mmap anon");
addr = (unsigned long)(ptr + HPAGE_SIZE - 1) & ~(HPAGE_SIZE - 1);
printf("starting...\n");
ptr = mmap((void *)addr, HPAGE_SIZE, PROT_READ|PROT_WRITE,
MAP_SHARED|MAP_FIXED|MAP_POPULATE, fd, 0);
if (ptr == MAP_FAILED)
handle_error("mmap file - start");
p = ptr;
for (i = 0; i < n_threads; i++) {
r = pthread_create(&threads[i], NULL, thread_funcs[i], NULL);
if (r)
handle_error("pthread_create");
}
// Run the flushing process in a different process, so msync() would
// not require mmap_lock.
pid = fork();
if (pid == 0)
flushing_process();
flusher_pid = pid;
sleep(60);
stop = true;
for (i = 0; i < n_threads; i++)
pthread_join(threads[i], NULL);
kill(flusher_pid, SIGTERM);
printf("Finished without an error\n");
exit(0);
}
On Oct 29, 2022, at 11:58 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Oct 29, 2022 at 11:36 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> Anyway, I think the best documentation for "this is what I meant" is >> simply the patch. Does this affect your PoC on your setup? > > Here's a slightly cleaned up set with preliminary commit messages, and > an explanation for why some of the 'struct page' declarations were > moved around a bit in case you wondered about that part of the change > in the full patch. > > The end result should be the same, so if you already looked at the > previous unified patch, never mind. But this one tries to make for a > better patch series. > > Still not tested in any way, shape, or form. I decided I wanted to > send this one before booting into this and possibly blowing up ;^) Running the PoC on Linux 6.0.6 with these patches caused the following splat on the following line: WARN_ON_ONCE(!folio_test_locked(folio) && !folio_test_dirty(folio)); Although I did not hit the warning on the next line (!folio_buffers(folio)), the commit log for the warning that actually triggered also leads to the same patch of Jan Kara that is intended to check if a page is dirtied without buffers (the scenario we are concerned about). Author: Jan Kara <jack@suse.cz> Date: Thu Dec 1 11:46:40 2016 -0500 ext4: warn when page is dirtied without buffers Warn when a page is dirtied without buffers (as that will likely lead to a crash in ext4_writepages()) or when it gets newly dirtied without the page being locked (as there is nothing that prevents buffers to get stripped just before calling set_page_dirty() under memory pressure). [ 908.444806] ------------[ cut here ]------------ [ 908.451010] WARNING: CPU: 16 PID: 2113 at fs/ext4/inode.c:3634 ext4_dirty_folio+0x74/0x80 [ 908.460343] Modules linked in: [ 908.463856] CPU: 16 PID: 2113 Comm: poc Not tainted 6.0.6+ #21 [ 908.470521] Hardware name: Dell Inc. PowerEdge R630/0CNCJW, BIOS 2.13.0 05/14/2021 [ 908.479202] RIP: 0010:ext4_dirty_folio+0x74/0x80 [ 908.484489] Code: d5 ee ff 41 5c 41 5d 5d c3 cc cc cc cc be 08 00 00 00 4c 89 e7 e8 bc 03 e0 ff 4c 89 e7 e8 f4 f8 df ff 49 8b 04 24 a8 08 75 bc <0f> 0b eb b8 0f 0b eb c6 0f 1f 40 00 0f 1f 44 00 00 55 48 89 e5 41 [ 908.505851] RSP: 0018:ffff88a1197df9a8 EFLAGS: 00010246 [ 908.511826] RAX: 0057ffffc0002014 RBX: ffffffff83414b60 RCX: ffffffff818ceafc [ 908.519964] RDX: dffffc0000000000 RSI: 0000000000000008 RDI: ffffea00fffd9f40 [ 908.528103] RBP: ffff88a1197df9b8 R08: 0000000000000001 R09: fffff9401fffb3e9 [ 908.536239] R10: ffffea00fffd9f47 R11: fffff9401fffb3e8 R12: ffffea00fffd9f40 [ 908.544376] R13: ffff88a087d368d8 R14: ffff88a1197dfb08 R15: ffff88a1197dfb00 [ 908.552509] FS: 00007ff7caa68700(0000) GS:ffff8897edc00000(0000) knlGS:0000000000000000 [ 908.561731] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 908.568299] CR2: 00007ff7caa67ed8 CR3: 00000020cc970001 CR4: 00000000001706e0 [ 908.576437] Call Trace: [ 908.579252] <TASK> [ 908.581683] folio_mark_dirty+0x69/0xa0 [ 908.586097] set_page_dirty+0x2a/0x90 [ 908.590301] tlb_flush_mmu+0xc1/0x320 [ 908.594517] tlb_finish_mmu+0x49/0x190 [ 908.598822] unmap_region+0x1fa/0x250 [ 908.603029] ? anon_vma_compatible+0x120/0x120 [ 908.608110] ? __kasan_check_read+0x11/0x20 [ 908.612926] ? __vma_rb_erase+0x38a/0x610 [ 908.617547] __do_munmap+0x313/0x770 [ 908.621669] mmap_region+0x227/0xa50 [ 908.625774] ? down_read+0x320/0x320 [ 908.629874] ? lock_acquire+0x19a/0x450 [ 908.634285] ? __x64_sys_brk+0x4e0/0x4e0 [ 908.641552] ? thp_get_unmapped_area+0xca/0x150 [ 908.649404] ? cap_mmap_addr+0x1d/0x90 [ 908.656373] ? security_mmap_addr+0x3c/0x50 [ 908.663781] ? get_unmapped_area+0x173/0x1f0 [ 908.671248] ? arch_get_unmapped_area+0x330/0x330 [ 908.679231] do_mmap+0x3c3/0x610 [ 908.685519] vm_mmap_pgoff+0x177/0x230 [ 908.692303] ? randomize_page+0x70/0x70 [ 908.699133] ksys_mmap_pgoff+0x241/0x2a0 [ 908.706011] __x64_sys_mmap+0x8d/0xb0 [ 908.712594] do_syscall_64+0x3b/0x90 [ 908.719090] entry_SYSCALL_64_after_hwframe+0x63/0xcd [ 908.727201] RIP: 0033:0x7ff7cbf868e6 [ 908.733559] Code: 00 00 00 00 f3 0f 1e fa 41 f7 c1 ff 0f 00 00 75 2b 55 48 89 fd 53 89 cb 48 85 ff 74 37 41 89 da 48 89 ef b8 09 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 62 5b 5d c3 0f 1f 80 00 00 00 00 48 8b 05 71 [ 908.759522] RSP: 002b:00007ff7caa67ea8 EFLAGS: 00000206 ORIG_RAX: 0000000000000009 [ 908.770475] RAX: ffffffffffffffda RBX: 0000000000008011 RCX: 00007ff7cbf868e6 [ 908.780919] RDX: 0000000000000003 RSI: 0000000000200000 RDI: 00007ff7cbc00000 [ 908.791344] RBP: 00007ff7cbc00000 R08: 0000000000000003 R09: 0000000000000000 [ 908.801751] R10: 0000000000008011 R11: 0000000000000206 R12: 00007ffed51cbc4e [ 908.812118] R13: 00007ffed51cbc4f R14: 00007ffed51cbc50 R15: 00007ff7caa67fc0 [ 908.822523] </TASK> [ 908.827213] irq event stamp: 4169 [ 908.833101] hardirqs last enabled at (4183): [<ffffffff8133f028>] __up_console_sem+0x68/0x80 [ 908.844884] hardirqs last disabled at (4194): [<ffffffff8133f00d>] __up_console_sem+0x4d/0x80 [ 908.856622] softirqs last enabled at (4154): [<ffffffff83000430>] __do_softirq+0x430/0x5db [ 908.868167] softirqs last disabled at (4149): [<ffffffff8125fd89>] irq_exit_rcu+0xe9/0x120 [ 908.879611] ---[ end trace 0000000000000000 ]---
On Sat, Oct 29, 2022 at 7:17 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > Running the PoC on Linux 6.0.6 with these patches caused the following splat > on the following line: > > WARN_ON_ONCE(!folio_test_locked(folio) && !folio_test_dirty(folio)); Yeah, this is a sign of that "folio_mkclean() serializes with folio_mark_dirty using rmap and the page table lock". And page_remove_rmap() could *almost* be called later, but it does have code that also depends on the page table lock, although it looks like realistically that's just because it "knows" that means that preemption is disabled, so it uses non-atomic statistics update. I say "knows" in quotes, because that's what the comment says, but it turns out that __mod_node_page_state() has to deal with CONFIG_RT anyway and does that preempt_disable_nested(); ... preempt_enable_nested(); thing. And then it wants to see the vma, although that's actually only to see if it's 'mlock'ed, so we could just squirrel that away. So we *could* move page_remove_rmap() later into the TLB flush region, but then we would have lost the page table lock anyway, so then folio_mkclean() can come in regardless. So that doesn't even help. End result: we do want to do the page_set_dirty() and the remove_rmap() under the paeg table lock, because it's what serializes folio_mkclean(). And we'd _like_ to do the TLB flush before the remove_rmap(), but we *really* don't want to do that for every page. So my current gut feel is that we should just say that if you do "MADV_DONTNEED or do a munmap() (which includes the "re-mmap() over the area", while some other thread is still writing to that memory region, you may lose writes. IOW, just accept the behavior that Nadav's test-program tries to show, and say "look, you're doing insane things, we've never given you any other semantics, it's your problem" to any user program that does that. If a user program does MADV_DONTNEED on an area that it is actively using at the same time in another thread, that sounds really really bogus. Same goes doubly for 'munmap()' or over-mapping. Linus
On Sun, Oct 30, 2022 at 11:19 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > And we'd _like_ to do the TLB flush before the remove_rmap(), but we > *really* don't want to do that for every page. Hmm. I have yet another crazy idea. We could keep the current placement of the TLB flush, to just before we drop the page table lock. And we could do all the things we do in 'page_remove_rmap()' right now *except* for the mapcount stuff. And only move the mapcount code to the page freeing stage. Because all the rmap() walk synchronization really needs is that 'page->_mapcount' is still elevated, and if it is it will serialize with the page table lock. And it turns out that 'page_remove_rmap()' already treats the case we care about differently, and all it does is lock_page_memcg(page); if (!PageAnon(page)) { page_remove_file_rmap(page, compound); goto out; } ... out: unlock_page_memcg(page); munlock_vma_page(page, vma, compound); for that case. And that 'page_remove_file_rmap()' is literally the code that modifies the _mapcount. Annoyingly, this is all complicated by that 'compound' argument, but that's always false in that zap_page_range() case. So what we *could* do, is make a new version of page_remove_rmap(), which is specialized for this case: no 'compound' argument (always false), and doesn't call 'page_remove_file_rmap()', because we'll do that for the !PageAnon(page) case later after the TLB flush. That would keep the existing TLB flush logic, keep the existing 'mark page dirty' and would just make sure that 'folio_mkclean()' ends up being serialized with the TLB flush simply because it will take the page table lock because we delay the '._mapcount' update until afterwards. Annoyingly, the organization of 'page_remove_rmap()' is a bit ugly, and we have several other callers that want the existing logic, so while the above sounds conceptually simple, I think the patch would be a bit messy. Linus
On Oct 30, 2022, at 11:19 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > And page_remove_rmap() could *almost* be called later, but it does > have code that also depends on the page table lock, although it looks > like realistically that's just because it "knows" that means that > preemption is disabled, so it uses non-atomic statistics update. > > I say "knows" in quotes, because that's what the comment says, but it > turns out that __mod_node_page_state() has to deal with CONFIG_RT > anyway and does that > > preempt_disable_nested(); > ... > preempt_enable_nested(); > > thing. > > And then it wants to see the vma, although that's actually only to see > if it's 'mlock'ed, so we could just squirrel that away. > > So we *could* move page_remove_rmap() later into the TLB flush region, > but then we would have lost the page table lock anyway, so then > folio_mkclean() can come in regardless. > > So that doesn't even help. Well, if you combine it with the per-page-table stale TLB detection mechanism that I proposed, I think this could work. Reminder (feel free to skip): you would have per-mm “completed TLB-generation” in addition to the current one, which would be renamed to “pending TLB-generation”. Whenever you update the page-tables in a manner that might require a TLB flush, you would increase the “pending TLB-generation” and save the pending TLB-generation in the page-table’s page-struct. All of that is done once under the page-table lock. When you finish a TLB-flush, you update the “completed TLB-generation”. Then on page_vma_mkclean_one(), you would check if the page-table’s TLB-generation is greater than the completed TLB-generation, which would indicate that TLB entries for PTEs in this table might be stale. In that case you would just flush the TLB. [ Of course you can instead just flush if mm_tlb_flush_pending(), but nobody likes this mechanism that has a very coarse granularity, and therefore can lead to many unnecessary TLB flushes. ] Indeed, there would be potentially some overhead in extreme cases, since mm's TLB-generation since its cache is already highly-contended in extreme cases. But I think it worth it to have simple logic that allows to reason about correctness. My intuition is that although you appear to be right that we can just mark this case as “extreme case nobody cares about”, it might have now or in the future some other implications that are hard to predict and prevent.
On Sun, Oct 30, 2022 at 11:51 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > We could keep the current placement of the TLB flush, to just before > we drop the page table lock. > > And we could do all the things we do in 'page_remove_rmap()' right now > *except* for the mapcount stuff. > > And only move the mapcount code to the page freeing stage. So I actually have a three-commit series to do the rmap simplification, but let me just post the end result of that series, because the end result is actually smaller than the individual commits (I did it as three incremental commits just to make it more obvious to me how to get to that end result). The three commits end up being mm: introduce simplified versions of 'page_remove_rmap()' mm: inline simpler case of page_remove_file_rmap() mm: re-unify the simplified page_zap_*_rmap() function and the end result of them is this attached patch. I'm *claiming* that the attached patch is semantically identical to what we do before it, just _hugely_ simplified. Basically, that new 'page_zap_pte_rmap()' does the same things that 'page_remove_rmap()' did, except it is limited to only last-level PTE entries (and that munlock_vma_page() has to be called separately). The simplification comes from 'compound' being false, from it always being about small pages, and from the atomic mapcount decrement having been moved outside the memcg lock, since it is independent of it. Anyway, this simplification patch basically means that the *next* step could be to just move that ipage_zap_pte_rmap()' after the TLB flush, and now it's trivial and no longer scary. I did *not* do that yet, because it still needs that "encoded_page[]" array - except now it doesn't encode the 'dirty' bit, now it would encode the 'do a page->_mapcount decrement' bit. I didn't do that part, because needed to do the rc3 release, plus I'd like to have somebody look at this introductory patch first. Linus
On Sun, Oct 30, 2022 at 3:47 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Anyway, this simplification patch basically means that the *next* step > could be to just move that ipage_zap_pte_rmap()' after the TLB flush, > and now it's trivial and no longer scary. So here's that next patch: it's patch 4/4 in this series. Patches 1-3 are the patches that I already sent out as one (smaller) combined patch. I'm including them here as the whole series in case somebody else wants to follow along with how I did the simplified version of page_remove_rmap(). So if you already looked at the previous patch and don't have any questions about that one, you could just apply PATCH 4/4 on top of that one. Or you can do the whole series with commit messages and (hopefully) clearer individual steps to the simplified version of page_remove_rmap(). I haven't actually tested 4/4 yet, so this is yet another of my "I think this should work" patches. The reason I haven't actually tested it is partly because I never recreated the original problem Navav reported, and partly because the meat of patch 4/4 is just the same "encode an extra flag bit in the low bit of the page pointer" that I _did_ test, just doing the "remove rmap" instead of "set dirty". In other words, I *think* this should make Nadav's test-case happy, and avoid the warning he saw. If somebody wants a git branch, I guess I can push that too, but it would be a non-stable branch only for testing. Also, it's worth noting that zap_pte_range() does this sanity test: if (unlikely(page_mapcount(page) < 0)) print_bad_pte(vma, addr, ptent, page); and that is likely worthless now (because it hasn't actually decremented the mapcount yet). I didn't remove it, because I wasn't sure which option was best: (a) just remove it entirely (b) change the "< 0" to "<= 0" (c) move it to clean_and_free_pages_and_swap_cache() that actually does the page_zap_pte_rmap() now. so I'm in no way claiming this series is any kind of final word, but I do mostly like how the code ended up looking. Now, this whole "remove rmap after flush" would probably make sense for some of the largepage cases too, and there's room for another bit in there (or more, if you align 'struct page') and that whole code could use page size hints etc too. But I suspect that the main case we really care is for individual small pages, just because that's the case where I think it would be much worse to do any fancy TLB tracking. The largepage cases presumably aren't as critical, since there by definition is fewer of those. Comments? Linus
On Oct 30, 2022, at 6:47 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > The reason I haven't actually tested it is partly because I never > recreated the original problem Navav reported, and partly because the > meat of patch 4/4 is just the same "encode an extra flag bit in the > low bit of the page pointer" that I _did_ test, just doing the "remove > rmap" instead of "set dirty". > > In other words, I *think* this should make Nadav's test-case happy, > and avoid the warning he saw. I am sorry for not managing to make it reproducible on your system. The fact that you did not get the warning that I got means that it is not a hardware-TLB differences issue (at least not only that), but the race does not happen on your system (assuming you used ext4 on the BRD). Anyhow, I ran the tests with the patches and there are no failures. Thanks for addressing this issue. I understand from the code that you decided to drop the deferring of set_page_dirty(), which could - at least for the munmap case (where mmap_lock is taken for write) - prevent the need for “force_flush” and potentially save TLB flushes. I was just wondering whether the reason for that is that you wanted to have small backportable and conservative patches, or whether you changed your mind about it.
On Oct 30, 2022, at 9:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > I understand from the code that you decided to drop the deferring of > set_page_dirty(), which could - at least for the munmap case (where > mmap_lock is taken for write) - prevent the need for “force_flush” and > potentially save TLB flushes. > > I was just wondering whether the reason for that is that you wanted > to have small backportable and conservative patches, or whether you > changed your mind about it. Please ignore this silly question. I understand - the buffers might still be dropped.
On Sun, Oct 30, 2022 at 9:09 PM Nadav Amit <nadav.amit@gmail.com> wrote: > > I am sorry for not managing to make it reproducible on your system. Heh, that's very much *not* your fault. Honestly, I didn't try very much or very hard. I felt like I understood the problem cause sufficiently that I didn't really need to have a reproducer, and I much prefer to just think the solution through and try to make it really robust. Or, put another way - I'm just lazy. > Anyhow, I ran the tests with the patches and there are no failures. Lovely. > Thanks for addressing this issue. Well, I'm not sure the issue is "addressed" yet. I think the patch series is likely the right thing to do, but others may disagree with this approach. And regardless of that, this still leaves some questions open. (a) there's the issue of s390, which does its own version of __tlb_remove_page_size. I *think* s390 basically does the TLB flush synchronously in zap_pte_range(), and that it would be for that reason trivial to just add that 'flags' argument to the s390 __tlb_remove_page_size(), and make it do if (flags & TLB_ZAP_RMAP) page_zap_pte_rmap(page); at the top synchronously too. But some s390 person would need to look at it. I *think* the issue is literally that straightforward and not a big deal, but it's probably not even worth bothering the s390 people until VM people have decided "yes, this makes sense". (b) the issue I mentioned with the currently useless "page_mapcount(page) < 0" test with that patch. Again, this is mostly just janitorial stuff associated with that patch series. (c) whether to worry about back-porting I don't *think* this is worth backporting, but if it causes other changes, then maybe.. > I understand from the code that you decided to drop the deferring of > set_page_dirty(), which could - at least for the munmap case (where > mmap_lock is taken for write) - prevent the need for “force_flush” and > potentially save TLB flushes. I really liked my dirty patch, but your warning case really made it obvious that it was just broken. The thing is, moving the "set_page_dirty()" to later is really nice, and really makes a *lot* of sense from a conceptual standpoint: only after that TLB flush do we really have no more people who can dirty it. BUT. Even if we just used another bit for in the array for "dirty", and did the set_page_dirty() later (but still before getting rid of the rmap), that wouldn't actually *work*. Why? Because the race with folio_mkclean() would just come back. Yes, now we'd have the rmap data, so mkclean would be forced to serialize with the page table lock. But if we get rid of the "force_flush" for the dirty bit, that serialization won't help, simply because we've *dropped* the page table lock before we actually then do the set_page_dirty() again. So the mkclean serialization needs *both* the late rmap dropping _and_ the page table lock being kept. So deferring set_page_dirty() is conceptually the right thing to do from a pure "just track dirty bit" standpoint, but it doesn't work with the way we currently expect mkclean to work. > I was just wondering whether the reason for that is that you wanted > to have small backportable and conservative patches, or whether you > changed your mind about it. See above: I still think it would be the right thing in a perfect world. But with the current folio_mkclean(), we just can't do it. I had completely forgotten / repressed that horror-show. So the current ordering rules are basically that we need to do set_page_dirty() *and* we need to flush the TLB's before dropping the page table lock. That's what gets us serialized with "mkclean". The whole "drop rmap" can then happen at any later time, the only important thing was that it was kept to at least after the TLB flush. We could do the rmap drop still inside the page table lock, but honestly, it just makes more sense to do as we free the batched pages anyway. Am I missing something still? And again, this is about our horrid serialization between folio_mkclean and set_page_dirty(). It's related to how GUP + set_page_dirty() is also fundamentally problematic. So that dirty bit situation *may* change if the rules for folio_mkclean() change... Linus
On Sun, Oct 30, 2022 at 03:47:36PM -0700, Linus Torvalds wrote: > include/linux/rmap.h | 1 + > mm/memory.c | 3 ++- > mm/rmap.c | 24 ++++++++++++++++++++++++ > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index bd3504d11b15..f62af001707c 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -196,6 +196,7 @@ void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, > unsigned long address); > void page_add_file_rmap(struct page *, struct vm_area_struct *, > bool compound); > +void page_zap_pte_rmap(struct page *); > void page_remove_rmap(struct page *, struct vm_area_struct *, > bool compound); > > diff --git a/mm/memory.c b/mm/memory.c > index f88c351aecd4..c893f5ffc5a8 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1452,8 +1452,9 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > likely(!(vma->vm_flags & VM_SEQ_READ))) > mark_page_accessed(page); > } > + page_zap_pte_rmap(page); > + munlock_vma_page(page, vma, false); > rss[mm_counter(page)]--; > - page_remove_rmap(page, vma, false); > if (unlikely(page_mapcount(page) < 0)) > print_bad_pte(vma, addr, ptent, page); > if (unlikely(__tlb_remove_page(tlb, page))) { > diff --git a/mm/rmap.c b/mm/rmap.c > index 2ec925e5fa6a..28b51a31ebb0 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1412,6 +1412,30 @@ static void page_remove_anon_compound_rmap(struct page *page) > __mod_lruvec_page_state(page, NR_ANON_MAPPED, -nr); > } > > +/** > + * page_zap_pte_rmap - take down a pte mapping from a page > + * @page: page to remove mapping from > + * > + * This is the simplified form of page_remove_rmap(), that only > + * deals with last-level pages, so 'compound' is always false, > + * and the caller does 'munlock_vma_page(page, vma, compound)' > + * separately. > + * > + * This allows for a much simpler calling convention and code. > + * > + * The caller holds the pte lock. > + */ > +void page_zap_pte_rmap(struct page *page) > +{ One could consider adding something like: #ifdef USE_SPLIT_PTE_PTLOCKS lockdep_assert_held(ptlock_ptr(page)) #endif > + if (!atomic_add_negative(-1, &page->_mapcount)) > + return; > + > + lock_page_memcg(page); > + __dec_lruvec_page_state(page, > + PageAnon(page) ? NR_ANON_MAPPED : NR_FILE_MAPPED); > + unlock_page_memcg(page); > +} Took me a little while, but yes, .compound=false seems to reduce to this.
On Sun, Oct 30, 2022 at 06:47:23PM -0700, Linus Torvalds wrote: > Also, it's worth noting that zap_pte_range() does this sanity test: > > if (unlikely(page_mapcount(page) < 0)) > print_bad_pte(vma, addr, ptent, page); > > and that is likely worthless now (because it hasn't actually > decremented the mapcount yet). I didn't remove it, because I wasn't > sure which option was best: > > (a) just remove it entirely > > (b) change the "< 0" to "<= 0" > > (c) move it to clean_and_free_pages_and_swap_cache() that actually > does the page_zap_pte_rmap() now. I'm leaning towards (c); simply because the error case is so terrifying I feel we should check for it (and I do have vague memories of us actually hitting something like this in the very distant past).
On Sun, Oct 30, 2022 at 06:47:23PM -0700, Linus Torvalds wrote: > - there's no 'compund' argument any more, because it was always false > > - we can get rid of the tests for 'compund' and 'PageAnon()' since we > know that they are You're making up new words ;-) s/compund/compound/g
On Sun, Oct 30, 2022 at 06:47:23PM -0700, Linus Torvalds wrote: > diff --git a/mm/memory.c b/mm/memory.c > index ba1d08a908a4..c893f5ffc5a8 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1451,9 +1451,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > if (pte_young(ptent) && > likely(!(vma->vm_flags & VM_SEQ_READ))) > mark_page_accessed(page); > + } > + page_zap_pte_rmap(page); > munlock_vma_page(page, vma, false); > rss[mm_counter(page)]--; > if (unlikely(page_mapcount(page) < 0)) > diff --git a/mm/rmap.c b/mm/rmap.c > index 69de6c833d5c..28b51a31ebb0 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1413,47 +1413,26 @@ static void page_remove_anon_compound_rmap(struct page *page) > } > > /** > + * page_zap_pte_rmap - take down a pte mapping from a page > * @page: page to remove mapping from > * > + * This is the simplified form of page_remove_rmap(), that only > + * deals with last-level pages, so 'compound' is always false, > + * and the caller does 'munlock_vma_page(page, vma, compound)' > + * separately. > * > + * This allows for a much simpler calling convention and code. > * > * The caller holds the pte lock. > */ > +void page_zap_pte_rmap(struct page *page) > { > if (!atomic_add_negative(-1, &page->_mapcount)) > return; > > lock_page_memcg(page); > + __dec_lruvec_page_state(page, > + PageAnon(page) ? NR_ANON_MAPPED : NR_FILE_MAPPED); > unlock_page_memcg(page); > } So we *could* use atomic_add_return() and include the print_bad_pte() thing in this function -- however that turns the whole thing into a mess again :/
On Oct 30, 2022, at 10:00 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > So the current ordering rules are basically that we need to do > set_page_dirty() *and* we need to flush the TLB's before dropping the > page table lock. That's what gets us serialized with "mkclean”. I understand. I am still not sure whether ordering the set_page_dirty() and dropping the mapcount reference cannot suffice for the reclaim logic not to free the buffers if the page is dirtied. According to the code, shrink_page_list() first checks for folio_mapped() and then for folio_test_dirty() to check whether pageout() is necessary. IIUC, the buffers are not dropped up to this point and set_page_dirty() would always set the page-struct dirty bit. IOW: In shrink_page_list(), when we decide on whether to pageout(), we should see whether the page is dirty (give or take smp_rmb()). But this is an optimization and I do not know all the cases in which buffers might be dropped. My intuition says that they cannot be dropped while mapcount != 0, but I need to further explore it.
On Mon, Oct 31, 2022 at 2:29 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Sun, Oct 30, 2022 at 03:47:36PM -0700, Linus Torvalds wrote: > > > + * This is the simplified form of page_remove_rmap(), that only > > + * deals with last-level pages, so 'compound' is always false, > > + * and the caller does 'munlock_vma_page(page, vma, compound)' > > + * separately. > > + * > > + * This allows for a much simpler calling convention and code. > > + * > > + * The caller holds the pte lock. > > + */ > > +void page_zap_pte_rmap(struct page *page) > > +{ > > One could consider adding something like: > > #ifdef USE_SPLIT_PTE_PTLOCKS > lockdep_assert_held(ptlock_ptr(page)) > #endif Yes, except that the page lock goes away in the next few patches and gets replaced by just using the safe dec_lruvec_page_state() instead, so it's not really worth it. > > + if (!atomic_add_negative(-1, &page->_mapcount)) > > + return; > > + > > + lock_page_memcg(page); > > + __dec_lruvec_page_state(page, > > + PageAnon(page) ? NR_ANON_MAPPED : NR_FILE_MAPPED); > > + unlock_page_memcg(page); > > +} > > Took me a little while, but yes, .compound=false seems to reduce to > this. Yeah - it's why I kept that thing as three separate patches, because even if each of the patches isn't "immediately obvious", you can at least go back and follow along and see what I did. The *full* simplification end result just looks like magic. Admittedly, I think a lot of that "looks like magic" is because the rmap code has seriously cruftified over the years. We had that time when we actually Go back a decade, and we literally used to do pretty much exactly what the simplified form does. The transformation to complexity hell starts with commit 89c06bd52fb9 ("memcg: use new logic for page stat accounting"), but just look at what it looked like before that: git show 89c06bd52fb9^:mm/rmap.c gets you the state back when it was simple. And look at what it did: void page_remove_rmap(struct page *page) { /* page still mapped by someone else? */ if (!atomic_add_negative(-1, &page->_mapcount)) return; ... statistics go here .. so in the end the simplified form of this page_zap_pte_rmap() really isn't *that* surprising. In fact, that old code handled PageHuge() fairly naturally too, and almost all the mess comes from the memcg accounting - and locking - changes. And I actually really wanted to one step further, and try to then batch up the page state accounting too. It's kind of stupid how we do all that memcg locking for each page, and with this new setup we have one nice array of pages that we *could* try to just batch things with. The pages in normal situations *probably* mostly all share the same memcg and node, so just optimistically trying to do something like "as long as it's the same memcg as last time, just keep the lock". Instead of locking and unlocking for every single page. But just looking at it exhausted me enough that I don't think I'll go there. Put another way: after this series, it's not the 'rmap' code that makes me go Ugh, it's the memcg tracking.. (But the hugepage rmap code is incredibly nasty stuff still, and I think the whole 'compound=true' case would be better with somebody taking a look at that case too, but that somebody won't be me). Linus
On Mon, Oct 31, 2022 at 2:39 AM Peter Zijlstra <peterz@infradead.org> wrote: > > You're making up new words ;-) > > s/compund/compound/g Oops. Fixed. Thanks, Linus
On Mon, Oct 31, 2022 at 2:36 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > (c) move it to clean_and_free_pages_and_swap_cache() that actually > > does the page_zap_pte_rmap() now. > > I'm leaning towards (c); simply because the error case is so terrifying > I feel we should check for it (and I do have vague memories of us > actually hitting something like this in the very distant past). Ok. At that point we no longer have the pte or the virtual address, so it's not goign to be exactly the same debug output. But I think it ends up being fairly natural to do VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); instead, and I've fixed that last patch up to do that. Linus
On Mon, Oct 31, 2022 at 8:43 AM Nadav Amit <nadav.amit@gmail.com> wrote: > > On Oct 30, 2022, at 10:00 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > So the current ordering rules are basically that we need to do > > set_page_dirty() *and* we need to flush the TLB's before dropping the > > page table lock. That's what gets us serialized with "mkclean”. > > I understand. I am still not sure whether ordering the set_page_dirty() and > dropping the mapcount reference cannot suffice for the reclaim logic not to > free the buffers if the page is dirtied. Ahh, ok. > According to the code, shrink_page_list() first checks for folio_mapped() > and then for folio_test_dirty() to check whether pageout() is necessary. > IIUC, the buffers are not dropped up to this point and set_page_dirty() > would always set the page-struct dirty bit. > > IOW: In shrink_page_list(), when we decide on whether to pageout(), we > should see whether the page is dirty (give or take smp_rmb()). > > But this is an optimization and I do not know all the cases in which buffers > might be dropped. My intuition says that they cannot be dropped while > mapcount != 0, but I need to further explore it. Yes, the above sounds like one fairly good way out of the whole forced TLB flushing for dirty pages, while still keeping the filesystem code happy. But at this point it's an independent issue. And I really would like any fix like that to also fix the whole issue with GUP too, which seems related. Linus
Updated subject line, and here's the link to the original discussion
for new people:
https://lore.kernel.org/all/B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com/
On Mon, Oct 31, 2022 at 10:28 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ok. At that point we no longer have the pte or the virtual address, so
> it's not going to be exactly the same debug output.
>
> But I think it ends up being fairly natural to do
>
> VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page);
>
> instead, and I've fixed that last patch up to do that.
Ok, so I've got a fixed set of patches based on the feedback from
PeterZ, and also tried to do the s390 updates for this blindly, and
pushed them out into a git branch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix
If people really want to see the patches in email again, I can do
that, but most of you already have, and the changes are either trivial
fixes or the s390 updates.
For the s390 people that I've now added to the participant list maybe
the git tree is fine - and the fundamental explanation of the problem
is in that top-most commit (with the three preceding commits being
prep-work). Or that link to the thread about this all.
That top-most commit is also where I tried to fix things up for s390
that uses its own non-gathering TLB flush due to
CONFIG_MMU_GATHER_NO_GATHER.
NOTE NOTE NOTE! Unlike my regular git branch, this one may end up
rebased etc for further comments and fixes. So don't consider that
stable, it's still more of an RFC branch.
At a minimum I'll update it with Ack's etc, assuming I get those, and
my s390 changes are entirely untested and probably won't work.
As far as I can tell, s390 doesn't actually *have* the problem that
causes this change, because of its synchronous TLB flush, but it
obviously needs to deal with the change of rmap zapping logic.
Also added a few people who are explicitly listed as being mmu_gather
maintainers. Maybe people saw the discussion on the linux-mm list, but
let's make it explicit.
Do people have any objections to this approach, or other suggestions?
I do *not* consider this critical, so it's a "queue for 6.2" issue for me.
It probably makes most sense to queue in the -MM tree (after the thing
is acked and people agree), but I can keep that branch alive too and
just deal with it all myself as well.
Anybody?
Linus
Am 31.10.22 um 19:43 schrieb Linus Torvalds: > Updated subject line, and here's the link to the original discussion > for new people: > > https://lore.kernel.org/all/B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com/ > > On Mon, Oct 31, 2022 at 10:28 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Ok. At that point we no longer have the pte or the virtual address, so >> it's not going to be exactly the same debug output. >> >> But I think it ends up being fairly natural to do >> >> VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); >> >> instead, and I've fixed that last patch up to do that. > > Ok, so I've got a fixed set of patches based on the feedback from > PeterZ, and also tried to do the s390 updates for this blindly, and > pushed them out into a git branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix > > If people really want to see the patches in email again, I can do > that, but most of you already have, and the changes are either trivial > fixes or the s390 updates. > > For the s390 people that I've now added to the participant list maybe > the git tree is fine - and the fundamental explanation of the problem > is in that top-most commit (with the three preceding commits being > prep-work). Or that link to the thread about this all. Adding Gerald. > > That top-most commit is also where I tried to fix things up for s390 > that uses its own non-gathering TLB flush due to > CONFIG_MMU_GATHER_NO_GATHER. > > NOTE NOTE NOTE! Unlike my regular git branch, this one may end up > rebased etc for further comments and fixes. So don't consider that > stable, it's still more of an RFC branch. > > At a minimum I'll update it with Ack's etc, assuming I get those, and > my s390 changes are entirely untested and probably won't work. > > As far as I can tell, s390 doesn't actually *have* the problem that > causes this change, because of its synchronous TLB flush, but it > obviously needs to deal with the change of rmap zapping logic. > > Also added a few people who are explicitly listed as being mmu_gather > maintainers. Maybe people saw the discussion on the linux-mm list, but > let's make it explicit. > > Do people have any objections to this approach, or other suggestions? > > I do *not* consider this critical, so it's a "queue for 6.2" issue for me. > > It probably makes most sense to queue in the -MM tree (after the thing > is acked and people agree), but I can keep that branch alive too and > just deal with it all myself as well. > > Anybody? > > Linus It certainly needs a build fix for s390: In file included from kernel/sched/core.c:78: ./arch/s390/include/asm/tlb.h: In function '__tlb_remove_page_size': ./arch/s390/include/asm/tlb.h:50:17: error: implicit declaration of function 'page_zap_pte_rmap' [-Werror=implicit-function-declaration] 50 | page_zap_pte_rmap(page); | ^~~~~~~~~~~~~~~~~
Am 02.11.22 um 10:14 schrieb Christian Borntraeger: > Am 31.10.22 um 19:43 schrieb Linus Torvalds: >> Updated subject line, and here's the link to the original discussion >> for new people: >> >> https://lore.kernel.org/all/B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com/ >> >> On Mon, Oct 31, 2022 at 10:28 AM Linus Torvalds >> <torvalds@linux-foundation.org> wrote: >>> >>> Ok. At that point we no longer have the pte or the virtual address, so >>> it's not going to be exactly the same debug output. >>> >>> But I think it ends up being fairly natural to do >>> >>> VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); >>> >>> instead, and I've fixed that last patch up to do that. >> >> Ok, so I've got a fixed set of patches based on the feedback from >> PeterZ, and also tried to do the s390 updates for this blindly, and >> pushed them out into a git branch: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix >> >> If people really want to see the patches in email again, I can do >> that, but most of you already have, and the changes are either trivial >> fixes or the s390 updates. >> >> For the s390 people that I've now added to the participant list maybe >> the git tree is fine - and the fundamental explanation of the problem >> is in that top-most commit (with the three preceding commits being >> prep-work). Or that link to the thread about this all. > > Adding Gerald. now the correct Gerald.... > >> >> That top-most commit is also where I tried to fix things up for s390 >> that uses its own non-gathering TLB flush due to >> CONFIG_MMU_GATHER_NO_GATHER. >> >> NOTE NOTE NOTE! Unlike my regular git branch, this one may end up >> rebased etc for further comments and fixes. So don't consider that >> stable, it's still more of an RFC branch. >> >> At a minimum I'll update it with Ack's etc, assuming I get those, and >> my s390 changes are entirely untested and probably won't work. >> >> As far as I can tell, s390 doesn't actually *have* the problem that >> causes this change, because of its synchronous TLB flush, but it >> obviously needs to deal with the change of rmap zapping logic. >> >> Also added a few people who are explicitly listed as being mmu_gather >> maintainers. Maybe people saw the discussion on the linux-mm list, but >> let's make it explicit. >> >> Do people have any objections to this approach, or other suggestions? >> >> I do *not* consider this critical, so it's a "queue for 6.2" issue for me. >> >> It probably makes most sense to queue in the -MM tree (after the thing >> is acked and people agree), but I can keep that branch alive too and >> just deal with it all myself as well. >> >> Anybody? >> >> Linus > > It certainly needs a build fix for s390: > > > In file included from kernel/sched/core.c:78: > ./arch/s390/include/asm/tlb.h: In function '__tlb_remove_page_size': > ./arch/s390/include/asm/tlb.h:50:17: error: implicit declaration of function 'page_zap_pte_rmap' [-Werror=implicit-function-declaration] > 50 | page_zap_pte_rmap(page); > | ^~~~~~~~~~~~~~~~~
On Mon, Oct 31, 2022 at 11:43:30AM -0700, Linus Torvalds wrote: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
On Wed, Nov 2, 2022 at 2:15 AM Christian Borntraeger <borntraeger@linux.ibm.com> wrote: > > It certainly needs a build fix for s390: > > In file included from kernel/sched/core.c:78: > ./arch/s390/include/asm/tlb.h: In function '__tlb_remove_page_size': > ./arch/s390/include/asm/tlb.h:50:17: error: implicit declaration of function 'page_zap_pte_rmap' [-Werror=implicit-function-declaration] > 50 | page_zap_pte_rmap(page); > | ^~~~~~~~~~~~~~~~~ Hmm. I'm not sure if I can add a #include <linux/rmap.h> to that s390 asm header file without causing more issues. The minimal damage would probably be to duplicate the declaration of page_zap_pte_rmap() in the s390 asm/tlb.h header where it is used. Not pretty to have two different declarations of that thing, but anything that then includes both <asm/tlb.h> and <linux/rmap.h> (which is much of mm) would then verify the consistency of them. So I'll do that minimal fix and update that branch, but if s390 people end up having a better fix, please holler. Linus
On Wed, Nov 2, 2022 at 10:55 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I'll do that minimal fix and update that branch, but if s390 people > end up having a better fix, please holler. I've updated the branch with that, so hopefully s390 builds now. I also fixed a typo in the commit message and added Peter's ack. Other than that it's all the same it was before. Linus
On Wed, 2 Nov 2022 10:55:10 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Nov 2, 2022 at 2:15 AM Christian Borntraeger > <borntraeger@linux.ibm.com> wrote: > > > > It certainly needs a build fix for s390: > > > > In file included from kernel/sched/core.c:78: > > ./arch/s390/include/asm/tlb.h: In function '__tlb_remove_page_size': > > ./arch/s390/include/asm/tlb.h:50:17: error: implicit declaration of function 'page_zap_pte_rmap' [-Werror=implicit-function-declaration] > > 50 | page_zap_pte_rmap(page); > > | ^~~~~~~~~~~~~~~~~ > > Hmm. I'm not sure if I can add a > > #include <linux/rmap.h> > > to that s390 asm header file without causing more issues. > > The minimal damage would probably be to duplicate the declaration of > page_zap_pte_rmap() in the s390 asm/tlb.h header where it is used. > > Not pretty to have two different declarations of that thing, but > anything that then includes both <asm/tlb.h> and <linux/rmap.h> (which > is much of mm) would then verify the consistency of them. > > So I'll do that minimal fix and update that branch, but if s390 people > end up having a better fix, please holler. It compiles now with your duplicate declaration, but adding the #include also did not cause any damage, so that should also be OK.
On Mon, 31 Oct 2022 11:43:30 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > Updated subject line, and here's the link to the original discussion > for new people: > > https://lore.kernel.org/all/B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com/ > > On Mon, Oct 31, 2022 at 10:28 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Ok. At that point we no longer have the pte or the virtual address, so > > it's not going to be exactly the same debug output. > > > > But I think it ends up being fairly natural to do > > > > VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); > > > > instead, and I've fixed that last patch up to do that. > > Ok, so I've got a fixed set of patches based on the feedback from > PeterZ, and also tried to do the s390 updates for this blindly, and > pushed them out into a git branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix > > If people really want to see the patches in email again, I can do > that, but most of you already have, and the changes are either trivial > fixes or the s390 updates. > > For the s390 people that I've now added to the participant list maybe > the git tree is fine - and the fundamental explanation of the problem > is in that top-most commit (with the three preceding commits being > prep-work). Or that link to the thread about this all. > > That top-most commit is also where I tried to fix things up for s390 > that uses its own non-gathering TLB flush due to > CONFIG_MMU_GATHER_NO_GATHER. > > NOTE NOTE NOTE! Unlike my regular git branch, this one may end up > rebased etc for further comments and fixes. So don't consider that > stable, it's still more of an RFC branch. > > At a minimum I'll update it with Ack's etc, assuming I get those, and > my s390 changes are entirely untested and probably won't work. > > As far as I can tell, s390 doesn't actually *have* the problem that > causes this change, because of its synchronous TLB flush, but it > obviously needs to deal with the change of rmap zapping logic. Correct, we need to flush already when we change a PTE, which is done in ptep_get_and_clear() etc. Only exception would be lazy flushing when only one active thread is attached, then we would flush later in flush_tlb_mm/range(), or as soon as another thread is attached (IIRC). So it seems straight forward to just call page_zap_pte_rmap() from our private __tlb_remove_page_size() implementation. Just wondering a bit why you did not also add the VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page), like in the generic change. Acked-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> # s390
On Wed, Nov 2, 2022 at 3:31 PM Gerald Schaefer <gerald.schaefer@linux.ibm.com> wrote: > > Just wondering a bit why you did not also add the > VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page), like > in the generic change. Heh, I had considered dropping it entirely even from the generic code, since I don't remember seeing that ever trigger, but PeterZ convinced me otherwise. For the s390 side I really wanted to keep things minimal since I (obviously) didn't even built-test it, so.. I'm perfectly happy with s390 people adding it later, of course. Linus
On 31.10.22 19:43, Linus Torvalds wrote: > Updated subject line, and here's the link to the original discussion > for new people: > > https://lore.kernel.org/all/B88D3073-440A-41C7-95F4-895D3F657EF2@gmail.com/ > > On Mon, Oct 31, 2022 at 10:28 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Ok. At that point we no longer have the pte or the virtual address, so >> it's not going to be exactly the same debug output. >> >> But I think it ends up being fairly natural to do >> >> VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); >> >> instead, and I've fixed that last patch up to do that. > > Ok, so I've got a fixed set of patches based on the feedback from > PeterZ, and also tried to do the s390 updates for this blindly, and > pushed them out into a git branch: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=mmu_gather-race-fix > > If people really want to see the patches in email again, I can do > that, but most of you already have, and the changes are either trivial > fixes or the s390 updates. > > For the s390 people that I've now added to the participant list maybe > the git tree is fine - and the fundamental explanation of the problem > is in that top-most commit (with the three preceding commits being > prep-work). Or that link to the thread about this all. > > That top-most commit is also where I tried to fix things up for s390 > that uses its own non-gathering TLB flush due to > CONFIG_MMU_GATHER_NO_GATHER. > > NOTE NOTE NOTE! Unlike my regular git branch, this one may end up > rebased etc for further comments and fixes. So don't consider that > stable, it's still more of an RFC branch. > > At a minimum I'll update it with Ack's etc, assuming I get those, and > my s390 changes are entirely untested and probably won't work. > > As far as I can tell, s390 doesn't actually *have* the problem that > causes this change, because of its synchronous TLB flush, but it > obviously needs to deal with the change of rmap zapping logic. > > Also added a few people who are explicitly listed as being mmu_gather > maintainers. Maybe people saw the discussion on the linux-mm list, but > let's make it explicit. > > Do people have any objections to this approach, or other suggestions? > > I do *not* consider this critical, so it's a "queue for 6.2" issue for me. > > It probably makes most sense to queue in the -MM tree (after the thing > is acked and people agree), but I can keep that branch alive too and > just deal with it all myself as well. > > Anybody? Happy to see that we're still decrementing the mapcount before decrementingthe refcount, I was briefly concerned. I was not able to come up quickly with something that would be fundamentally wrong here, but devil is in the detail. Some minor things could be improved IMHO (ENCODE_PAGE_BITS naming is unfortunate, TLB_ZAP_RMAP could be a __bitwise type, using VM_WARN_ON instead of VM_BUG_ON). I agree that 6.2 is good enough and that upstreaming this via the -MM tree would be a good way to move forward.
On Thu, Nov 3, 2022 at 2:52 AM David Hildenbrand <david@redhat.com> wrote: > > Happy to see that we're still decrementing the mapcount before > decrementingthe refcount, I was briefly concerned. Oh, that would have been horribly wrong. > I was not able to come up quickly with something that would be > fundamentally wrong here, but devil is in the detail. So I tried to be very careful. The biggest change in the whole series (visible in last patch, but there in the prep-patches too) is how it narrows down some lock coverage. Now, that locking didn't *do* anything valid, but I did try to point it out when it happens - how first the mapcount is decremented outside the memcg lock (in preparatory patches), and then later on the __dec_lruvec_page_state() turns into a dec_lruvec_page_state() because it's then done outside the page table lock. The locking in the second case did do something - not locking-wise, but simply the "running under the spinlock means we are not preemptable without RT". And in the memcg case it was just plain overly wide lock regions. None of the other changes should have any real semantic meaning *apart* from just keeping ->_mapcount elevated slightly longer. > Some minor things could be improved IMHO (ENCODE_PAGE_BITS naming is > unfortunate, TLB_ZAP_RMAP could be a __bitwise type, using VM_WARN_ON > instead of VM_BUG_ON). That VM_BUG_ON() is a case of "what to do if this ever triggers?" So a WARN_ON() would be fatal too, it's some seriously bogus stuff to try to put bits in that won't fit. It probably should be removed, since the value should always be pretty much a simple constant. It was more of a "let's be careful with new code, not for production". Probably like pretty much all VM_BUG_ON's are - test code that just got left around. I considered just getting rid of ENCODE_PAGE_BITS entirely, since there is only one bit. But it was always "let's keep the option open for dirty bits etc", so I kept it, but I agree that the name isn't wonderful. And in fact I wanted the encoding to really be done by the caller (so that TLB_ZAP_RMAP wouldn't be a new argument, but the 'page' argument to __tlb_remove_page_*() would simply be an 'encoded page' pointer, but that would have caused the patch to be much bigger (and expanded the s390 side too). Which I didn't want to do. Long-term that's probably still the right thing to do, including passing the encoded pointers all the way to free_pages_and_swap_cache(). Because it's pretty disgusting how it cleans up that array in place and then does that cast to a new array type, but it is also disgusting how it traverses that array multiple times (ie free_pages_and_swap_cache() will just have *another* loop). But again, those changes would have made the patch bigger, which I didn't want at this point (and 'release_pages()' would need that clean-in-place anyway, unless we changed *that* too and made the whole page encoding be something widely available). That's also why I then went with that fairly generic "ENCODE_PAGE_BITS" name. The *use* of it right now is very very specific to just the TLB gather, and the TLB_ZAP_RMAP bit shows that in the name. But then I went for a fairly generic "encode extra bits in the page pointer" name because it felt like it might expand beyond the initial intentionally small patch in the long run. So it's a combination of "we might want to expand on this in the future" and yet also "I really want to keep the changes localized in this patch". And the two are kind of inverses of each other, which hopefully explains the seemingly disparate naming logic. Linus
On Thu, Nov 3, 2022 at 9:54 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > But again, those changes would have made the patch bigger, which I > didn't want at this point (and 'release_pages()' would need that > clean-in-place anyway, unless we changed *that* too and made the whole > page encoding be something widely available). And just to clarify: this is not just me trying to expand the reach of my patch. I'd suggest people look at mlock_pagevec(), and realize that LRU_PAGE and NEW_PAGE are both *exactly* the same kind of "encoded_page" bits that TLB_ZAP_RMAP is. Except the mlock code does *not* show that in the type system, and instead just passes a "struct page **" array around in pvec->pages, and then you'd just better know that "oh, it's not *really* just a page pointer". So I really think that the "array of encoded page pointers" thing is a generic notion that we *already* have. It's just that we've done it disgustingly in the past, and I didn't want to do that disgusting thing again. So I would hope that the nasty things that the mlock code would some day use the same page pointer encoding logic to actually make the whole "this is not a page pointer that you can use directly, it has low bits set for flags" very explicit. I am *not* sure if then the actual encoded bits would be unified. Probably not - you might have very different and distinct uses of the encode_page() thing where the bits mean different things in different contexts. Anyway, this is me just explaining the thinking behind it all. The page bit encoding is a very generic thing (well, "very generic" in this case means "has at least one other independent user"), explaining the very generic naming. But at the same time, the particular _patch_ was meant to be very targeted. So slightly schizophrenic name choices as a result. Linus
On 03.11.22 18:09, Linus Torvalds wrote: > On Thu, Nov 3, 2022 at 9:54 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> But again, those changes would have made the patch bigger, which I >> didn't want at this point (and 'release_pages()' would need that >> clean-in-place anyway, unless we changed *that* too and made the whole >> page encoding be something widely available). > > And just to clarify: this is not just me trying to expand the reach of my patch. > > I'd suggest people look at mlock_pagevec(), and realize that LRU_PAGE > and NEW_PAGE are both *exactly* the same kind of "encoded_page" bits > that TLB_ZAP_RMAP is. > > Except the mlock code does *not* show that in the type system, and > instead just passes a "struct page **" array around in pvec->pages, > and then you'd just better know that "oh, it's not *really* just a > page pointer". > > So I really think that the "array of encoded page pointers" thing is a > generic notion that we *already* have. > > It's just that we've done it disgustingly in the past, and I didn't > want to do that disgusting thing again. > > So I would hope that the nasty things that the mlock code would some > day use the same page pointer encoding logic to actually make the > whole "this is not a page pointer that you can use directly, it has > low bits set for flags" very explicit. > > I am *not* sure if then the actual encoded bits would be unified. > Probably not - you might have very different and distinct uses of the > encode_page() thing where the bits mean different things in different > contexts. > > Anyway, this is me just explaining the thinking behind it all. The > page bit encoding is a very generic thing (well, "very generic" in > this case means "has at least one other independent user"), explaining > the very generic naming. > > But at the same time, the particular _patch_ was meant to be very targeted. > > So slightly schizophrenic name choices as a result. Thanks for the explanation. I brought it up because the generic name somehow felt weird in include/asm-generic/tlb.h. Skimming over the code I'd have expected something like TLB_ENCODE_PAGE_BITS, so making the "very generic" things "very specific" as long as it lives in tlb.h :)
On Mon, Oct 31, 2022 at 11:43:30AM -0700, Linus Torvalds wrote: [...] > If people really want to see the patches in email again, I can do > that, but most of you already have, and the changes are either trivial > fixes or the s390 updates. > > For the s390 people that I've now added to the participant list maybe > the git tree is fine - and the fundamental explanation of the problem > is in that top-most commit (with the three preceding commits being > prep-work). Or that link to the thread about this all. I rather have a question to the generic part (had to master the code quotting). > static void clean_and_free_pages_and_swap_cache(struct encoded_page **pages, unsigned int nr) > { > for (unsigned int i = 0; i < nr; i++) { > struct encoded_page *encoded = pages[i]; > unsigned int flags = encoded_page_flags(encoded); > if (flags) { > /* Clean the flagged pointer in-place */ > struct page *page = encoded_page_ptr(encoded); > pages[i] = encode_page(page, 0); > > /* The flag bit being set means that we should zap the rmap */ Why TLB_ZAP_RMAP bit is not checked explicitly here, like in s390 version? (I assume, when/if ENCODE_PAGE_BITS is not TLB_ZAP_RMAP only, calling page_zap_pte_rmap() without such a check would be a bug). > page_zap_pte_rmap(page); > VM_WARN_ON_ONCE_PAGE(page_mapcount(page) < 0, page); > } > } > > /* > * Now all entries have been un-encoded, and changed to plain > * page pointers, so we can cast the 'encoded_page' array to > * a plain page array and free them > */ > free_pages_and_swap_cache((struct page **)pages, nr); > } Thanks!
On Thu, Nov 3, 2022 at 11:33 PM Alexander Gordeev <agordeev@linux.ibm.com> wrote: > > I rather have a question to the generic part (had to master the code quotting). Sure. Although now I think the series in in Andrew's -mm tree, or just about to get moved in there, so I'm not going to touch my actual branch any more. > > static void clean_and_free_pages_and_swap_cache(struct encoded_page **pages, unsigned int nr) > > { > > for (unsigned int i = 0; i < nr; i++) { > > struct encoded_page *encoded = pages[i]; > > unsigned int flags = encoded_page_flags(encoded); > > if (flags) { > > /* Clean the flagged pointer in-place */ > > struct page *page = encoded_page_ptr(encoded); > > pages[i] = encode_page(page, 0); > > > > /* The flag bit being set means that we should zap the rmap */ > > Why TLB_ZAP_RMAP bit is not checked explicitly here, like in s390 version? > (I assume, when/if ENCODE_PAGE_BITS is not TLB_ZAP_RMAP only, calling > page_zap_pte_rmap() without such a check would be a bug). No major reason. This is basically the same issue as the naming, which I touched on in https://lore.kernel.org/all/CAHk-=wiDg_1up8K4PhK4+kzPN7xJG297=nw+tvgrGn7aVgZdqw@mail.gmail.com/ and the follow-up note about how I hope the "encoded page pointer with flags" thing gets used by the mlock code some day too. IOW, there's kind of a generic "I have extra flags associated with the pointer", and then the specific "this case uses this flag", and depending on which mindset you have at the time, you might do one or the other. So in that clean_and_free_pages_and_swap_cache(), the code basically knows "I have a pointer with extra flags", and it's written that way. And that's partly historical, because it actually started with the original code tracking the dirty bit as the extra piece of information, and then transformed into this "no, the information is TLB_ZAP_RMAP". So "unsigned int flags" at one point was "bool dirty" instead, but then became more of a "I think this whole page pointer with flags is general", and the naming changed, and I had both cases in mind, and then the code is perhaps not so specifically named. I'm not sure the zap_page_range() case will ever use more than one flag, but the mlock case already has two different flags. So the "encode_page" thing is very much written to be about more than just the zap_page_range() case. But yes, that code could (and maybe should) use "if (flags & TLB_ZAP_RMAP)" to make it clear that in this case, the single flags bit is that one bit. But the "if ()" there actually does two conceptually *separate* things: it needs to clean the pointer in-place (which is regardless of "which" flag bit is set, and then it does that page_zap_pte_rmap(), which is just for the TLB_ZAP_RMAP bit. So to be really explicit about it, you'd have two different tests: one for "do I have flags that need to be cleaned up" and then an inner test for each flag. And since there is only one flag in this particular use case, it's essentially that inner test that I dropped as pointless. In contrast, in the s390 version, that bit was never encoded as a a general "flags associated with a page pointer" in the first place, so there was never any such duality. There is only TLB_ZAP_RMAP. Hope that explains the thinking. Linus
Adding Stephen to Cc, for next-20221107 alert. Adding Johannes to Cc, particularly for lock_page_memcg discussion. On Fri, 4 Nov 2022, Linus Torvalds wrote: > On Thu, Nov 3, 2022 at 11:33 PM Alexander Gordeev > <agordeev@linux.ibm.com> wrote: > > > > I rather have a question to the generic part (had to master the code quotting). > > Sure. > > Although now I think the series in in Andrew's -mm tree, or just about > to get moved in there, so I'm not going to touch my actual branch any > more. Linus, we've been exchanging about my mm/rmap.c mods in private mail, I need to raise some points about your mods here in public mail. Four topics - munlock (good), anon_vma (bad), mm-unstable (bad), lock_page_memcg (uncertain). I'm asking Andrew here to back your patchset out of mm-unstable for now, until we have its fixes in: otherwise I'm worried that next-20221107 will waste everyone's time. munlock (good) -------------- You've separated out the munlock_vma_page() from the rest of PTE remove rmap work. Worried me at first, but I think that's fine: bundling it into page_remove_rmap() was mainly so that we wouldn't forget to do it anywhere (and other rmap funcs already took vma arg). Certainly during development, I had been using page mapcount somewhere inside munlock_vma_page(), either for optimization or for sanity check, I forget; but gave up on that as unnecessary complication, and I think it became impossible with the pagevec; so not an issue now. If it were my change, I'd certainly do it by changing the name of the vma arg in page_remove_rmap() from vma to munlock_vma, not doing the munlock when that is NULL, and avoid the need for duplicating code: whereas you're very pleased with cutting out the unnecessary stuff in slimmed-down page_zap_pte_rmap(). Differing tastes (or perhaps you'll say taste versus no taste). anon_vma (bad) -------------- My first reaction to your patchset was, that it wasn't obvious to me that delaying the decrementation of mapcount is safe: I needed time to think about it. But happily, testing saved me from needing much thought. The first problem, came immediately in the first iteration of my huge tmpfs kbuild swapping loads, was BUG at mm/migrate.c:1105! VM_BUG_ON_FOLIO(folio_test_anon(src) && !folio_test_ksm(src) && !anon_vma, src); Okay, that's interesting but not necessarily fatal, we can very easily make it a recoverable condition. I didn't bother to think on that one, just patched around it. The second problem was more significant: came after nearly five hours of load, BUG NULL dereference (address 8) in down_read_trylock() in rmap_walk_anon(), while kswapd was doing a folio_referenced(). That one goes right to the heart of the matter, and instinct had been correct to worry about delaying the decrementation of mapcount, that is, extending the life of page_mapped() or folio_mapped(). See folio_lock_anon_vma_read(): folio_mapped() plays a key role in establishing the continued validity of an anon_vma. See comments above folio_get_anon_vma(), some by me but most by PeterZ IIRC. I believe what has happened is that your patchset has, very intentionally, kept the page as "folio_mapped" until after free_pgtables() does its unlink_anon_vmas(); but that is telling folio_lock_anon_vma_read() that the anon_vma is safe to use when actually it has been freed. (It looked like a page table when I peeped at it.) I'm not certain, but I think that you made page_zap_pte_rmap() handle anon as well as file, just for the righteous additional simplification; but I'm afraid that (without opening a huge anon_vma refcounting can of worms) that unification has to be reverted, and anon left to go the same old way it did before. I didn't look into whether reverting one of your patches would achieve that, I just adjusted the code in zap_pte_range() to go the old way for PageAnon; and that has been running successfully, hitting neither BUG, for 15 hours now. mm-unstable (bad) ----------------- Aside from that PageAnon issue, mm-unstable is in an understandably bad state because you could not have foreseen my subpages_mapcount addition to page_remove_rmap(). page_zap_pte_rmap() now needs to handle the PageCompound (but not the "compound") case too. I rushed you and akpm an emergency patch for that on Friday night, but you, let's say, had reservations about it. So I haven't posted it, and while the PageAnon issue remains, I think your patchset has to be removed from mm-unstable and linux-next anyway. What happens to mm-unstable with page_zap_pte_rmap() not handling subpages_mapcount? In my CONFIG_TRANSPARENT_HUGEPAGE_ALWAYS=y case, I get a "Bad page state" even before reaching first login after boot, "page dumped because: nonzero subpages_mapcount". Yes, I make it worse for myself by having a BUG() in bad_page(); others will limp along with more and more of those "Bad page" messages. lock_page_memcg (uncertain) --------------------------- Johannes, please help! Linus has got quite upset by lock_page_memcg(), its calls from mm/rmap.c anyway, and most particularly by the way in which it is called at the start of page_remove_rmap(), before anyone's critical atomic_add_negative(), yet its use is to guarantee the stability of page memcg while doing the stats updates, done only when atomic_add_negative() says so. I do have one relevant insight on this. It (or its antecedents under other names) date from the days when we did "reparenting" of memcg charges from an LRU: and in those days the lock_page_memcg() before mapcount adjustment was vital, to pair with the uses of folio_mapped() or page_mapped() in mem_cgroup_move_account() - those "mapped" checks are precisely around the stats which the rmap functions affect. But nowadays mem_cgroup_move_account() is only called, with page table lock held, on matching pages found in a task's page table: so its "mapped" checks are redundant - I've sometimes thought in the past of removing them, but held back, because I always have the notion (not hope!) that "reparenting" may one day be brought back from the grave. I'm too out of touch with memcg to know where that notion stands today. I've gone through a multiverse of opinions on those lock_page_memcg()s in the last day: I currently believe that Linus is right, that the lock_page_memcg()s could and should be moved just before the stats updates. But I am not 100% certain of that - is there still some reason why it's important that the page memcg at the instant of the critical mapcount transition be kept unchanged until the stats are updated? I've tried running scenarios through my mind but given up. (And note that the answer might be different with Linus's changes than without them: since he delays the mapcount decrementation until long after pte was removed and page table lock dropped). And I do wish that we could settle this lock_page_memcg() question in an entirely separate patch: as it stands, page_zap_pte_rmap() gets to benefit from Linus's insight (or not), and all the other rmap functions carry on with the mis?placed lock_page_memcg() as before. Let's press Send, Hugh
[ Editing down to just the bare-bones problem cases ] On Sun, Nov 6, 2022 at 1:06 PM Hugh Dickins <hughd@google.com> wrote: > > anon_vma (bad) > -------------- > > See folio_lock_anon_vma_read(): folio_mapped() plays a key role in > establishing the continued validity of an anon_vma. See comments > above folio_get_anon_vma(), some by me but most by PeterZ IIRC. > > I believe what has happened is that your patchset has, very intentionally, > kept the page as "folio_mapped" until after free_pgtables() does its > unlink_anon_vmas(); but that is telling folio_lock_anon_vma_read() > that the anon_vma is safe to use when actually it has been freed. > (It looked like a page table when I peeped at it.) > > I'm not certain, but I think that you made page_zap_pte_rmap() handle > anon as well as file, just for the righteous additional simplification; > but I'm afraid that (without opening a huge anon_vma refcounting can of > worms) that unification has to be reverted, and anon left to go the > same old way it did before. Indeed. I made them separate initially, just because the only case that mattered for the dirty bit was the file-mapped case. But then the two functions ended up being basically the identical function, so I unified them again. But the anonvma lifetime issue looks very real, and so doing the "delay rmap only for file mappings" seems sane. In fact, I wonder if we should delay it only for *dirty* file mappings, since it doesn't matter for the clean case. Hmm. I already threw away my branch (since Andrew picked the patches up), so a question for Andrew: do you want me to re-do the branch entirely, or do you want me to just send you an incremental patch? To make for minimal changes, I'd drop the 're-unification' patch, and then small updates to the zap_pte_range() code to keep the anon (and possibly non-dirty) case synchronous. And btw, this one is interesting: for anonymous (and non-dirty file-mapped) patches, we actually can end up delaying the final page free (and the rmap zapping) all the way to "tlb_finish_mmu()". Normally we still have the vma's all available, but yes, free_pgtables() can and does happen before the final TLB flush. The file-mapped dirty case doesn't have that issue - not just because it doesn't have an anonvma at all, but because it also does that "force_flush" thing that just measn that the page freeign never gets delayed that far in the first place. > mm-unstable (bad) > ----------------- > Aside from that PageAnon issue, mm-unstable is in an understandably bad > state because you could not have foreseen my subpages_mapcount addition > to page_remove_rmap(). page_zap_pte_rmap() now needs to handle the > PageCompound (but not the "compound") case too. I rushed you and akpm > an emergency patch for that on Friday night, but you, let's say, had > reservations about it. So I haven't posted it, and while the PageAnon > issue remains, I think your patchset has to be removed from mm-unstable > and linux-next anyway. So I think I'm fine with your patch, I just want to move the memcg accounting to outside of it. I can re-do my series on top of mm-unstable, I guess. That's probably the easiest way to handle this all. Andrew - can you remove those patches again, and I'll create a new series for you? Linus
On Sun, 6 Nov 2022 14:34:51 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > mm-unstable (bad) > > ----------------- > > Aside from that PageAnon issue, mm-unstable is in an understandably bad > > state because you could not have foreseen my subpages_mapcount addition > > to page_remove_rmap(). page_zap_pte_rmap() now needs to handle the > > PageCompound (but not the "compound") case too. I rushed you and akpm > > an emergency patch for that on Friday night, but you, let's say, had > > reservations about it. So I haven't posted it, and while the PageAnon > > issue remains, I think your patchset has to be removed from mm-unstable > > and linux-next anyway. > > So I think I'm fine with your patch, I just want to move the memcg > accounting to outside of it. > > I can re-do my series on top of mm-unstable, I guess. That's probably > the easiest way to handle this all. > > Andrew - can you remove those patches again, and I'll create a new > series for you? Yes, I've removed both serieses and shall push the tree out within half an hour from now.
Hi all, On Sun, 6 Nov 2022 15:14:16 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > Yes, I've removed both serieses and shall push the tree out within half > an hour from now. And I have picked up the new version for today's linux-next. Thanks all.
Hi Hugh! On Sun, Nov 06, 2022 at 01:06:19PM -0800, Hugh Dickins wrote: > See folio_lock_anon_vma_read(): folio_mapped() plays a key role in > establishing the continued validity of an anon_vma. See comments > above folio_get_anon_vma(), some by me but most by PeterZ IIRC. Ohhh, you're quite right. Unfortunately I seem to have completely forgotten about that :-(
On Sun, Nov 6, 2022 at 3:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > Yes, I've removed both serieses and shall push the tree out within half > an hour from now. Oh, can you please just put Hugh's series back in? I don't love the mapcount changes, but I'm going to re-do my parts so that there is no clash with them. And while I'd much rather see the mapcounts done another way, that's a completely separate issue. Linus
Hello, On Sun, Nov 06, 2022 at 01:06:19PM -0800, Hugh Dickins wrote: > lock_page_memcg (uncertain) > --------------------------- > Johannes, please help! Linus has got quite upset by lock_page_memcg(), > its calls from mm/rmap.c anyway, and most particularly by the way > in which it is called at the start of page_remove_rmap(), before > anyone's critical atomic_add_negative(), yet its use is to guarantee > the stability of page memcg while doing the stats updates, done only > when atomic_add_negative() says so. As you mentioned, the pte lock historically wasn't always taken on the move side. And so the reason lock_page_memcg() covers the mapcount update is that move_account() needs to atomically see either a) the page is mapped and counted, or b) unmapped and uncounted. If we lock after mapcountdec, move_account() could miss pending updates that need transferred, and it would break the scheme breaks thusly: memcg1->nr_mapped = 1 memcg2->nr_mapped = 0 page_remove_rmap: mem_cgroup_move_account(): if atomic_add_negative(page->mapcount): lock_page_memcg() if page->mapcount: // NOT TAKEN memcg1->nr_mapped-- memcg2->nr_mapped++ page->memcg = memcg2 unlock_page_memcg() lock_page_memcg() page->memcg->nr_mapped-- // UNDERFLOW memcg2->nr_mapped unlock_page_memcg() > I do have one relevant insight on this. It (or its antecedents under > other names) date from the days when we did "reparenting" of memcg > charges from an LRU: and in those days the lock_page_memcg() before > mapcount adjustment was vital, to pair with the uses of folio_mapped() > or page_mapped() in mem_cgroup_move_account() - those "mapped" checks > are precisely around the stats which the rmap functions affect. > > But nowadays mem_cgroup_move_account() is only called, with page table > lock held, on matching pages found in a task's page table: so its > "mapped" checks are redundant - I've sometimes thought in the past of > removing them, but held back, because I always have the notion (not > hope!) that "reparenting" may one day be brought back from the grave. > I'm too out of touch with memcg to know where that notion stands today. > > I've gone through a multiverse of opinions on those lock_page_memcg()s > in the last day: I currently believe that Linus is right, that the > lock_page_memcg()s could and should be moved just before the stats > updates. But I am not 100% certain of that - is there still some > reason why it's important that the page memcg at the instant of the > critical mapcount transition be kept unchanged until the stats are > updated? I've tried running scenarios through my mind but given up. Okay, I think there are two options. - If we don't want to codify the pte lock requirement on the move side, then moving the lock_page_memcg() like that would break the locking scheme, as per above. - If we DO want to codify the pte lock requirement, we should just remove the lock_page_memcg() altogether, as it's fully redundant. I'm leaning toward the second option. If somebody brings back reparenting they can bring the lock back along with it. [ If it's even still necessary by then. It's conceivable reparenting is brought back only for cgroup2, where the race wouldn't matter because of the hierarchical stats. The reparenting side wouldn't have to move page state to the parent - it's already there. And whether rmap would see the dying child or the parent doesn't matter much either: the parent will see the update anyway, directly or recursively, and we likely don't care to balance the books on a dying cgroup. It's then just a matter of lifetime - which should be guaranteed also, as long as the pte lock prevents an RCU quiescent state. ] So how about something like below? UNTESTED, just for illustration. This is cgroup1 code, which I haven't looked at too closely in a while. If you can't spot an immediate hole in it, I'd go ahead and test it and send a proper patch. --- From 88a32b1b5737630fb981114f6333d8fd057bd8e9 Mon Sep 17 00:00:00 2001 From: Johannes Weiner <hannes@cmpxchg.org> Date: Mon, 7 Nov 2022 12:05:09 -0500 Subject: [PATCH] mm: remove lock_page_memcg() from rmap rmap changes (mapping and unmapping) of a page currently take lock_page_memcg() to serialize 1) update of the mapcount and the cgroup mapped counter with 2) cgroup moving the page and updating the old cgroup and the new cgroup counters based on page_mapped(). Before b2052564e66d ("mm: memcontrol: continue cache reclaim from offlined groups"), we used to reassign all pages that could be found on a cgroup's LRU list on deletion - something that rmap didn't naturally serialize against. Since that commit, however, the only pages that get moved are those mapped into page tables of a task that's being migrated. In that case, the pte lock is always held (and we know the page is mapped), which keeps rmap changes at bay already. The additional lock_page_memcg() by rmap is redundant. Remove it. NOT-Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/memcontrol.c | 27 ++++++++++++--------------- mm/rmap.c | 30 ++++++++++++------------------ 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2d8549ae1b30..f7716e9038e9 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5666,7 +5666,10 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma, * @from: mem_cgroup which the page is moved from. * @to: mem_cgroup which the page is moved to. @from != @to. * - * The caller must make sure the page is not on LRU (isolate_page() is useful.) + * This function acquires folio_lock() and folio_lock_memcg(). The + * caller must exclude all other possible ways of accessing + * page->memcg, such as LRU isolation (to lock out isolation) and + * having the page mapped and pte-locked (to lock out rmap). * * This function doesn't do "charge" to new cgroup and doesn't do "uncharge" * from old cgroup. @@ -5685,6 +5688,7 @@ static int mem_cgroup_move_account(struct page *page, VM_BUG_ON(from == to); VM_BUG_ON_FOLIO(folio_test_lru(folio), folio); VM_BUG_ON(compound && !folio_test_large(folio)); + VM_WARN_ON_ONCE(!folio_mapped(folio)); /* * Prevent mem_cgroup_migrate() from looking at @@ -5705,30 +5709,23 @@ static int mem_cgroup_move_account(struct page *page, folio_memcg_lock(folio); if (folio_test_anon(folio)) { - if (folio_mapped(folio)) { - __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages); - __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages); - if (folio_test_transhuge(folio)) { - __mod_lruvec_state(from_vec, NR_ANON_THPS, - -nr_pages); - __mod_lruvec_state(to_vec, NR_ANON_THPS, - nr_pages); - } + __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages); + __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages); + if (folio_test_transhuge(folio)) { + __mod_lruvec_state(from_vec, NR_ANON_THPS, -nr_pages); + __mod_lruvec_state(to_vec, NR_ANON_THPS, nr_pages); } } else { __mod_lruvec_state(from_vec, NR_FILE_PAGES, -nr_pages); __mod_lruvec_state(to_vec, NR_FILE_PAGES, nr_pages); + __mod_lruvec_state(from_vec, NR_FILE_MAPPED, -nr_pages); + __mod_lruvec_state(to_vec, NR_FILE_MAPPED, nr_pages); if (folio_test_swapbacked(folio)) { __mod_lruvec_state(from_vec, NR_SHMEM, -nr_pages); __mod_lruvec_state(to_vec, NR_SHMEM, nr_pages); } - if (folio_mapped(folio)) { - __mod_lruvec_state(from_vec, NR_FILE_MAPPED, -nr_pages); - __mod_lruvec_state(to_vec, NR_FILE_MAPPED, nr_pages); - } - if (folio_test_dirty(folio)) { struct address_space *mapping = folio_mapping(folio); diff --git a/mm/rmap.c b/mm/rmap.c index 2ec925e5fa6a..60c31375f274 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1197,11 +1197,6 @@ void page_add_anon_rmap(struct page *page, bool compound = flags & RMAP_COMPOUND; bool first; - if (unlikely(PageKsm(page))) - lock_page_memcg(page); - else - VM_BUG_ON_PAGE(!PageLocked(page), page); - if (compound) { atomic_t *mapcount; VM_BUG_ON_PAGE(!PageLocked(page), page); @@ -1217,19 +1212,19 @@ void page_add_anon_rmap(struct page *page, if (first) { int nr = compound ? thp_nr_pages(page) : 1; /* - * We use the irq-unsafe __{inc|mod}_zone_page_stat because - * these counters are not modified in interrupt context, and - * pte lock(a spinlock) is held, which implies preemption - * disabled. + * We use the irq-unsafe __{inc|mod}_zone_page_stat + * because these counters are not modified in + * interrupt context, and pte lock(a spinlock) is + * held, which implies preemption disabled. + * + * The pte lock also stabilizes page->memcg wrt + * mem_cgroup_move_account(). */ if (compound) __mod_lruvec_page_state(page, NR_ANON_THPS, nr); __mod_lruvec_page_state(page, NR_ANON_MAPPED, nr); } - if (unlikely(PageKsm(page))) - unlock_page_memcg(page); - /* address might be in next vma when migration races vma_adjust */ else if (first) __page_set_anon_rmap(page, vma, address, @@ -1290,7 +1285,6 @@ void page_add_file_rmap(struct page *page, int i, nr = 0; VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page); - lock_page_memcg(page); if (compound && PageTransHuge(page)) { int nr_pages = thp_nr_pages(page); @@ -1311,6 +1305,7 @@ void page_add_file_rmap(struct page *page, if (nr == nr_pages && PageDoubleMap(page)) ClearPageDoubleMap(page); + /* The pte lock stabilizes page->memcg */ if (PageSwapBacked(page)) __mod_lruvec_page_state(page, NR_SHMEM_PMDMAPPED, nr_pages); @@ -1328,7 +1323,6 @@ void page_add_file_rmap(struct page *page, out: if (nr) __mod_lruvec_page_state(page, NR_FILE_MAPPED, nr); - unlock_page_memcg(page); mlock_vma_page(page, vma, compound); } @@ -1356,6 +1350,7 @@ static void page_remove_file_rmap(struct page *page, bool compound) } if (!atomic_add_negative(-1, compound_mapcount_ptr(page))) goto out; + /* The pte lock stabilizes page->memcg */ if (PageSwapBacked(page)) __mod_lruvec_page_state(page, NR_SHMEM_PMDMAPPED, -nr_pages); @@ -1423,8 +1418,6 @@ static void page_remove_anon_compound_rmap(struct page *page) void page_remove_rmap(struct page *page, struct vm_area_struct *vma, bool compound) { - lock_page_memcg(page); - if (!PageAnon(page)) { page_remove_file_rmap(page, compound); goto out; @@ -1443,6 +1436,9 @@ void page_remove_rmap(struct page *page, * We use the irq-unsafe __{inc|mod}_zone_page_stat because * these counters are not modified in interrupt context, and * pte lock(a spinlock) is held, which implies preemption disabled. + * + * The pte lock also stabilizes page->memcg wrt + * mem_cgroup_move_account(). */ __dec_lruvec_page_state(page, NR_ANON_MAPPED); @@ -1459,8 +1455,6 @@ void page_remove_rmap(struct page *page, * faster for those pages still in swapcache. */ out: - unlock_page_memcg(page); - munlock_vma_page(page, vma, compound); }
On Mon, Nov 7, 2022 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > - If we DO want to codify the pte lock requirement, we should just > remove the lock_page_memcg() altogether, as it's fully redundant. > > I'm leaning toward that second option. The thing is, that's very much the case we do *not* want. We need to delay the rmap removal until at least after the TLB flush. At least for dirty filemapped pages - because the page cleaning needs to see that they exists as mapped entities until all CPU's have *actually* dropped them. Now, we do the TLB flush still under the page table lock, so we could still then do the rmap removal before dropping the lock. But it would be much cleaner from the TLB flushing standpoint to delay it until the page freeing, which ends up being delayed until after the lock is dropped. That said, if always doing the rmap removal under the page table lock means that that memcg lock can just be deleted in that whole path, I will certainly bow to _that_ simplification instead, and just handle the dirty pages after the TLB flush but before the page table drop. Linus
On Mon, 7 Nov 2022 08:19:24 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, Nov 6, 2022 at 3:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > Yes, I've removed both serieses and shall push the tree out within half > > an hour from now. > > Oh, can you please just put Hugh's series back in? > Done, all pushed out to git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm mm-unstable.
Hi all, On Mon, 7 Nov 2022 15:02:42 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 7 Nov 2022 08:19:24 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Done, all pushed out to > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm mm-unstable. Will be in today's linux-next.
On Mon, Nov 7, 2022 at 12:29 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > That said, if always doing the rmap removal under the page table lock > means that that memcg lock can just be deleted in that whole path, I > will certainly bow to _that_ simplification instead, and just handle > the dirty pages after the TLB flush but before the page table drop. Ok, so I think I have a fairly clean way to do this. Let me try to make that series look reasonable, although it might be until tomorrow. I'll need to massage my mess into not just prettier code, but a sane history. Linus
On Mon, Nov 7, 2022 at 3:47 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Ok, so I think I have a fairly clean way to do this. > > Let me try to make that series look reasonable, although it might be > until tomorrow. I'll need to massage my mess into not just prettier > code, but a sane history. Ugh. Ok, so massaging it into a saner form and splitting it out into a pretty history took a lot longer than writing the initial ugly "something like this". Anyway, the end result looks very different from the previous series, since I very consciously tried to keep away from rmap changes to not clash with Hugh's work, and also made sure we still do the page_remove_rmap() under the page table lock, just *after* the TLB flush. It does share some basic stuff - I'm still using that "struct encoded_page" thing, I just moved it into a separate commit, and moved it to where it conceptually belongs (I'd love to say that I also made the mlock.c code use it, but I didn't - that 'pagevec' thing is a pretty pointless abstraction and I didn't want to fight it). So you'll see some familiar support structures and scaffolding, but the actual approach to zap_pte_range() is very different. The approach taken to the "s390 is very different" issue is also completely different. It should actually be better for s390: we used to cause that "force_flush" whenever we hit a dirty shared page, and s390 doesn't care. The new model makes that "s390 doesn't care" be part of the whole design, so now s390 treats dirty shared pages basically as if they weren't anything special at all. Which they aren't on s390. But, as with the previous case, I don't even try to cross-compile for s390, so my attempts at handling s390 well are just that: attempts. The code may be broken. Of course, the code may be broken on x86-64 too, but at least there I've compiled it and am running it right now. Oh, and because I copied large parts of the commit message from the previous approach (because the problem description is the same), I noticed that I also kept the "Acked-by:". Those are bogus, because the code is sufficiently different that any previous acks are just not valid any more, but I just hadn't fixed that yet. The meat of it all is in that last patch, the rest is just type system cleanups to prep for it. But it was also that last patch that I spent hours on just tweaking it to look sensible. The *code* was pretty easy. Making it have sensible naming and a sensible abstraction interface that worked well for s390 too, that was 90% of the effort. But also I hope the end result is reasonably easy to review as a result. I'm sending this out because I'm stepping away from the keyboard, because that whole "let's massage it into something lagible" was really somewhat exhausting. You don't see all the small side turns it took only to go "that's ugly, let's try again" ;) Anybody interested in giving this a peek? (Patch 2/4 might make some people pause. It's fairly small and simple. It's effective and makes it easy to do some of the later changes. And it's also quite different from our usual model. It was "inspired" by the signed-vs-unsigned char thread from a few weeks ago. But patch 4/4 is the one that matters). Linus
On Mon, Nov 7, 2022 at 8:28 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I'm sending this out because I'm stepping away from the keyboard, > because that whole "let's massage it into something legible" was > really somewhat exhausting. You don't see all the small side turns it > took only to go "that's ugly, let's try again" ;) Ok, I actually sent the individual patches with 'git-send-email', although I only sent them to the mailing list and to people that were mentioned in the commit descriptions. I hope that makes review easier. See https://lore.kernel.org/all/20221108194139.57604-1-torvalds@linux-foundation.org for the series if you weren't mentioned and are interested. Oh, and because I decided to just use the email in this thread as the reference and cover letter, it turns out that this all confuses 'b4', because it actually walks up the whole thread all the way to the original 13-patch series by PeterZ that started this whole discussion. I've seen that before with other peoples patch series, but now that it happened to my own, I'm cc'ing Konstantine here too to see if there's some magic for b4 to say "look, I pointed you to a msg-id that is clearly a new series, don't walk all the way up and then take patches from a completely different one. Oh well. I guess I should just have not been lazy and done a cover-letter and a whole new thread. My bad. Konstantin, please help me look like less of a tool? Linus
On Tue, Nov 08, 2022 at 11:56:13AM -0800, Linus Torvalds wrote: > On Mon, Nov 7, 2022 at 8:28 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > I'm sending this out because I'm stepping away from the keyboard, > > because that whole "let's massage it into something legible" was > > really somewhat exhausting. You don't see all the small side turns it > > took only to go "that's ugly, let's try again" ;) > > Ok, I actually sent the individual patches with 'git-send-email', > although I only sent them to the mailing list and to people that were > mentioned in the commit descriptions. > > I hope that makes review easier. > > See > > https://lore.kernel.org/all/20221108194139.57604-1-torvalds@linux-foundation.org > > for the series if you weren't mentioned and are interested. > > Oh, and because I decided to just use the email in this thread as the > reference and cover letter, it turns out that this all confuses 'b4', > because it actually walks up the whole thread all the way to the > original 13-patch series by PeterZ that started this whole discussion. > > I've seen that before with other peoples patch series, but now that it > happened to my own, I'm cc'ing Konstantine here too to see if there's > some magic for b4 to say "look, I pointed you to a msg-id that is > clearly a new series, don't walk all the way up and then take patches > from a completely different one. Yes, --no-parent. It's slightly more complicated in your case because the patches aren't threaded to the first patch/cover letter, but you can choose an arbitrary msgid upthread and tell b4 to ignore anything that came before it. E.g.: b4 am -o/tmp --no-parent 20221108194139.57604-1-torvalds@linux-foundation.org -K
On Tue, Nov 8, 2022 at 12:03 PM Konstantin Ryabitsev <mricon@kernel.org> wrote: > > Yes, --no-parent. Ahh, that's new. I guess I need to update my ancient b4-0.8.0 install.. But yes, with that, and the manual parent lookup (because otherwise "--no-parent" will fetch *just* the patch itself, not even walking up the single parent chain). it works. Maybe a "--single-parent" or "--deadbeat-parent" option would be a good idea? Anyway, with a more recent b4 version, the command b4 am --no-parent CAHk-=wh6MxaCA4pXpt1F5Bn2__6MxCq0Dr-rES4i=MOL9ibjpg@mail.gmail.com gets that series and only that series. Linus
--- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -260,15 +260,12 @@ static inline pte_t ptep_get(pte_t *ptep #ifdef CONFIG_GUP_GET_PTE_LOW_HIGH /* - * WARNING: only to be used in the get_user_pages_fast() implementation. - * - * With get_user_pages_fast(), we walk down the pagetables without taking any - * locks. For this we would like to load the pointers atomically, but sometimes - * that is not possible (e.g. without expensive cmpxchg8b on x86_32 PAE). What - * we do have is the guarantee that a PTE will only either go from not present - * to present, or present to not present or both -- it will not switch to a - * completely different present page without a TLB flush in between; something - * that we are blocking by holding interrupts off. + * For walking the pagetables without holding any locks. Some architectures + * (eg x86-32 PAE) cannot load the entries atomically without using expensive + * instructions. We are guaranteed that a PTE will only either go from not + * present to present, or present to not present -- it will not switch to a + * completely different present page without a TLB flush inbetween; which we + * are blocking by holding interrupts off. * * Setting ptes from not present to present goes: *
Improve the comment. Suggested-by: Matthew Wilcox <willy@infradead.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- include/linux/pgtable.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)