Hi,
This patchset enables MOVE_PMD/MOVE_PUD support on power. This requires
the platform to support updating higher-level page tables without
updating page table entries. This also needs to invalidate the Page Walk
Cache on architecture supporting the same.
Changes from v4:
* Change function name and arguments based on review feedback.
Changes from v3:
* Fix build error reported by kernel test robot
* Address review feedback.
Changes from v2:
* switch from using mmu_gather to flush_pte_tlb_pwc_range()
Changes from v1:
* Rebase to recent upstream
* Fix build issues with tlb_gather_mmu changes
Aneesh Kumar K.V (9):
selftest/mremap_test: Update the test to handle pagesize other than 4K
selftest/mremap_test: Avoid crash with static build
mm/mremap: Use pmd/pud_poplulate to update page table entries
powerpc/mm/book3s64: Fix possible build error
powerpc/mm/book3s64: Update tlb flush routines to take a page walk
cache flush argument
mm/mremap: Use range flush that does TLB and page walk cache flush
mm/mremap: Move TLB flush outside page table lock
mm/mremap: Allow arch runtime override
powerpc/mm: Enable move pmd/pud
.../include/asm/book3s/64/tlbflush-radix.h | 19 +--
arch/powerpc/include/asm/book3s/64/tlbflush.h | 29 ++++-
arch/powerpc/include/asm/tlb.h | 6 +
arch/powerpc/mm/book3s64/radix_hugetlbpage.c | 4 +-
arch/powerpc/mm/book3s64/radix_tlb.c | 55 ++++----
arch/powerpc/platforms/Kconfig.cputype | 2 +
mm/mremap.c | 40 ++++--
tools/testing/selftests/vm/mremap_test.c | 118 ++++++++++--------
8 files changed, 170 insertions(+), 103 deletions(-)
--
2.30.2
Instead of hardcoding 4K page size fetch it using sysconf(). For the performance
measurements test still assume 2M and 1G are hugepage sizes.
Reviewed-by: Kalesh Singh <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
tools/testing/selftests/vm/mremap_test.c | 113 ++++++++++++-----------
1 file changed, 61 insertions(+), 52 deletions(-)
With a large mmap map size, we can overlap with the text area and using
MAP_FIXED results in unmapping that area. Switch to MAP_FIXED_NOREPLACE
and handle the EEXIST error.
Reviewed-by: Kalesh Singh <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
tools/testing/selftests/vm/mremap_test.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/mremap.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -257,9 +258,8 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,pmd_clear(old_pmd);VM_BUG_ON(!pmd_none(*new_pmd));+pmd_populate(mm,new_pmd,(pgtable_t)pmd_page_vaddr(pmd));-/* Set the new pmd */-set_pmd_at(mm,new_addr,new_pmd,pmd);flush_tlb_range(vma,old_addr,old_addr+PMD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
@@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,VM_BUG_ON(!pud_none(*new_pud));-/* Set the new pud */-set_pud_at(mm,new_addr,new_pud,pud);+pud_populate(mm,new_pud,(pmd_t*)pud_page_vaddr(pud));flush_tlb_range(vma,old_addr,old_addr+PUD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
Hi Aneesh,
On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
quoted hunk
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/mremap.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -257,9 +258,8 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,pmd_clear(old_pmd);VM_BUG_ON(!pmd_none(*new_pmd));+pmd_populate(mm,new_pmd,(pgtable_t)pmd_page_vaddr(pmd));-/* Set the new pmd */-set_pmd_at(mm,new_addr,new_pmd,pmd);flush_tlb_range(vma,old_addr,old_addr+PMD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
@@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,VM_BUG_ON(!pud_none(*new_pud));-/* Set the new pud */-set_pud_at(mm,new_addr,new_pud,pud);+pud_populate(mm,new_pud,(pmd_t*)pud_page_vaddr(pud));flush_tlb_range(vma,old_addr,old_addr+PUD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
--
2.30.2
This commit causes my WSL2 VM to close when compiling something memory
intensive, such as an x86_64_defconfig + CONFIG_LTO_CLANG_FULL=y kernel
or LLVM/Clang. Unfortunately, I do not have much further information to
provide since I do not see any sort of splat in dmesg right before it
closes and I have found zero information about getting the previous
kernel message in WSL2 (custom init so no systemd or anything).
The config file is the stock one from Microsoft:
https://github.com/microsoft/WSL2-Linux-Kernel/blob/a571dc8cedc8e0e56487c0dc93243e0b5db8960a/Microsoft/config-wsl
I have attached my .config anyways, which includes CONFIG_DEBUG_VM,
which does not appear to show anything out of the ordinary. I have also
attached a dmesg just in case anything sticks out. I am happy to provide
any additional information or perform additional debugging steps as
needed.
Cheers,
Nathan
$ git bisect log
# bad: [cd557f1c605fc5a2c0eb0b540610f50dc67dd849] Add linux-next specific files for 20210514
# good: [315d99318179b9cd5077ccc9f7f26a164c9fa998] Merge tag 'pm-5.13-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect start 'cd557f1c605fc5a2c0eb0b540610f50dc67dd849' '315d99318179b9cd5077ccc9f7f26a164c9fa998'
# good: [9634d7cb3c506ae886a5136d12b4af696b9cee8a] Merge remote-tracking branch 'drm-misc/for-linux-next'
git bisect good 9634d7cb3c506ae886a5136d12b4af696b9cee8a
# good: [294636a24ae819a7caf0807d05d8eb5b964ec06f] Merge remote-tracking branch 'rcu/rcu/next'
git bisect good 294636a24ae819a7caf0807d05d8eb5b964ec06f
# good: [cb753d0611f912439c8e814f4254d15fa8fa1d75] Merge remote-tracking branch 'gpio-brgl/gpio/for-next'
git bisect good cb753d0611f912439c8e814f4254d15fa8fa1d75
# bad: [b1e7389449084b74a044a70860c6a1c7466781cb] lib/string_helpers: switch to use BIT() macro
git bisect bad b1e7389449084b74a044a70860c6a1c7466781cb
# bad: [bf5570ed0654a21000e5dad9243ea1ba30bfe208] kasan: use dump_stack_lvl(KERN_ERR) to print stacks
git bisect bad bf5570ed0654a21000e5dad9243ea1ba30bfe208
# good: [4a292ff7a819404039588c7a9af272aca22c869e] fixup! mm: gup: pack has_pinned in MMF_HAS_PINNED
git bisect good 4a292ff7a819404039588c7a9af272aca22c869e
# good: [5ed68c90c7fb884c3c493d5529aca79dcf125848] mm: memcontrol: move obj_cgroup_uncharge_pages() out of css_set_lock
git bisect good 5ed68c90c7fb884c3c493d5529aca79dcf125848
# good: [f96ae2c1e63b71134e216e9940df3f2793a9a4b1] mm/memory.c: fix comment of finish_mkwrite_fault()
git bisect good f96ae2c1e63b71134e216e9940df3f2793a9a4b1
# bad: [5b0a28a7f9f5fdc2fe5a5e2cce7ea17b98e5eaeb] mm/mremap: use range flush that does TLB and page walk cache flush
git bisect bad 5b0a28a7f9f5fdc2fe5a5e2cce7ea17b98e5eaeb
# bad: [dbee97d1f49a2f2f1f5c26bf15151cc998572e89] mm/mremap: use pmd/pud_poplulate to update page table entries
git bisect bad dbee97d1f49a2f2f1f5c26bf15151cc998572e89
# good: [c4c8a76d96a7d38d1ec8732e3f852418d18a7424] selftest/mremap_test: avoid crash with static build
git bisect good c4c8a76d96a7d38d1ec8732e3f852418d18a7424
# first bad commit: [dbee97d1f49a2f2f1f5c26bf15151cc998572e89] mm/mremap: use pmd/pud_poplulate to update page table entries
Hi Aneesh,
On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
quoted
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/mremap.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -257,9 +258,8 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,pmd_clear(old_pmd);VM_BUG_ON(!pmd_none(*new_pmd));+pmd_populate(mm,new_pmd,(pgtable_t)pmd_page_vaddr(pmd));-/* Set the new pmd */-set_pmd_at(mm,new_addr,new_pmd,pmd);flush_tlb_range(vma,old_addr,old_addr+PMD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
@@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,VM_BUG_ON(!pud_none(*new_pud));-/* Set the new pud */-set_pud_at(mm,new_addr,new_pud,pud);+pud_populate(mm,new_pud,(pmd_t*)pud_page_vaddr(pud));flush_tlb_range(vma,old_addr,old_addr+PUD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
--
2.30.2
This commit causes my WSL2 VM to close when compiling something memory
intensive, such as an x86_64_defconfig + CONFIG_LTO_CLANG_FULL=y kernel
or LLVM/Clang. Unfortunately, I do not have much further information to
provide since I do not see any sort of splat in dmesg right before it
closes and I have found zero information about getting the previous
kernel message in WSL2 (custom init so no systemd or anything).
The config file is the stock one from Microsoft:
https://github.com/microsoft/WSL2-Linux-Kernel/blob/a571dc8cedc8e0e56487c0dc93243e0b5db8960a/Microsoft/config-wsl
I have attached my .config anyways, which includes CONFIG_DEBUG_VM,
which does not appear to show anything out of the ordinary. I have also
attached a dmesg just in case anything sticks out. I am happy to provide
any additional information or perform additional debugging steps as
needed.
Hi Aneesh,
On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
quoted
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/mremap.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
@@ -257,9 +258,8 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,pmd_clear(old_pmd);VM_BUG_ON(!pmd_none(*new_pmd));+pmd_populate(mm,new_pmd,(pgtable_t)pmd_page_vaddr(pmd));-/* Set the new pmd */-set_pmd_at(mm,new_addr,new_pmd,pmd);flush_tlb_range(vma,old_addr,old_addr+PMD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
@@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,VM_BUG_ON(!pud_none(*new_pud));-/* Set the new pud */-set_pud_at(mm,new_addr,new_pud,pud);+pud_populate(mm,new_pud,(pmd_t*)pud_page_vaddr(pud));flush_tlb_range(vma,old_addr,old_addr+PUD_SIZE);if(new_ptl!=old_ptl)spin_unlock(new_ptl);
--
2.30.2
This commit causes my WSL2 VM to close when compiling something memory
intensive, such as an x86_64_defconfig + CONFIG_LTO_CLANG_FULL=y kernel
or LLVM/Clang. Unfortunately, I do not have much further information to
provide since I do not see any sort of splat in dmesg right before it
closes and I have found zero information about getting the previous
kernel message in WSL2 (custom init so no systemd or anything).
The config file is the stock one from Microsoft:
https://github.com/microsoft/WSL2-Linux-Kernel/blob/a571dc8cedc8e0e56487c0dc93243e0b5db8960a/Microsoft/config-wsl
I have attached my .config anyways, which includes CONFIG_DEBUG_VM,
which does not appear to show anything out of the ordinary. I have also
attached a dmesg just in case anything sticks out. I am happy to provide
any additional information or perform additional debugging steps as
needed.
Can you try this change?
Thank you for the quick diff! This resolves my issue.
Tested-by: Nathan Chancellor <nathan@kernel.org>
From: Peter Xu <peterx@redhat.com> Date: 2021-05-20 02:18:50
On Wed, May 19, 2021 at 10:16:07AM +0530, Aneesh Kumar K.V wrote:
quoted
On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
quoted
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
I reported this issue today somewhere else:
https://lore.kernel.org/linux-mm/YKVemB5DuSqLFmmz@t490s/
And came to this same line after the bisection.
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
--
Peter Xu
On Wed, May 19, 2021 at 10:16:07AM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
quoted
pmd/pud_populate is the right interface to be used to set the respective
page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
can only be used to set a hugepage PTE. Since we are not setting up a hugepage
PTE here, use the pmd/pud_populate interface.
I reported this issue today somewhere else:
https://lore.kernel.org/linux-mm/YKVemB5DuSqLFmmz@t490s/
And came to this same line after the bisection.
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
-aneesh
From: Peter Xu <peterx@redhat.com> Date: 2021-05-20 12:46:43
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
--
Peter Xu
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
Are huge pud thp only allowed with DAX vmas?
-aneesh
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
something like this? (not even compile tested). I am still not sure
whether this is really needed or we handle DAX VMA's in some other form.
@@ -324,10 +324,51 @@ static inline bool move_normal_pud(struct vm_area_struct *vma,}#endif++staticboolmove_huge_pud(structvm_area_struct*vma,unsignedlongold_addr,+unsignedlongnew_addr,pud_t*old_pud,pud_t*new_pud)+{+spinlock_t*old_ptl,*new_ptl;+structmm_struct*mm=vma->vm_mm;+pud_tpud;++/*+*Thedestinationpudshouldn'tbeestablished,free_pgtables()+*shouldhavereleasedit.+*/+if(WARN_ON_ONCE(!pud_none(*new_pud)))+returnfalse;++/*+*Wedon'thavetoworryabouttheorderingofsrcanddst+*ptlocksbecauseexclusivemmap_lockpreventsdeadlock.+*/+old_ptl=pud_lock(vma->vm_mm,old_pud);+new_ptl=pud_lockptr(mm,new_pud);+if(new_ptl!=old_ptl)+spin_lock_nested(new_ptl,SINGLE_DEPTH_NESTING);++/* Clear the pud */+pud=*old_pud;+pud_clear(old_pud);++VM_BUG_ON(!pud_none(*new_pud));++/* Set the new pud */+set_pud_at(mm,new_addr,new_pud,pud);+flush_pud_tlb_range(vma,old_addr,old_addr+HPAGE_PUD_SIZE);+if(new_ptl!=old_ptl)+spin_unlock(new_ptl);+spin_unlock(old_ptl);++returntrue;+}+enumpgt_entry{NORMAL_PMD,HPAGE_PMD,NORMAL_PUD,+HPAGE_PUD,};/*
@@ -429,15 +477,23 @@ unsigned long move_page_tables(struct vm_area_struct *vma,*PUDlevelifpossible.*/extent=get_extent(NORMAL_PUD,old_addr,old_end,new_addr);-if(IS_ENABLED(CONFIG_HAVE_MOVE_PUD)&&extent==PUD_SIZE){-pud_t*old_pud,*new_pud;-old_pud=get_old_pud(vma->vm_mm,old_addr);-if(!old_pud)+old_pud=get_old_pud(vma->vm_mm,old_addr);+if(!old_pud)+continue;+new_pud=alloc_new_pud(vma->vm_mm,vma,new_addr);+if(!new_pud)+break;+if(pud_trans_huge(*old_pud)||pud_devmap(*old_pud)){+if(extent==HPAGE_PUD_SIZE){+move_pgt_entry(HPAGE_PUD,vma,old_addr,new_addr,+old_pud,new_pud,need_rmap_locks);+/* We ignore and continue on error? */continue;-new_pud=alloc_new_pud(vma->vm_mm,vma,new_addr);-if(!new_pud)-break;+}+}elseif(IS_ENABLED(CONFIG_HAVE_MOVE_PUD)&&extent==PUD_SIZE){+pud_t*old_pud,*new_pud;+if(move_pgt_entry(NORMAL_PUD,vma,old_addr,new_addr,old_pud,new_pud,need_rmap_locks))continue;
Are huge pud thp only allowed with DAX vmas?
-aneesh
From: Peter Xu <peterx@redhat.com> Date: 2021-05-20 14:58:06
On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
On 5/20/21 6:16 PM, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
something like this? (not even compile tested). I am still not sure
whether this is really needed or we handle DAX VMA's in some other form.
Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
default so far) it does seem to work even with huge pud, while after this patch
it seems to be not working anymore, even with your follow up fix.
Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
for dax or whatever backend?
Ideally maybe rework this patch (or series?) and repost it for a better review?
Agree the risk seems low. I'll leave that to you and Andrew to decide..
--
Peter Xu
From: Zi Yan <ziy@nvidia.com> Date: 2021-05-20 19:06:41
On 20 May 2021, at 10:57, Peter Xu wrote:
On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
quoted
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
On 5/20/21 6:16 PM, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
something like this? (not even compile tested). I am still not sure
whether this is really needed or we handle DAX VMA's in some other form.
Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
default so far) it does seem to work even with huge pud, while after this patch
it seems to be not working anymore, even with your follow up fix.
Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
for dax or whatever backend?
Ideally maybe rework this patch (or series?) and repost it for a better review?
Agree the risk seems low. I'll leave that to you and Andrew to decide..
From: Peter Xu <peterx@redhat.com> Date: 2021-05-20 20:01:16
On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote:
On 20 May 2021, at 10:57, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
quoted
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
On 5/20/21 6:16 PM, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
something like this? (not even compile tested). I am still not sure
whether this is really needed or we handle DAX VMA's in some other form.
Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
default so far) it does seem to work even with huge pud, while after this patch
it seems to be not working anymore, even with your follow up fix.
Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
for dax or whatever backend?
Ideally maybe rework this patch (or series?) and repost it for a better review?
Agree the risk seems low. I'll leave that to you and Andrew to decide..
It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP
was implemented[1].
Yes, but trickily as I mentioned it seems Android's CONFIG_HAVE_MOVE_PUD has
done this right (with no intention I guess) with the set_pud_at() before this
patch is merged, so we might have a short period that this might start to work..
I guess no one is using mremap on 1GB DAX THP. Maybe we want
to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud()
to handle the situation properly?
Agreed, if we decide to go with the patches, some warning (or even VM_BUG_ON,
which iiuc should be very not-suggested in most cases) looks better than
pgtable corruption reports.
--
Peter Xu
On Thu, May 20, 2021 at 4:01 PM Peter Xu [off-list ref] wrote:
On Thu, May 20, 2021 at 03:06:30PM -0400, Zi Yan wrote:
quoted
On 20 May 2021, at 10:57, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 07:07:57PM +0530, Aneesh Kumar K.V wrote:
quoted
"Aneesh Kumar K.V" [off-list ref] writes:
quoted
On 5/20/21 6:16 PM, Peter Xu wrote:
quoted
On Thu, May 20, 2021 at 01:56:54PM +0530, Aneesh Kumar K.V wrote:
quoted
quoted
This seems to work at least for my userfaultfd test on shmem, however I don't
fully understand the commit message [1] on: How do we guarantee we're not
moving a thp pte?
move_page_tables() checks for pmd_trans_huge() and ends up calling
move_huge_pmd if it is a THP entry.
Sorry to be unclear: what if a huge pud thp?
I am still checking. Looking at the code before commit
c49dd340180260c6239e453263a9a244da9a7c85, I don't see kernel handling
huge pud thp. I haven't studied huge pud thp enough to understand
whether c49dd340180260c6239e453263a9a244da9a7c85 intent to add that
support.
We can do a move_huge_pud() like we do for huge pmd thp. But I am not
sure whether we handle those VMA's earlier and restrict mremap on them?
something like this? (not even compile tested). I am still not sure
whether this is really needed or we handle DAX VMA's in some other form.
Yeah maybe (you may want to at least drop that extra "case HPAGE_PUD").
It's just that if with CONFIG_HAVE_MOVE_PUD (x86 and arm64 enables it by
default so far) it does seem to work even with huge pud, while after this patch
it seems to be not working anymore, even with your follow up fix.
Indeed I saw CONFIG_HAVE_MOVE_PUD is introduced a few months ago so breaking
someone seems to be unlikely, perhaps no real user yet to mremap() a huge pud
for dax or whatever backend?
Ideally maybe rework this patch (or series?) and repost it for a better review?
Agree the risk seems low. I'll leave that to you and Andrew to decide..
It seems that the mremap function for 1GB DAX THP was not added when 1GB DAX THP
was implemented[1].
Yes, but trickily as I mentioned it seems Android's CONFIG_HAVE_MOVE_PUD has
done this right (with no intention I guess) with the set_pud_at() before this
patch is merged, so we might have a short period that this might start to work..
It may have coincidentally handled the huge PUD case, but I hadn't
considered huge PUDs when implementing the HAVE_MOVE_PUD patchset. Or
as Zi suggested, huge PUD mremap may be unused atm, I haven't seen any
related breakages since enabling HAVE_MOVE_PUD for x86 and arm64
quoted
I guess no one is using mremap on 1GB DAX THP. Maybe we want
to at least add a warning or VM_BUG_ON to catch this or use Aneesh’s move_huge_pud()
to handle the situation properly?
Agreed, if we decide to go with the patches, some warning (or even VM_BUG_ON,
which iiuc should be very not-suggested in most cases) looks better than
pgtable corruption reports.
--
Peter Xu
Update _tlbiel_pid() such that we can avoid build errors like below when
using this function in other places.
arch/powerpc/mm/book3s64/radix_tlb.c: In function ‘__radix__flush_tlb_range_psize’:
arch/powerpc/mm/book3s64/radix_tlb.c:114:2: warning: ‘asm’ operand 3 probably does not match constraints
114 | asm volatile(PPC_TLBIEL(%0, %4, %3, %2, %1)
| ^~~
arch/powerpc/mm/book3s64/radix_tlb.c:114:2: error: impossible constraint in ‘asm’
make[4]: *** [scripts/Makefile.build:271: arch/powerpc/mm/book3s64/radix_tlb.o] Error 1
m
With this fix, we can also drop the __always_inline in __radix_flush_tlb_range_psize
which was added by commit e12d6d7d46a6 ("powerpc/mm/radix: mark __radix__flush_tlb_range_psize() as __always_inline")
Reviewed-by: Christophe Leroy <redacted>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/mm/book3s64/radix_tlb.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
@@ -291,22 +291,30 @@ static inline void fixup_tlbie_lpid(unsigned long lpid)/**Weuse128setinradixmodeand256setinhptmode.*/-static__always_inlinevoid_tlbiel_pid(unsignedlongpid,unsignedlongric)+staticinlinevoid_tlbiel_pid(unsignedlongpid,unsignedlongric){intset;asmvolatile("ptesync":::"memory");-/*-*FlushthefirstsetoftheTLB,andifwe'redoingaRIC_FLUSH_ALL,-*alsoflushtheentirePageWalkCache.-*/-__tlbiel_pid(pid,0,ric);+switch(ric){+caseRIC_FLUSH_PWC:-/* For PWC, only one flush is needed */-if(ric==RIC_FLUSH_PWC){+/* For PWC, only one flush is needed */+__tlbiel_pid(pid,0,RIC_FLUSH_PWC);ppc_after_tlbiel_barrier();return;+caseRIC_FLUSH_TLB:+__tlbiel_pid(pid,0,RIC_FLUSH_TLB);+break;+caseRIC_FLUSH_ALL:+default:+/*+*FlushthefirstsetoftheTLB,andif+*we'redoingaRIC_FLUSH_ALL,alsoflush+*theentirePageWalkCache.+*/+__tlbiel_pid(pid,0,RIC_FLUSH_ALL);}if(!cpu_has_feature(CPU_FTR_ARCH_31)){
unsigned long start, unsigned long end)
+{
+ return flush_pmd_tlb_pwc_range(vma, start, end, false);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Doesn't that cause build warnings/errors all over the place ?
It will, thanks. I queued a fix.
Also in mm/mremap.c, in case you didn't see it:
#ifndef flush_pte_tlb_pwc_range
#define flush_pte_tlb_pwc_range flush_pte_tlb_pwc_range
static inline void flush_pte_tlb_pwc_range(struct vm_area_struct *vma,
^^^^
unsigned long start,
unsigned long end)
{
return flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
}
+ unsigned long start,
+ unsigned long end,
+ bool flush_pwc)
+{
+ if (radix_enabled())
+ return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
+ return hash__flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
quoted
+}
In this specific case we won't have build errors because,
static inline void hash__flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
But I agree the below is better to read.
static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma,
unsigned long start,
unsigned long end,
bool flush_pwc)
{
if (radix_enabled())
radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
else
hash__flush_tlb_range(vma, start, end);
return
}
+ unsigned long start,
+ unsigned long end,
+ bool flush_pwc)
+{
+ if (radix_enabled())
+ return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
+ return hash__flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
quoted
+}
In this specific case we won't have build errors because,
static inline void hash__flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
Sorry, you completely lost me.
Building parisc:allnoconfig ... failed
--------------
Error log:
In file included from arch/parisc/include/asm/cacheflush.h:7,
from include/linux/highmem.h:12,
from include/linux/pagemap.h:11,
from include/linux/ksm.h:13,
from mm/mremap.c:14:
mm/mremap.c: In function 'flush_pte_tlb_pwc_range':
arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void
Guenter
But I agree the below is better to read.
static inline void flush_pmd_tlb_pwc_range(struct vm_area_struct *vma,
unsigned long start,
unsigned long end,
bool flush_pwc)
{
if (radix_enabled())
radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
else
hash__flush_tlb_range(vma, start, end);
return
}
+ unsigned long start,
+ unsigned long end,
+ bool flush_pwc)
+{
+ if (radix_enabled())
+ return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
+ return hash__flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
quoted
+}
In this specific case we won't have build errors because,
static inline void hash__flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
Sorry, you completely lost me.
Building parisc:allnoconfig ... failed
--------------
Error log:
In file included from arch/parisc/include/asm/cacheflush.h:7,
from include/linux/highmem.h:12,
from include/linux/pagemap.h:11,
from include/linux/ksm.h:13,
from mm/mremap.c:14:
mm/mremap.c: In function 'flush_pte_tlb_pwc_range':
arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void
+ unsigned long start,
+ unsigned long end,
+ bool flush_pwc)
+{
+ if (radix_enabled())
+ return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
+ return hash__flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
quoted
+}
In this specific case we won't have build errors because,
static inline void hash__flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
Sorry, you completely lost me.
Building parisc:allnoconfig ... failed
--------------
Error log:
In file included from arch/parisc/include/asm/cacheflush.h:7,
from include/linux/highmem.h:12,
from include/linux/pagemap.h:11,
from include/linux/ksm.h:13,
from mm/mremap.c:14:
mm/mremap.c: In function 'flush_pte_tlb_pwc_range':
arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
Guenter
+ unsigned long start,
+ unsigned long end,
+ bool flush_pwc)
+{
+ if (radix_enabled())
+ return radix__flush_pmd_tlb_range(vma, start, end, flush_pwc);
+ return hash__flush_tlb_range(vma, start, end);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
quoted
+}
In this specific case we won't have build errors because,
static inline void hash__flush_tlb_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
{
Sorry, you completely lost me.
Building parisc:allnoconfig ... failed
--------------
Error log:
In file included from arch/parisc/include/asm/cacheflush.h:7,
from include/linux/highmem.h:12,
from include/linux/pagemap.h:11,
from include/linux/ksm.h:13,
from mm/mremap.c:14:
mm/mremap.c: In function 'flush_pte_tlb_pwc_range':
arch/parisc/include/asm/tlbflush.h:20:2: error: 'return' with a value, in function returning void
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
cheers
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
Guenter Roeck [off-list ref] writes:
quoted
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
Segher
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
quoted
Guenter Roeck [off-list ref] writes:
quoted
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
Segher
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
quoted
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
quoted
Guenter Roeck [off-list ref] writes:
quoted
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
In C99:
"6.8.6.4 The return statement
Constraints
A return statement with an expression shall not appear in a function whose return type
is void. A return statement without an expression shall only appear in a function
whose return type is void."
Sounds like invalid to me, not just undefined behavior.
Guenter
On Wed, May 19, 2021 at 06:37:44AM -0700, Guenter Roeck wrote:
On 5/19/21 5:03 AM, Segher Boessenkool wrote:
quoted
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
quoted
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
In C99:
"6.8.6.4 The return statement
Constraints
A return statement with an expression shall not appear in a function whose
return type
is void. A return statement without an expression shall only appear in a
function
whose return type is void."
Sounds like invalid to me, not just undefined behavior.
I don't know what "invalid" would mean here other than UB, it isn't a
specific defined term, unlike the latter, which is precisely defined in
3.4.3/1:
undefined behavior
behavior, upon use of a nonportable or erroneous program construct or
of erroneous data, for which this International Standard imposes no
requirements
This is the strongest thing the standard can say, it is not Law, it does
not prohibit anyone from doing anything :-)
"Shall" and "shall not" X means it is undefined behaviour if X (or its
inverse) is violated. See 4.2:
If a ''shall'' or ''shall not'' requirement that appears outside of a
constraint or runtime-constraint is violated, the behavior is
undefined. Undefined behavior is otherwise indicated in this
International Standard by the words ''undefined behavior'' or by the
omission of any explicit definition of behavior. There is no
difference in emphasis among these three; they all describe ''behavior
that is undefined''.
which also explains that what you call "invalid" has undefined behaviour
just as well, most likely.
Segher
On Wed, May 19, 2021 at 09:20:38AM -0500, Segher Boessenkool wrote:
On Wed, May 19, 2021 at 06:37:44AM -0700, Guenter Roeck wrote:
quoted
On 5/19/21 5:03 AM, Segher Boessenkool wrote:
quoted
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
quoted
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
In C99:
"6.8.6.4 The return statement
Constraints
A return statement with an expression shall not appear in a function whose
return type
is void. A return statement without an expression shall only appear in a
function
whose return type is void."
Sounds like invalid to me, not just undefined behavior.
I don't know what "invalid" would mean here other than UB, it isn't a
specific defined term, unlike the latter, which is precisely defined in
3.4.3/1:
undefined behavior
behavior, upon use of a nonportable or erroneous program construct or
of erroneous data, for which this International Standard imposes no
requirements
This is the strongest thing the standard can say, it is not Law, it does
not prohibit anyone from doing anything :-)
"Shall" and "shall not" X means it is undefined behaviour if X (or its
inverse) is violated. See 4.2:
If a ''shall'' or ''shall not'' requirement that appears outside of a
constraint or runtime-constraint is violated, the behavior is
undefined. Undefined behavior is otherwise indicated in this
International Standard by the words ''undefined behavior'' or by the
omission of any explicit definition of behavior. There is no
difference in emphasis among these three; they all describe ''behavior
that is undefined''.
which also explains that what you call "invalid" has undefined behaviour
just as well, most likely.
I'd have assumed that "shall not" is syntactically wrong, but I stand
corrected.
Guenter
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-05-20 07:37:33
Segher Boessenkool [off-list ref] writes:
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
quoted
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
quoted
Guenter Roeck [off-list ref] writes:
quoted
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
We use gnu89, which presumably does not make it UB.
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
There's no chance we'll ever enable -Wpedantic, so I guess it's allowed
for practical purposes. I guess clang must accept it too or we'd be
seeing warnings from it.
cheers
Hi!
On Thu, May 20, 2021 at 05:37:20PM +1000, Michael Ellerman wrote:
Segher Boessenkool [off-list ref] writes:
quoted
On Tue, May 18, 2021 at 07:45:14PM -0500, Segher Boessenkool wrote:
quoted
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
quoted
Guenter Roeck [off-list ref] writes:
quoted
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
We use gnu89, which presumably does not make it UB.
Indeed. That is kind of implied by the "as a GNU extension" below, but
some explicit statement would be better, yup.
quoted
... but there is a GCC extension that allows this by default:
<https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wreturn-type>
For C only, warn about a 'return' statement with an expression in a
function whose return type is 'void', unless the expression type is
also 'void'. As a GNU extension, the latter case is accepted
without a warning unless '-Wpedantic' is used.
There's no chance we'll ever enable -Wpedantic,
Good, because -pedantic adds a lot of much more annoying warnings as
well. I find this extension questionable (like Guenter says it is
confusing and has no purpose), so the only thing it is "good" for is it
causes long email threads ;-)
Other than those things it is harmless though.
so I guess it's allowed
for practical purposes. I guess clang must accept it too or we'd be
seeing warnings from it.
On 5/18/21 5:26 PM, Michael Ellerman wrote:
[ ... ]
quoted
quoted
That was the generic header change in the patch. I was commenting about the
ppc64 specific change causing build failures.
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
Agreed, but it is surprisingly wide-spread. Try to run the coccinelle
script below, just for fun. The script doesn't even catch instances
in include files, yet there are more than 450 hits.
Guenter
---
virtual report
@d@
identifier f;
expression e;
position p;
@@
void f(...)
{
<...
return e@p;
...>
}
@script:python depends on report@
f << d.f;
p << d.p;
@@
print "void function %s:%s() with non-void return in line %s" % (p[0].file, f, p[0].line)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-05-20 11:38:11
Guenter Roeck [off-list ref] writes:
On 5/18/21 5:26 PM, Michael Ellerman wrote:
[ ... ]
quoted
quoted
quoted
That was the generic header change in the patch. I was commenting about the
ppc64 specific change causing build failures.
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
Agreed, but it is surprisingly wide-spread. Try to run the coccinelle
script below, just for fun. The script doesn't even catch instances
in include files, yet there are more than 450 hits.
Yikes, that is a lot.
I guess they're pretty harmless, but would be nice to clean them up
eventually.
Why doesn't the script work for instances in headers?
cheers
On 5/18/21 5:26 PM, Michael Ellerman wrote:
[ ... ]
quoted
quoted
quoted
That was the generic header change in the patch. I was commenting about the
ppc64 specific change causing build failures.
Ah, sorry. I wasn't aware that the following is valid C code
void f1()
{
return f2();
^^^^^^
}
as long as f2() is void as well. Confusing, but we live and learn.
It might be valid, but it's still bad IMHO.
It's confusing to readers, and serves no useful purpose.
Agreed, but it is surprisingly wide-spread. Try to run the coccinelle
script below, just for fun. The script doesn't even catch instances
in include files, yet there are more than 450 hits.
Yikes, that is a lot.
I guess they're pretty harmless, but would be nice to clean them up
eventually.
Why doesn't the script work for instances in headers?
Ah, that is one of those Coccinelle details. No idea. I run the script with
make coccicheck COCCI="return-void.cocci" MODE=report M=.
Probably there is a better way to invoke it which does handle include files.
Guenter
Some architectures do have the concept of page walk cache which need
to be flush when updating higher levels of page tables. A fast mremap
that involves moving page table pages instead of copying pte entries
should flush page walk cache since the old translation cache is no more
valid.
Add new helper flush_pte_tlb_pwc_range() which invalidates both TLB and
page walk cache where TLB entries are mapped with page size PAGE_SIZE.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
arch/powerpc/include/asm/book3s/64/tlbflush.h | 10 ++++++++++
mm/mremap.c | 14 ++++++++++++--
2 files changed, 22 insertions(+), 2 deletions(-)
Move TLB flush outside page table lock so that kernel does
less with page table lock held. Releasing the ptl with old
TLB contents still valid will behave such that such access
happened before the level3 or level2 entry update.
Signed-off-by: Aneesh Kumar K.V <redacted>
---
mm/mremap.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Move TLB flush outside page table lock so that kernel does
less with page table lock held. Releasing the ptl with old
TLB contents still valid will behave such that such access
happened before the level3 or level2 entry update.
Ok this break the page lifetime rule
commit: eb66ae030829 ("mremap: properly flush TLB before releasing the
page")
I will respin dropping this change and add a comment around explaining
why we need to do tlb flush before dropping ptl.
Move TLB flush outside page table lock so that kernel does
less with page table lock held. Releasing the ptl with old
TLB contents still valid will behave such that such access
happened before the level3 or level2 entry update.
Ok this break the page lifetime rule
commit: eb66ae030829 ("mremap: properly flush TLB before releasing the
page")
I will respin dropping this change and add a comment around explaining
why we need to do tlb flush before dropping ptl.
Wondering whether this is correct considering we are holding mmap_sem in
write mode in mremap. Can a parallel free/zap happen?
*vma, unsigned long old_addr,
* We don't have to worry about the ordering of src and dst
* ptlocks because exclusive mmap_lock prevents deadlock.
*/
- old_ptl = pmd_lock(vma->vm_mm, old_pmd);
+ old_ptl = pmd_lock(mm, old_pmd);
new_ptl = pmd_lockptr(mm, new_pmd);
if (new_ptl != old_ptl)
spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
*vma, unsigned long old_addr,
* We don't have to worry about the ordering of src and dst
* ptlocks because exclusive mmap_lock prevents deadlock.
*/
- old_ptl = pud_lock(vma->vm_mm, old_pud);
+ old_ptl = pud_lock(mm, old_pud);
new_ptl = pud_lockptr(mm, new_pud);
if (new_ptl != old_ptl)
spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
On Thu, May 20, 2021 at 6:57 AM Aneesh Kumar K.V
[off-list ref] wrote:
Wondering whether this is correct considering we are holding mmap_sem in
write mode in mremap.
Right. So *normally* the rule is to EITHER
- hold the mmap_sem for writing
OR
- hold the page table lock
and that the TLB flush needs to happen before you release that lock.
But as that commit message of commit eb66ae030829 ("mremap: properly
flush TLB before releasing the page") says, "mremap()" is a bit
special. It's special because mremap() didn't take ownership of the
page - it only moved it somewhere else. So now the page-out logic -
that relies on the page table lock - can free the page immediately
after we've released the page table lock.
So basically, in order to delay the TLB flush after releasing the page
table lock, it's not really sufficient to _just_ hold the mmap_sem for
writing. You also need to guarantee that the lifetime of the page
itself is held until after the TLB flush.
For normal operations like "munmap()", this happens naturally, because
we remove the page from the page table, and add it to the list of
pages to be freed after the TLB flush.
But mremap never did that "remove the page and add it to a list to be
free'd later". Instead, it just moved the page somewhere else. And
thus there is no guarantee that the page that got moved will continue
to exist until a TLB flush is done.
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
Linus
On Thu, May 20, 2021 at 6:57 AM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
Wondering whether this is correct considering we are holding mmap_sem in
write mode in mremap.
Right. So *normally* the rule is to EITHER
- hold the mmap_sem for writing
OR
- hold the page table lock
and that the TLB flush needs to happen before you release that lock.
But as that commit message of commit eb66ae030829 ("mremap: properly
flush TLB before releasing the page") says, "mremap()" is a bit
special. It's special because mremap() didn't take ownership of the
page - it only moved it somewhere else. So now the page-out logic -
that relies on the page table lock - can free the page immediately
after we've released the page table lock.
So basically, in order to delay the TLB flush after releasing the page
table lock, it's not really sufficient to _just_ hold the mmap_sem for
writing. You also need to guarantee that the lifetime of the page
itself is held until after the TLB flush.
For normal operations like "munmap()", this happens naturally, because
we remove the page from the page table, and add it to the list of
pages to be freed after the TLB flush.
But mremap never did that "remove the page and add it to a list to be
free'd later". Instead, it just moved the page somewhere else. And
thus there is no guarantee that the page that got moved will continue
to exist until a TLB flush is done.
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
-aneesh
On Thu, May 20, 2021 at 6:57 AM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
Wondering whether this is correct considering we are holding mmap_sem in
write mode in mremap.
Right. So *normally* the rule is to EITHER
- hold the mmap_sem for writing
OR
- hold the page table lock
and that the TLB flush needs to happen before you release that lock.
But as that commit message of commit eb66ae030829 ("mremap: properly
flush TLB before releasing the page") says, "mremap()" is a bit
special. It's special because mremap() didn't take ownership of the
page - it only moved it somewhere else. So now the page-out logic -
that relies on the page table lock - can free the page immediately
after we've released the page table lock.
So basically, in order to delay the TLB flush after releasing the page
table lock, it's not really sufficient to _just_ hold the mmap_sem for
writing. You also need to guarantee that the lifetime of the page
itself is held until after the TLB flush.
For normal operations like "munmap()", this happens naturally, because
we remove the page from the page table, and add it to the list of
pages to be freed after the TLB flush.
But mremap never did that "remove the page and add it to a list to be
free'd later". Instead, it just moved the page somewhere else. And
thus there is no guarantee that the page that got moved will continue
to exist until a TLB flush is done.
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
This should help? ie, we flush tlb before we move pagetables to the new
address?
modified mm/mremap.c
@@ -277,11 +277,14 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr, /* Clear the pmd */ pmd = *old_pmd; pmd_clear(old_pmd);-+ /*+ * flush the TLB before we move the page table entries.+ * TLB flush includes necessary barriers.+ */+ flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE); VM_BUG_ON(!pmd_none(*new_pmd)); pmd_populate(mm, new_pmd, pmd_pgtable(pmd));- flush_pte_tlb_pwc_range(vma, old_addr, old_addr + PMD_SIZE); if (new_ptl != old_ptl) spin_unlock(new_ptl); spin_unlock(old_ptl);-aneesh
On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
[off-list ref] wrote:
On 5/21/21 8:10 AM, Linus Torvalds wrote:
quoted
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
Hmm. Interesting.
Your patch (to flush the TLB after clearing the old location, and
before inserting it into the new one) looks like an "obvious" fix.
But I'm putting that "obvious" in quotes, because I'm now wondering if
it actually fixes anything.
Lookie here:
- CPU1 does a mremap of a pmd or pud.
It clears the old pmd/pud, flushes the old TLB range, and then
inserts the pmd/pud at the new location.
- CPU2 does a page shrinker, which calls try_to_unmap, which calls
try_to_unmap_one.
These are entirely asynchronous, because they have no shared lock. The
mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
which does the pte lock.
Now, imagine that the following ordering happens with the two
operations above, and a CPU3 that does accesses:
- CPU2 follows (and sees) the old page tables in the old location and
the took the pte lock
- the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
*and* inserts in the new place.
- a user thread on CPU3 accesses the new location and fills the TLB
of the *new* address
- only now does CPU2 get to the "pte_get_and_clear()" to remove one page
- CPU2 does a TLB flush and frees the page
End result:
- both CPU1 _and_ CPU2 have flushed the TLB.
- but both flushed the *OLD* address
- the page is freed
- CPU3 still has the stale TLB entry pointing to the page that is now
free and might be reused for something else
Am I missing something?
Linus
On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
On 5/21/21 8:10 AM, Linus Torvalds wrote:
quoted
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
Hmm. Interesting.
Your patch (to flush the TLB after clearing the old location, and
before inserting it into the new one) looks like an "obvious" fix.
But I'm putting that "obvious" in quotes, because I'm now wondering if
it actually fixes anything.
Lookie here:
- CPU1 does a mremap of a pmd or pud.
It clears the old pmd/pud, flushes the old TLB range, and then
inserts the pmd/pud at the new location.
- CPU2 does a page shrinker, which calls try_to_unmap, which calls
try_to_unmap_one.
These are entirely asynchronous, because they have no shared lock. The
mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
which does the pte lock.
Now, imagine that the following ordering happens with the two
operations above, and a CPU3 that does accesses:
- CPU2 follows (and sees) the old page tables in the old location and
the took the pte lock
- the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
*and* inserts in the new place.
- a user thread on CPU3 accesses the new location and fills the TLB
of the *new* address
- only now does CPU2 get to the "pte_get_and_clear()" to remove one page
- CPU2 does a TLB flush and frees the page
End result:
- both CPU1 _and_ CPU2 have flushed the TLB.
- but both flushed the *OLD* address
- the page is freed
- CPU3 still has the stale TLB entry pointing to the page that is now
free and might be reused for something else
Am I missing something?
That is a problem. With that it looks like CONFIG_HAVE_MOVE_PMD/PUD is
broken? I don't see an easy way to fix this?
-aneesh
On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
On 5/21/21 8:10 AM, Linus Torvalds wrote:
quoted
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
Hmm. Interesting.
Your patch (to flush the TLB after clearing the old location, and
before inserting it into the new one) looks like an "obvious" fix.
But I'm putting that "obvious" in quotes, because I'm now wondering if
it actually fixes anything.
Lookie here:
- CPU1 does a mremap of a pmd or pud.
It clears the old pmd/pud, flushes the old TLB range, and then
inserts the pmd/pud at the new location.
- CPU2 does a page shrinker, which calls try_to_unmap, which calls
try_to_unmap_one.
These are entirely asynchronous, because they have no shared lock. The
mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
which does the pte lock.
Now, imagine that the following ordering happens with the two
operations above, and a CPU3 that does accesses:
- CPU2 follows (and sees) the old page tables in the old location and
the took the pte lock
- the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
*and* inserts in the new place.
- a user thread on CPU3 accesses the new location and fills the TLB
of the *new* address
- only now does CPU2 get to the "pte_get_and_clear()" to remove one page
- CPU2 does a TLB flush and frees the page
End result:
- both CPU1 _and_ CPU2 have flushed the TLB.
- but both flushed the *OLD* address
- the page is freed
- CPU3 still has the stale TLB entry pointing to the page that is now
free and might be reused for something else
Am I missing something?
That is a problem. With that it looks like CONFIG_HAVE_MOVE_PMD/PUD is
broken? I don't see an easy way to fix this?
We could do MOVE_PMD with something like below? A equivalent MOVE_PUD
will be costlier which makes me wonder whether we should even support that?
On Fri, May 21, 2021 at 3:04 AM Aneesh Kumar K.V
[off-list ref] wrote:
We could do MOVE_PMD with something like below? A equivalent MOVE_PUD
will be costlier which makes me wonder whether we should even support that?
Well, without USE_SPLIT_PTE_PTLOCKS the pud case would be trivial too.
But everybody uses split pte locks in practice.
I get the feeling that the rmap code might have to use
pud_lock/pmd_lock. I wonder how painful that would be.
Linus
On Fri, May 21, 2021 at 3:04 AM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
We could do MOVE_PMD with something like below? A equivalent MOVE_PUD
will be costlier which makes me wonder whether we should even support that?
Well, without USE_SPLIT_PTE_PTLOCKS the pud case would be trivial too.
But everybody uses split pte locks in practice.
I get the feeling that the rmap code might have to use
pud_lock/pmd_lock. I wonder how painful that would be.
Looking at this further, i guess we need to do the above to close the
race window. We do
static bool map_pte(struct page_vma_mapped_walk *pvmw)
{
pvmw->pte = pte_offset_map(pvmw->pmd, pvmw->address);
..
pvmw->ptl = pte_lockptr(pvmw->vma->vm_mm, pvmw->pmd);
spin_lock(pvmw->ptl);
}
That is we walk the table without holding the pte ptl. Hence we still
can race with the optimized PMD move.
-aneesh
On Thu, May 20, 2021 at 5:03 PM Aneesh Kumar K.V
[off-list ref] wrote:
quoted
On 5/21/21 8:10 AM, Linus Torvalds wrote:
quoted
So mremap does need to flush the TLB before releasing the page table
lock, because that's the lifetime boundary for the page that got
moved.
How will we avoid that happening with
c49dd340180260c6239e453263a9a244da9a7c85 /
2c91bd4a4e2e530582d6fd643ea7b86b27907151 . The commit improves mremap
performance by moving level3/level2 page table entries. When doing so we
are not holding level 4 ptl lock (pte_lock()). But rather we are holding
pmd_lock or pud_lock(). So if we move pages around without holding the
pte lock, won't the above issue happen even if we do a tlb flush with
holding pmd lock/pud lock?
Hmm. Interesting.
Your patch (to flush the TLB after clearing the old location, and
before inserting it into the new one) looks like an "obvious" fix.
But I'm putting that "obvious" in quotes, because I'm now wondering if
it actually fixes anything.
Lookie here:
- CPU1 does a mremap of a pmd or pud.
It clears the old pmd/pud, flushes the old TLB range, and then
inserts the pmd/pud at the new location.
- CPU2 does a page shrinker, which calls try_to_unmap, which calls
try_to_unmap_one.
These are entirely asynchronous, because they have no shared lock. The
mremap uses the pmd lock, the try_to_unmap_one() does the rmap walk,
which does the pte lock.
Now, imagine that the following ordering happens with the two
operations above, and a CPU3 that does accesses:
- CPU2 follows (and sees) the old page tables in the old location and
the took the pte lock
- the mremap on CPU1 starts - cleared the old pmd, flushed the tlb,
*and* inserts in the new place.
- a user thread on CPU3 accesses the new location and fills the TLB
of the *new* address
mremap holds the mmap_sem in write mode as well, doesn't it? How is the user thread
getting the new location?
quoted
- only now does CPU2 get to the "pte_get_and_clear()" to remove one page
- CPU2 does a TLB flush and frees the page
End result:
- both CPU1 _and_ CPU2 have flushed the TLB.
- but both flushed the *OLD* address
- the page is freed
- CPU3 still has the stale TLB entry pointing to the page that is now
free and might be reused for something else
Am I missing something?
That is a problem. With that it looks like CONFIG_HAVE_MOVE_PMD/PUD is
broken? I don't see an easy way to fix this?
-aneesh