[PATCH V2] powerpc: thp: Fix crash on mremap

[PATCH V2] powerpc: thp: Fix crash on mremap

[Aneesh Kumar K.V]

From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

This patch fix the below crash

NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
LR [c0000000000439ac] .hash_page+0x18c/0x5e0
...
Call Trace:
[c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
[437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
[437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58

On ppc64 we use the pgtable for storing the hpte slot information and
store address to the pgtable at a constant offset (PTRS_PER_PMD) from
pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
from new pmd.

We also want to move the withdraw and deposit before the set_pmd so
that, when page fault find the pmd as trans huge we can be sure that
pgtable can be located at the offset.

variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
for 3.12 stable series

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/Kconfig                           |  3 +++
 arch/powerpc/platforms/Kconfig.cputype |  1 +
 mm/huge_memory.c                       | 12 ++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index af2cc6eabcc7..bca9e7a18bd2 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -365,6 +365,9 @@ config HAVE_ARCH_TRANSPARENT_HUGEPAGE
 config HAVE_ARCH_SOFT_DIRTY
 	bool
 
+config ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW
+	bool
+
 config HAVE_MOD_ARCH_SPECIFIC
 	bool
 	help
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 6704e2e20e6b..0225011231ea 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -71,6 +71,7 @@ config PPC_BOOK3S_64
 	select PPC_FPU
 	select PPC_HAVE_PMU_SUPPORT
 	select SYS_SUPPORTS_HUGETLBFS
+	select ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if PPC_64K_PAGES
 
 config PPC_BOOK3E_64
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 292a266e0d42..89b7a647f1cb 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1474,8 +1474,20 @@ int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
 
 	ret = __pmd_trans_huge_lock(old_pmd, vma);
 	if (ret == 1) {
+#ifdef CONFIG_ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW
+		pgtable_t pgtable;
+#endif
 		pmd = pmdp_get_and_clear(mm, old_addr, old_pmd);
 		VM_BUG_ON(!pmd_none(*new_pmd));
+#ifdef CONFIG_ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW
+		/*
+		 * Archs like ppc64 use pgtable to store per pmd
+		 * specific information. So when we switch the pmd,
+		 * we should also withdraw and deposit the pgtable
+		 */
+		pgtable = pgtable_trans_huge_withdraw(mm, old_pmd);
+		pgtable_trans_huge_deposit(mm, new_pmd, pgtable);
+#endif
 		set_pmd_at(mm, new_addr, new_pmd, pmd_mksoft_dirty(pmd));
 		spin_unlock(&mm->page_table_lock);
 	}
-- 
1.8.3.2

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Greg KH]

On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> This patch fix the below crash
> 
> NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
> LR [c0000000000439ac] .hash_page+0x18c/0x5e0
> ...
> Call Trace:
> [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
> [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
> [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
> 
> On ppc64 we use the pgtable for storing the hpte slot information and
> store address to the pgtable at a constant offset (PTRS_PER_PMD) from
> pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
> the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
> from new pmd.
> 
> We also want to move the withdraw and deposit before the set_pmd so
> that, when page fault find the pmd as trans huge we can be sure that
> pgtable can be located at the offset.
> 
> variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
> for 3.12 stable series

This doesn't look like a "variant", it looks totally different.  Why
can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
(and follow-on fix) for 3.12?

I _REALLY_ dislike patches that are totally different from Linus's tree
in stable trees, it has caused nothing but problems in the past.

greg k-h

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Benjamin Herrenschmidt]

On Tue, 2014-02-11 at 09:31 -0800, Greg KH wrote:
> On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
> > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> > 
> > This patch fix the below crash
> > 
> > NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
> > LR [c0000000000439ac] .hash_page+0x18c/0x5e0
> > ...
> > Call Trace:
> > [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
> > [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
> > [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
> > 
> > On ppc64 we use the pgtable for storing the hpte slot information and
> > store address to the pgtable at a constant offset (PTRS_PER_PMD) from
> > pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
> > the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
> > from new pmd.
> > 
> > We also want to move the withdraw and deposit before the set_pmd so
> > that, when page fault find the pmd as trans huge we can be sure that
> > pgtable can be located at the offset.
> > 
> > variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
> > for 3.12 stable series
> 
> This doesn't look like a "variant", it looks totally different.  Why
> can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
> (and follow-on fix) for 3.12?
> 
> I _REALLY_ dislike patches that are totally different from Linus's tree
> in stable trees, it has caused nothing but problems in the past.

I don't think it applies... (I tried on an internal tree) but the
affected function changed in 3.13 in various ways. Aneesh, please
provide a more details explanation and whether we should backport those
other changes too or whether this is not necessary.

BTW. Aneesh, we need a 3.11.x one too

Cheers,
Ben.

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Aneesh Kumar K.V]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Tue, 2014-02-11 at 09:31 -0800, Greg KH wrote:
>> On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
>> > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> > 
>> > This patch fix the below crash
>> > 
>> > NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
>> > LR [c0000000000439ac] .hash_page+0x18c/0x5e0
>> > ...
>> > Call Trace:
>> > [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
>> > [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
>> > [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
>> > 
>> > On ppc64 we use the pgtable for storing the hpte slot information and
>> > store address to the pgtable at a constant offset (PTRS_PER_PMD) from
>> > pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
>> > the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
>> > from new pmd.
>> > 
>> > We also want to move the withdraw and deposit before the set_pmd so
>> > that, when page fault find the pmd as trans huge we can be sure that
>> > pgtable can be located at the offset.
>> > 
>> > variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
>> > for 3.12 stable series
>> 
>> This doesn't look like a "variant", it looks totally different.  Why
>> can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
>> (and follow-on fix) for 3.12?
>> 
>> I _REALLY_ dislike patches that are totally different from Linus's tree
>> in stable trees, it has caused nothing but problems in the past.
>
> I don't think it applies... (I tried on an internal tree) but the
> affected function changed in 3.13 in various ways. Aneesh, please
> provide a more details explanation and whether we should backport those
> other changes too or whether this is not necessary

Yes the affected function added support for split ptl locks for huge
pte. I don't think that is a stable material.

.
>
> BTW. Aneesh, we need a 3.11.x one too
>

3.11.x it is already applied.

-aneesh

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Aneesh Kumar K.V]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> This patch fix the below crash
>> 
>> NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
>> LR [c0000000000439ac] .hash_page+0x18c/0x5e0
>> ...
>> Call Trace:
>> [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
>> [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
>> [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
>> 
>> On ppc64 we use the pgtable for storing the hpte slot information and
>> store address to the pgtable at a constant offset (PTRS_PER_PMD) from
>> pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
>> the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
>> from new pmd.
>> 
>> We also want to move the withdraw and deposit before the set_pmd so
>> that, when page fault find the pmd as trans huge we can be sure that
>> pgtable can be located at the offset.
>> 
>> variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
>> for 3.12 stable series
>
> This doesn't look like a "variant", it looks totally different.  Why
> can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
> (and follow-on fix) for 3.12?

Because the code in that function changed in 3.13. Kirill added split
ptl locks for huge pte, and we decide whether to withdraw and
deposit again based on the ptl locks in 3.13. In 3.12 we do that only
for ppc64 using #ifdef


>
> I _REALLY_ dislike patches that are totally different from Linus's tree
> in stable trees, it has caused nothing but problems in the past.
>

-aneesh

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Greg KH]

On Wed, Feb 12, 2014 at 08:22:02AM +0530, Aneesh Kumar K.V wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> > On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >> 
> >> This patch fix the below crash
> >> 
> >> NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
> >> LR [c0000000000439ac] .hash_page+0x18c/0x5e0
> >> ...
> >> Call Trace:
> >> [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
> >> [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
> >> [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
> >> 
> >> On ppc64 we use the pgtable for storing the hpte slot information and
> >> store address to the pgtable at a constant offset (PTRS_PER_PMD) from
> >> pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
> >> the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
> >> from new pmd.
> >> 
> >> We also want to move the withdraw and deposit before the set_pmd so
> >> that, when page fault find the pmd as trans huge we can be sure that
> >> pgtable can be located at the offset.
> >> 
> >> variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
> >> for 3.12 stable series
> >
> > This doesn't look like a "variant", it looks totally different.  Why
> > can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
> > (and follow-on fix) for 3.12?
> 
> Because the code in that function changed in 3.13. Kirill added split
> ptl locks for huge pte, and we decide whether to withdraw and
> deposit again based on the ptl locks in 3.13. In 3.12 we do that only
> for ppc64 using #ifdef

I have no idea what that means...

If you want this patch applied, please be specific as to what is going
on, why the code is _very_ different, and all of that.  Make it
_obvious_ as to what is happening, and why I would be a fool not to take
it in the stable tree.

As it is, the code in this patch looks so different that I'm just
assuming you got something wrong and are trying to really send me
something else, so I'll just ignore it.

greg k-h

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Aneesh Kumar K.V]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Wed, Feb 12, 2014 at 08:22:02AM +0530, Aneesh Kumar K.V wrote:
>> Greg KH <gregkh@linuxfoundation.org> writes:
>> 
>> > On Fri, Feb 07, 2014 at 07:21:57PM +0530, Aneesh Kumar K.V wrote:
>> >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> >> 
>> >> This patch fix the below crash
>> >> 
>> >> NIP [c00000000004cee4] .__hash_page_thp+0x2a4/0x440
>> >> LR [c0000000000439ac] .hash_page+0x18c/0x5e0
>> >> ...
>> >> Call Trace:
>> >> [c000000736103c40] [00001ffffb000000] 0x1ffffb000000(unreliable)
>> >> [437908.479693] [c000000736103d50] [c0000000000439ac] .hash_page+0x18c/0x5e0
>> >> [437908.479699] [c000000736103e30] [c00000000000924c] .do_hash_page+0x4c/0x58
>> >> 
>> >> On ppc64 we use the pgtable for storing the hpte slot information and
>> >> store address to the pgtable at a constant offset (PTRS_PER_PMD) from
>> >> pmd. On mremap, when we switch the pmd, we need to withdraw and deposit
>> >> the pgtable again, so that we find the pgtable at PTRS_PER_PMD offset
>> >> from new pmd.
>> >> 
>> >> We also want to move the withdraw and deposit before the set_pmd so
>> >> that, when page fault find the pmd as trans huge we can be sure that
>> >> pgtable can be located at the offset.
>> >> 
>> >> variant of upstream SHA1: b3084f4db3aeb991c507ca774337c7e7893ed04f
>> >> for 3.12 stable series
>> >
>> > This doesn't look like a "variant", it looks totally different.  Why
>> > can't I just take the b3084f4db3aeb991c507ca774337c7e7893ed04f patch
>> > (and follow-on fix) for 3.12?
>> 
>> Because the code in that function changed in 3.13. Kirill added split
>> ptl locks for huge pte, and we decide whether to withdraw and
>> deposit again based on the ptl locks in 3.13. In 3.12 we do that only
>> for ppc64 using #ifdef
>
> I have no idea what that means...
>
> If you want this patch applied, please be specific as to what is going
> on, why the code is _very_ different, and all of that.  Make it
> _obvious_ as to what is happening, and why I would be a fool not to take
> it in the stable tree.
>
> As it is, the code in this patch looks so different that I'm just
> assuming you got something wrong and are trying to really send me
> something else, so I'll just ignore it.

3.13 we added split huge ptl lock which introduced separate lock at pmd
level for hugepage (bf929152e9f6c49b66fad4ebf08cc95b02ce48f5). This
required us 3592806cfa08b7cca968f793c33f8e9460bab395. ie, when we move
huge page, we need to withdraw and deposit PTE page if we are moving
them across different pmd page. We did that by checking spin lock
address in 3.13. ie, we have


 if (new_ptl != old_ptl) {
.....
       pgtable = pgtable_trans_huge_withdraw(mm, old_pmd);
       pgtable_trans_huge_deposit(mm, new_pmd,pgtable);
...
}

ppc64 even without using split ptl had PTE page per pmd entry. The
details for that are explained in the commit message above. So when
we move huge page we need to withdraw and deposit PTE page always on
ppc64.

Now on 3.13 we added a new function which did


static inline int pmd_move_must_withdraw(spinlock_t *new_pmd_ptl,
                                        spinlock_t *old_pmd_ptl)
{
       /*
        * With split pmd lock we also need to move preallocated
        * PTE page table if new_pmd is on different PMD page table.
        */
       return new_pmd_ptl != old_pmd_ptl;
}

for x86

and on ppc64 we did

static inline int pmd_move_must_withdraw(spinlock_t *new_pmd_ptl,
                                        spinlock_t *old_pmd_ptl)
{
       /*
        * Archs like ppc64 use pgtable to store per pmd
        * specific information. So when we switch the pmd,
        * we should also withdraw and deposit the pgtable
        */
       return true;
}

ie, on ppc64 we always did withdraw and deposit and on x86 we do that
only when spin lock address are different.

For 3.12, since we don't have split huge ptl locks yet, we did the below

+#ifdef CONFIG_ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW
+		/*
+		 * Archs like ppc64 use pgtable to store per pmd
+		 * specific information. So when we switch the pmd,
+		 * we should also withdraw and deposit the pgtable
+		 */
+		pgtable = pgtable_trans_huge_withdraw(mm, old_pmd);
+		pgtable_trans_huge_deposit(mm, new_pmd, pgtable);
+#endif

CONFIG_ARCH_THP_MOVE_PMD_ALWAYS_WITHDRAW is only set for PPC64.

-aneesh

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Benjamin Herrenschmidt]

On Wed, 2014-02-12 at 06:23 -0800, Greg KH wrote:
> I have no idea what that means...
> 
> If you want this patch applied, please be specific as to what is going
> on, why the code is _very_ different, and all of that.  Make it
> _obvious_ as to what is happening, and why I would be a fool not to
> take
> it in the stable tree.
> 
> As it is, the code in this patch looks so different that I'm just
> assuming you got something wrong and are trying to really send me
> something else, so I'll just ignore it.

It looks very different because the function that needs to be fixed
changed a lot upstream in 3.13.

In practice it's *not* very different in behaviour. It's just that
on powerpc we need to unconditionally call withdraw and deposit when
moving PTEs or it will crash, due to how we keep the transparent
huge page in sync with the hash table.

With the 3.13 code, due to lock breaking introduced by Kirill in
3.13-rc's, there's already a generic case for doing that (if we dropped
the lock). So we just changed the condition to essentially force the
condition to true to always do it under control of an arch helper.

The pre-3.13 code didn't do the withdraw and deposit at all in that
function however, so in that case, the patch (this 3.12 one) basically
just adds the calls to withdraw and deposit under control of an ifdef
which is only enabled for powerpc64.

So you are taking 0 risk with other architecture and as the powerpc
maintainer I'm happy with the patch.

Cheers,
Ben.

Re: [PATCH V2] powerpc: thp: Fix crash on mremap

[Benjamin Herrenschmidt]

On Thu, 2014-02-13 at 08:03 +1100, Benjamin Herrenschmidt wrote:

> It looks very different because the function that needs to be fixed
> changed a lot upstream in 3.13.

 .../...

Hi Greg !

You didn't say if that explanation was to your liking :-)

If it is, do you want Aneesh to re-submit the patch with such an
explanation in the changelog ?

Cheers,
Ben.

> In practice it's *not* very different in behaviour. It's just that
> on powerpc we need to unconditionally call withdraw and deposit when
> moving PTEs or it will crash, due to how we keep the transparent
> huge page in sync with the hash table.
> 
> With the 3.13 code, due to lock breaking introduced by Kirill in
> 3.13-rc's, there's already a generic case for doing that (if we dropped
> the lock). So we just changed the condition to essentially force the
> condition to true to always do it under control of an arch helper.
> 
> The pre-3.13 code didn't do the withdraw and deposit at all in that
> function however, so in that case, the patch (this 3.12 one) basically
> just adds the calls to withdraw and deposit under control of an ifdef
> which is only enabled for powerpc64.
> 
> So you are taking 0 risk with other architecture and as the powerpc
> maintainer I'm happy with the patch.
> 
> Cheers,
> Ben.
> 
>