Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Regards,
Gerald
From: Kirill A. Shutemov <hidden> Date: 2016-02-11 19:09:47
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
From: Kirill A. Shutemov <hidden> Date: 2016-02-11 19:13:00
On Thu, Feb 11, 2016 at 09:09:42PM +0200, Kirill A. Shutemov wrote:
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
From: Sebastian Ott <hidden> Date: 2016-02-12 12:22:00
On Thu, 11 Feb 2016, Kirill A. Shutemov wrote:
On Thu, Feb 11, 2016 at 09:09:42PM +0200, Kirill A. Shutemov wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Based on your suggestion Gerald provided the following patch but sadly it
didn't fix the problem.
Sebastian
---
arch/s390/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
If yes, I'll prepare patch tomorrow (some sleep required).
We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
It would also be good if Martin has a look at this, he'll return on
Monday.
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390).
Why can't we do this ? I did this for ppc64.
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
- pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
+ pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
quoted
If yes, I'll prepare patch tomorrow (some sleep required).
We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
It would also be good if Martin has a look at this, he'll return on
Monday.
On Fri, 12 Feb 2016 09:34:33 +0530
"Aneesh Kumar K.V" [off-list ref] wrote:
Gerald Schaefer [off-list ref] writes:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390).
Why can't we do this ? I did this for ppc64.
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
- pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
+ pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
Wouldn't that semantically change what pmdp_invalidate() was supposed to
do? The comment before the call says "the pmd_trans_huge and
pmd_trans_splitting must remain set at all times on the pmd". So, after
removing pmd_trans_splitting, it seems to be necessary to at least keep
pmd_trans_huge set.
In your case, the pmd would be completely cleared, which may help to find
it in fast_gup with pmd_none(), but I'm not sure if this would open up
other problems, e.g. with concurrent page faults. But I must also admit that
my THP overview got a little rusty.
quoted
So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
quoted
If yes, I'll prepare patch tomorrow (some sleep required).
We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
It would also be good if Martin has a look at this, he'll return on
Monday.
-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 12 Feb 2016 09:34:33 +0530
"Aneesh Kumar K.V" [off-list ref] wrote:
quoted
Gerald Schaefer [off-list ref] writes:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390).
Why can't we do this ? I did this for ppc64.
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
- pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
+ pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
Wouldn't that semantically change what pmdp_invalidate() was supposed to
do? The comment before the call says "the pmd_trans_huge and
pmd_trans_splitting must remain set at all times on the pmd". So, after
removing pmd_trans_splitting, it seems to be necessary to at least keep
pmd_trans_huge set.
In your case, the pmd would be completely cleared, which may help to find
it in fast_gup with pmd_none(), but I'm not sure if this would open up
other problems, e.g. with concurrent page faults. But I must also admit that
my THP overview got a little rusty.
Thinking about this more, I guess, I should not be doing this. Because
this bring in the exit_mmap race that I outlined in the patch even
though the window now is small.
I guess we should fix this in the gup path by checking for what ever
trick we are using to mark the pmd splitting. For ppc64 we clear the
_PAGE_USER. We are ok as long as autonuma is enabled because
pmd_protnone() check will check against _PAGE_USER. But that may not be
sufficient.
-aneesh
From: Will Deacon <hidden> Date: 2016-02-12 10:01:40
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
quoted
If yes, I'll prepare patch tomorrow (some sleep required).
We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
It would also be good if Martin has a look at this, he'll return on
Monday.
Do you have a reliable way to trigger the "random kernel crashes"? We've not
seen anything reported on arm64, but I don't see why we wouldn't be affected
by the same bug and it would be good to confirm and validate a fix.
Cheers,
Will
From: Sebastian Ott <hidden> Date: 2016-02-12 10:13:00
On Fri, 12 Feb 2016, Will Deacon wrote:
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
quoted
If yes, I'll prepare patch tomorrow (some sleep required).
We'll check if adding kick_all_cpus_sync() to pmdp_invalidate() helps.
It would also be good if Martin has a look at this, he'll return on
Monday.
Do you have a reliable way to trigger the "random kernel crashes"? We've not
seen anything reported on arm64, but I don't see why we wouldn't be affected
by the same bug and it would be good to confirm and validate a fix.
My testcase was compiling the kernel. Most of the time my test system
didn't survive a single compile run. During bisect I did at least 20
compile runs to flag a commit as good.
Sebastian
From: Will Deacon <hidden> Date: 2016-02-12 15:52:21
On Fri, Feb 12, 2016 at 11:12:34AM +0100, Sebastian Ott wrote:
On Fri, 12 Feb 2016, Will Deacon wrote:
quoted
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
[...]
quoted
Do you have a reliable way to trigger the "random kernel crashes"? We've not
seen anything reported on arm64, but I don't see why we wouldn't be affected
by the same bug and it would be good to confirm and validate a fix.
My testcase was compiling the kernel. Most of the time my test system
didn't survive a single compile run. During bisect I did at least 20
compile runs to flag a commit as good.
I've been building kernels all day with -rc3 on my arm64 box and haven't
seen any problems yet.. :/.
I'll leave it going over the weekend.
Will
From: Kirill A. Shutemov <hidden> Date: 2016-02-12 15:41:21
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for me :-/
Does it make the pmd !pmd_present()?
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
--
Kirill A. Shutemov
From: Christian Borntraeger <hidden> Date: 2016-02-12 15:57:38
On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for me :-/
Does it make the pmd !pmd_present()?
It uses the idte instruction, which in an atomic fashion flushes the associated
TLB entry and changes the value of the pmd entry to invalid. This comes from the
HW requirement to not change a PTE/PMD that might be still in use, other than
with special instructions that does the tlb handling and the invalidation together.
(It also does some some other magic to the attach_count, which might hold off
finish_arch_post_lock_switch while some flushing is happening, but this should
be unrelated here)
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger [off-list ref] wrote:
On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
quoted
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for me :-/
Does it make the pmd !pmd_present()?
It uses the idte instruction, which in an atomic fashion flushes the associated
TLB entry and changes the value of the pmd entry to invalid. This comes from the
HW requirement to not change a PTE/PMD that might be still in use, other than
with special instructions that does the tlb handling and the invalidation together.
Correct, and it does _not_ make the pmd !pmd_present(), that would only be the
case after a _clear_flush(). It only marks the pmd as invalid and flushes,
so that it cannot generate a new TLB entry before the following pmd_populate(),
but it keeps its other content. This is to fulfill the requirements outlined in
the comment in mm/huge_memory.c before the call to pmdp_invalidate(). And
independent from that comment, we would need such an _invalidate() or
_clear_flush() on s390 before the pmd_populate() because of the HW details
that Christian described.
Reading the comment again, I do now notice that it also says "mark the current
pmd notpresent", which we cannot do w/o losing the huge and (formerly) splitting
bits, but it also shouldn't be needed to provide the "single TLB guarantee" that
is required from the comment. So, a pmd_present() check on s390 in this state
would still return true. Not sure yet if this is a problem, need more thinking,
this behavior was already present before the THP rework but maybe it was OK
before and is not OK now.
At least for fast_gup this should not be a problem though.
(It also does some some other magic to the attach_count, which might hold off
finish_arch_post_lock_switch while some flushing is happening, but this should
be unrelated here)
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Kirill A. Shutemov <hidden> Date: 2016-02-12 23:15:17
On Fri, Feb 12, 2016 at 06:16:40PM +0100, Gerald Schaefer wrote:
On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger [off-list ref] wrote:
quoted
On 02/12/2016 04:41 PM, Kirill A. Shutemov wrote:
quoted
On Thu, Feb 11, 2016 at 08:57:02PM +0100, Gerald Schaefer wrote:
quoted
On Thu, 11 Feb 2016 21:09:42 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Thu, Feb 11, 2016 at 07:22:23PM +0100, Gerald Schaefer wrote:
quoted
Hi,
Sebastian Ott reported random kernel crashes beginning with v4.5-rc1 and
he also bisected this to commit 61f5d698 "mm: re-enable THP". Further
review of the THP rework patches, which cannot be bisected, revealed
commit fecffad "s390, thp: remove infrastructure for handling splitting PMDs"
(and also similar commits for other archs).
This commit removes the THP splitting bit and also the architecture
implementation of pmdp_splitting_flush(), which took care of the IPI for
fast_gup serialization. The commit message says
pmdp_splitting_flush() is not needed too: on splitting PMD we will do
pmdp_clear_flush() + set_pte_at(). pmdp_clear_flush() will do IPI as
needed for fast_gup
The assumption that a TLB flush will also produce an IPI is wrong on s390,
and maybe also on other architectures, and I thought that this was actually
the main reason for having an arch-specific pmdp_splitting_flush().
At least PowerPC and ARM also had an individual implementation of
pmdp_splitting_flush() that used kick_all_cpus_sync() instead of a TLB
flush to send the IPI, and those were also removed. Putting the arch
maintainers and mailing lists on cc to verify.
On s390 this will break the IPI serialization against fast_gup, which
would certainly explain the random kernel crashes, please revert or fix
the pmdp_splitting_flush() removal.
Hmm, not sure about that. After pmdp_invalidate(), a pmd_none() check in
fast_gup will still return false, because the pmd is not empty (at least
on s390). So I don't see spontaneously how it will help fast_gup to break
out to the slow path in case of THP splitting.
What pmdp_flush_direct() does in pmdp_invalidate()? It's hard to unwrap for me :-/
Does it make the pmd !pmd_present()?
It uses the idte instruction, which in an atomic fashion flushes the associated
TLB entry and changes the value of the pmd entry to invalid. This comes from the
HW requirement to not change a PTE/PMD that might be still in use, other than
with special instructions that does the tlb handling and the invalidation together.
Correct, and it does _not_ make the pmd !pmd_present(), that would only be the
case after a _clear_flush(). It only marks the pmd as invalid and flushes,
so that it cannot generate a new TLB entry before the following pmd_populate(),
but it keeps its other content. This is to fulfill the requirements outlined in
the comment in mm/huge_memory.c before the call to pmdp_invalidate(). And
independent from that comment, we would need such an _invalidate() or
_clear_flush() on s390 before the pmd_populate() because of the HW details
that Christian described.
Reading the comment again, I do now notice that it also says "mark the current
pmd notpresent", which we cannot do w/o losing the huge and (formerly) splitting
bits, but it also shouldn't be needed to provide the "single TLB guarantee" that
is required from the comment. So, a pmd_present() check on s390 in this state
would still return true. Not sure yet if this is a problem, need more thinking,
this behavior was already present before the THP rework but maybe it was OK
before and is not OK now.
At least for fast_gup this should not be a problem though.
I'm trying to wrap my head around the issue and I don't think missing
serialization with gup_fast is the cause -- we just don't need it
anymore.
Previously, __split_huge_page_splitting() required serialization against
gup_fast to make sure nobody can obtain new reference to the page after
__split_huge_page_splitting() returns. This was a way to stabilize page
references before starting to distribute them from head page to tail
pages.
With new refcounting, we don't care about this. Splitting PMD is now
decoupled from splitting underlying compound page. It's okay to get new
pins after split_huge_pmd(). To stabilize page references during
split_huge_page() we rely on setting up migration entries once all
pmds are split into page table entries.
The theory that serialization against gup_fast is not a root cause of the
crashes is consistent no crashes on arm64. Problem is somewhere else.
quoted
(It also does some some other magic to the attach_count, which might hold off
finish_arch_post_lock_switch while some flushing is happening, but this should
be unrelated here)
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
One bit was freed up by the commit you've pointed to as a cause.
I wounder If it's possible that screw up something while removing it? I
don't see it, but who knows.
Could you check if revert of fecffad25458 helps?
And could you share how crashes looks like? I haven't seen backtraces yet.
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
And could you share how crashes looks like? I haven't seen backtraces yet.
Sure. I didn't because they really looked random to me. Most of the time
in rcu or list debugging but I thought these have just been the messenger
observing a corruption first. Anyhow, here is an older one that might look
interesting:
[ 59.851421] list_del corruption. next->prev should be 000000006e1eb000, but was 0000000000000400
This kinda interesting: 0x400 is TAIL_MAPPING.. Hm..
Could you check if you see the problem on commit 1c290f642101 and its
immediate parent?
--
Kirill A. Shutemov
=20
And could you share how crashes looks like? I haven't seen backtraces=
yet.
quoted
quoted
=20
=20
Sure. I didn't because they really looked random to me. Most of the time
in rcu or list debugging but I thought these have just been the messeng=
er
quoted
observing a corruption first. Anyhow, here is an older one that might l=
ook
quoted
interesting:
=20
[ 59.851421] list_del corruption. next->prev should be 000000006e1eb0=
00, but was 0000000000000400
=20
This kinda interesting: 0x400 is TAIL_MAPPING.. Hm..
=20
Could you check if you see the problem on commit 1c290f642101 and its
immediate parent?
=20
How should the page->mapping poison end up as next->prev in the list of
pre-allocated THP splitting page tables? Also, commit 1c290f642101
is before the THP rework, at least the non-bisectable part, so we should
expect not to see the problem there.
0x400 is also the value of an empty pte on s390, and the thp_deposit/withdr=
aw
listheads are placed inside the pre-allocated pagetables instead of page->l=
ru,
because we have 2K pagetables on s390 and cannot use struct page =3D=3D pgt=
able_t.
So, for example, two concurrent withdraws could produce such a list
corruption, because the first withdraw will overwrite the listhead at the
beginning of the pagetable with 2 empty ptes.
Has anything changed regarding the general THP deposit/withdraw logic?
And could you share how crashes looks like? I haven't seen backtraces yet.
Sure. I didn't because they really looked random to me. Most of the time
in rcu or list debugging but I thought these have just been the messenger
observing a corruption first. Anyhow, here is an older one that might look
interesting:
[ 59.851421] list_del corruption. next->prev should be 000000006e1eb000, but was 0000000000000400
This kinda interesting: 0x400 is TAIL_MAPPING.. Hm..
Could you check if you see the problem on commit 1c290f642101 and its
immediate parent?
How should the page->mapping poison end up as next->prev in the list of
pre-allocated THP splitting page tables?
May be pgtable was casted to struct page or something. I don't know.
Also, commit 1c290f642101 is before the THP rework, at least the
non-bisectable part, so we should expect not to see the problem there.
Just to make sure: commit 122afea9626a is fine, commit 61f5d698cc97
crashes. Correct?
0x400 is also the value of an empty pte on s390, and the thp_deposit/withdraw
listheads are placed inside the pre-allocated pagetables instead of page->lru,
because we have 2K pagetables on s390 and cannot use struct page == pgtable_t.
0x400 from empty pte makes more sense than TAIL_MAPPING. But I guess it
worth changing TAIL_MAPPING to some other value to make sure.
So, for example, two concurrent withdraws could produce such a list
corruption, because the first withdraw will overwrite the listhead at the
beginning of the pagetable with 2 empty ptes.
Has anything changed regarding the general THP deposit/withdraw logic?
I don't see any changes in this area.
To eliminate one more variable, I would propose to disable split pmd lock
for testing and check if it makes difference.
Is there any chance that I'll be able to trigger the bug using QEMU?
Does anybody have an QEMU image I can use?
--
Kirill A. Shutemov
=20
And could you share how crashes looks like? I haven't seen backtr=
aces yet.
quoted
quoted
quoted
quoted
=20
=20
Sure. I didn't because they really looked random to me. Most of the=
time
quoted
quoted
quoted
in rcu or list debugging but I thought these have just been the mes=
senger
quoted
quoted
quoted
observing a corruption first. Anyhow, here is an older one that mig=
ht look
quoted
quoted
quoted
interesting:
=20
[ 59.851421] list_del corruption. next->prev should be 000000006e=
1eb000, but was 0000000000000400
quoted
quoted
=20
This kinda interesting: 0x400 is TAIL_MAPPING.. Hm..
=20
Could you check if you see the problem on commit 1c290f642101 and its
immediate parent?
=20
=20
How should the page->mapping poison end up as next->prev in the list of
pre-allocated THP splitting page tables?
=20
May be pgtable was casted to struct page or something. I don't know.
=20
quoted
Also, commit 1c290f642101 is before the THP rework, at least the
non-bisectable part, so we should expect not to see the problem there.
=20
Just to make sure: commit 122afea9626a is fine, commit 61f5d698cc97
crashes. Correct?
=20
quoted
0x400 is also the value of an empty pte on s390, and the thp_deposit/wi=
thdraw
quoted
listheads are placed inside the pre-allocated pagetables instead of pag=
e->lru,
quoted
because we have 2K pagetables on s390 and cannot use struct page =3D=3D=
pgtable_t.
=20
0x400 from empty pte makes more sense than TAIL_MAPPING. But I guess it
worth changing TAIL_MAPPING to some other value to make sure.
Right, but we cannot trigger this list corruption symptom reliably, in fact
I didn't hit it at all during the last runs, and previous crash logs also
showed list corruptions with other values than 0x400, which may hint towards
concurrent pagetable freeing and re-use, given that our THP splitting paget=
able
listhead is located inside the pre-allocated pagetables.
=20
quoted
So, for example, two concurrent withdraws could produce such a list
corruption, because the first withdraw will overwrite the listhead at t=
he
quoted
beginning of the pagetable with 2 empty ptes.
=20
Has anything changed regarding the general THP deposit/withdraw logic?
=20
I don't see any changes in this area.
=20
To eliminate one more variable, I would propose to disable split pmd lock
for testing and check if it makes difference.
Disabling ARCH_ENABLE_SPLIT_PMD_PTLOCK didn't make any difference, other
than maybe a little reduction in "randomness" of the crashes, but that
may be pure coincidence. Out of about 10 runs, I always ended up with either
ODEBUG "WARNING: at lib/debugobjects.c:263" and subsequent "kernel BUG at
mm/slub.c:3629", or "bad swap file / page map" with subsequent "kernel BUG
at kernel/cred.c:142", see below for the full traces.
=20
Is there any chance that I'll be able to trigger the bug using QEMU?
Does anybody have an QEMU image I can use?
=20
From: Kirill A. Shutemov <hidden> Date: 2016-02-17 15:28:10
On Tue, Feb 16, 2016 at 05:24:44PM +0100, Gerald Schaefer wrote:
On Mon, 15 Feb 2016 23:35:26 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
Is there any chance that I'll be able to trigger the bug using QEMU?
Does anybody have an QEMU image I can use?
I have no image, but trying to reproduce this under virtualization may
help to trigger this also on other architectures. After ruling out IPI
vs. fast_gup I do not really see why this should be arch-specific, and
it wouldn't be the first time that we hit subtle races first on s390, due
to our virtualized environment (my test case is make -j20 with 10 CPUs and
4GB of memory, no swap).
Could you post your kernel config?
It would be nice also to check if disabling split_huge_page() would make
any difference:
From: Sebastian Ott <hidden> Date: 2016-02-17 19:05:23
Hi,
On Wed, 17 Feb 2016, Kirill A. Shutemov wrote:
On Tue, Feb 16, 2016 at 05:24:44PM +0100, Gerald Schaefer wrote:
quoted
On Mon, 15 Feb 2016 23:35:26 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
Is there any chance that I'll be able to trigger the bug using QEMU?
Does anybody have an QEMU image I can use?
I have no image, but trying to reproduce this under virtualization may
help to trigger this also on other architectures. After ruling out IPI
vs. fast_gup I do not really see why this should be arch-specific, and
it wouldn't be the first time that we hit subtle races first on s390, due
to our virtualized environment (my test case is make -j20 with 10 CPUs and
4GB of memory, no swap).
Could you post your kernel config?
Attached.
quoted hunk
It would be nice also to check if disabling split_huge_page() would make
any difference:
The BUG at mm/huge_memory.c:2884 is interesting, it's the BUG_ON(!pte_none(*pte))
check in __split_huge_pmd_locked(). Obviously we expect the pre-allocated
pagetables to be empty, but in collapse_huge_page() we deposit the original
pagetable instead of allocating a new (empty) one. This saves an allocation,
which is good, but doesn't that mean that if such a collapsed hugepage will
ever be split, we will always run into the BUG_ON(!pte_none(*pte)), or one
of the two other VM_BUG_ONs in mm/huge_memory.c that check the same?
This behavior is not new, it was the same before the THP rework, so I do not
assume that it is related to the current problems, maybe with the exception
of this specific crash. I never saw the BUG at mm/huge_memory.c:2884 myself,
and the other crashes probably cannot be explained with this. Maybe I am
also missing something, but I do not see how collapse_huge_page() and the
(non-empty) pgtable deposit there can work out with the BUG_ON(!pte_none(*pte))
checks. Any thoughts?
The BUG at mm/huge_memory.c:2884 is interesting, it's the BUG_ON(!pte_none(*pte))
check in __split_huge_pmd_locked(). Obviously we expect the pre-allocated
pagetables to be empty, but in collapse_huge_page() we deposit the original
pagetable instead of allocating a new (empty) one. This saves an allocation,
which is good, but doesn't that mean that if such a collapsed hugepage will
ever be split, we will always run into the BUG_ON(!pte_none(*pte)), or one
of the two other VM_BUG_ONs in mm/huge_memory.c that check the same?
This behavior is not new, it was the same before the THP rework, so I do not
assume that it is related to the current problems, maybe with the exception
of this specific crash. I never saw the BUG at mm/huge_memory.c:2884 myself,
and the other crashes probably cannot be explained with this. Maybe I am
also missing something, but I do not see how collapse_huge_page() and the
(non-empty) pgtable deposit there can work out with the BUG_ON(!pte_none(*pte))
checks. Any thoughts?
I don't think there's a problem: ptes in the pgtable are cleared with
pte_clear() in __collapse_huge_page_copy().
--
Kirill A. Shutemov
The BUG at mm/huge_memory.c:2884 is interesting, it's the BUG_ON(!pte_none(*pte))
check in __split_huge_pmd_locked(). Obviously we expect the pre-allocated
pagetables to be empty, but in collapse_huge_page() we deposit the original
pagetable instead of allocating a new (empty) one. This saves an allocation,
which is good, but doesn't that mean that if such a collapsed hugepage will
ever be split, we will always run into the BUG_ON(!pte_none(*pte)), or one
of the two other VM_BUG_ONs in mm/huge_memory.c that check the same?
This behavior is not new, it was the same before the THP rework, so I do not
assume that it is related to the current problems, maybe with the exception
of this specific crash. I never saw the BUG at mm/huge_memory.c:2884 myself,
and the other crashes probably cannot be explained with this. Maybe I am
also missing something, but I do not see how collapse_huge_page() and the
(non-empty) pgtable deposit there can work out with the BUG_ON(!pte_none(*pte))
checks. Any thoughts?
I don't think there's a problem: ptes in the pgtable are cleared with
pte_clear() in __collapse_huge_page_copy().
Ah OK, I didn't see that. Still the BUG_ON() tells us that something went
wrong with the pre-allocated pagetable, or at least with the deposit/withdraw
list, or both. Given that on s390 we keep the listheads for the deposit/withdraw
list inside the pre-allocated pgtables, instead of the struct pages, it may
also explain why we see don't the problems on x86.
We already have the list corruption warning in exit_mmap -> zap_huge_pmd ->
withdraw, and from time to time I also hit the BUG_ON(page->pmd_huge_pte)
in exit_mmap -> free_pgtables -> free_pmd_range, which also indicates some
issues with the deposit/withdraw list, see below:
[ 2489.384069] page:000003d101aa6f00 count:1 mapcount:0 mapping: (null) index:0x0
[ 2489.384075] flags: 0x0()
[ 2489.384078] page dumped because: VM_BUG_ON_PAGE(page->pmd_huge_pte)
[ 2489.384086] ------------[ cut here ]------------
[ 2489.384088] kernel BUG at include/linux/mm.h:1700!
[ 2489.384131] illegal operation: 0001 ilc:1 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 2489.384137] Modules linked in: bridge stp llc mlx4_ib ib_sa ib_mad mlx4_en ib_core vxlan udp_tunnel ptp pps_core ib_addr ghash_s390 prng ecb mlx4_core aes_s390 des_s390 des_generic sha512_s390 sha256_s390 sha1_s390 sha_common eadm_sch dm_mod vhost_net tun vhost macvtap macvlan kvm autofs4
[ 2489.384173] CPU: 5 PID: 173619 Comm: cc1 Tainted: G B W 4.5.0-rc3-00083-gc05235d #10
[ 2489.384176] task: 00000000c54d0000 ti: 0000000060504000 task.ti: 0000000060504000
[ 2489.384179] Krnl PSW : 0704c00180000000 0000000000283cf4 (free_pgd_range+0x334/0x460)
[ 2489.384184] R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
Krnl GPRS: 0000000001a161c7 0000000000000000 0000000000000037 0000000000000000
[ 2489.384189] 0000000000283cf0 0000000000000000 000003ff7d980000 0000000060507e18
[ 2489.384192] 000003ff00000000 0000000075e43ff0 000003ff7d97ffff 000003ff7d980000
[ 2489.384195] 000000006a9bc000 00000000006cc390 0000000000283cf0 0000000060507c68
[ 2489.384201] Krnl Code: 0000000000283ce4: c030002e14dd larl %%r3,84669e
0000000000283cea: c0e5ffffd217 brasl %%r14,27e118
#0000000000283cf0: a7f40001 brc 15,283cf2
>0000000000283cf4: c0e5fffffe5a brasl %%r14,2839a8
0000000000283cfa: b9040027 lgr %%r2,%%r7
0000000000283cfe: b904003c lgr %%r3,%%r12
0000000000283d02: c0e5fff509e3 brasl %%r14,1250c8
0000000000283d08: e31070000004 lg %%r1,0(%%r7)
[ 2489.384221] Call Trace:
[ 2489.384224] ([<0000000000283cf0>] free_pgd_range+0x330/0x460)
[ 2489.384227] [<0000000000283f38>] free_pgtables+0x118/0x148
[ 2489.384230] [<000000000028c32e>] exit_mmap+0xd6/0x300
[ 2489.384233] [<0000000000134d70>] mmput+0x90/0x118
[ 2489.384235] [<000000000013a55c>] do_exit+0x41c/0xd18
[ 2489.384238] [<000000000013c3c2>] do_group_exit+0x92/0xd8
[ 2489.384241] [<000000000013c432>] SyS_exit_group+0x2a/0x30
[ 2489.384244] [<00000000006b1a36>] system_call+0xd6/0x258
[ 2489.384246] [<000003ff7d343698>] 0x3ff7d343698
[ 2489.384248] INFO: lockdep is turned off.
[ 2489.384251] Last Breaking-Event-Address:
[ 2489.384253] [<0000000000283cf0>] free_pgd_range+0x330/0x460
[ 2489.384256]
[ 2489.384258] Kernel panic - not syncing: Fatal exception: panic_on_oops
I'll try to add a BUG_ON(pmd_huge(*pmd)) to free_pte_range() and see if that
catches anything, and I'll also check if debug_cow = 1 or use_zero_page = 0
makes any difference.
The BUG at mm/huge_memory.c:2884 is interesting, it's the BUG_ON(!pte_none(*pte))
check in __split_huge_pmd_locked(). Obviously we expect the pre-allocated
pagetables to be empty, but in collapse_huge_page() we deposit the original
pagetable instead of allocating a new (empty) one. This saves an allocation,
which is good, but doesn't that mean that if such a collapsed hugepage will
ever be split, we will always run into the BUG_ON(!pte_none(*pte)), or one
of the two other VM_BUG_ONs in mm/huge_memory.c that check the same?
This behavior is not new, it was the same before the THP rework, so I do not
assume that it is related to the current problems, maybe with the exception
of this specific crash. I never saw the BUG at mm/huge_memory.c:2884 myself,
and the other crashes probably cannot be explained with this. Maybe I am
also missing something, but I do not see how collapse_huge_page() and the
(non-empty) pgtable deposit there can work out with the BUG_ON(!pte_none(*pte))
checks. Any thoughts?
I don't think there's a problem: ptes in the pgtable are cleared with
pte_clear() in __collapse_huge_page_copy().
Ah OK, I didn't see that. Still the BUG_ON() tells us that something went
wrong with the pre-allocated pagetable, or at least with the deposit/withdraw
list, or both. Given that on s390 we keep the listheads for the deposit/withdraw
list inside the pre-allocated pgtables, instead of the struct pages, it may
also explain why we see don't the problems on x86.
We already have the list corruption warning in exit_mmap -> zap_huge_pmd ->
withdraw, and from time to time I also hit the BUG_ON(page->pmd_huge_pte)
in exit_mmap -> free_pgtables -> free_pmd_range, which also indicates some
issues with the deposit/withdraw list, see below:
[ 2489.384069] page:000003d101aa6f00 count:1 mapcount:0 mapping: (null) index:0x0
[ 2489.384075] flags: 0x0()
[ 2489.384078] page dumped because: VM_BUG_ON_PAGE(page->pmd_huge_pte)
[ 2489.384086] ------------[ cut here ]------------
[ 2489.384088] kernel BUG at include/linux/mm.h:1700!
[ 2489.384131] illegal operation: 0001 ilc:1 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 2489.384137] Modules linked in: bridge stp llc mlx4_ib ib_sa ib_mad mlx4_en ib_core vxlan udp_tunnel ptp pps_core ib_addr ghash_s390 prng ecb mlx4_core aes_s390 des_s390 des_generic sha512_s390 sha256_s390 sha1_s390 sha_common eadm_sch dm_mod vhost_net tun vhost macvtap macvlan kvm autofs4
[ 2489.384173] CPU: 5 PID: 173619 Comm: cc1 Tainted: G B W 4.5.0-rc3-00083-gc05235d #10
[ 2489.384176] task: 00000000c54d0000 ti: 0000000060504000 task.ti: 0000000060504000
[ 2489.384179] Krnl PSW : 0704c00180000000 0000000000283cf4 (free_pgd_range+0x334/0x460)
[ 2489.384184] R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
Krnl GPRS: 0000000001a161c7 0000000000000000 0000000000000037 0000000000000000
[ 2489.384189] 0000000000283cf0 0000000000000000 000003ff7d980000 0000000060507e18
[ 2489.384192] 000003ff00000000 0000000075e43ff0 000003ff7d97ffff 000003ff7d980000
[ 2489.384195] 000000006a9bc000 00000000006cc390 0000000000283cf0 0000000060507c68
[ 2489.384201] Krnl Code: 0000000000283ce4: c030002e14dd larl %%r3,84669e
0000000000283cea: c0e5ffffd217 brasl %%r14,27e118
#0000000000283cf0: a7f40001 brc 15,283cf2
>0000000000283cf4: c0e5fffffe5a brasl %%r14,2839a8
0000000000283cfa: b9040027 lgr %%r2,%%r7
0000000000283cfe: b904003c lgr %%r3,%%r12
0000000000283d02: c0e5fff509e3 brasl %%r14,1250c8
0000000000283d08: e31070000004 lg %%r1,0(%%r7)
[ 2489.384221] Call Trace:
[ 2489.384224] ([<0000000000283cf0>] free_pgd_range+0x330/0x460)
[ 2489.384227] [<0000000000283f38>] free_pgtables+0x118/0x148
[ 2489.384230] [<000000000028c32e>] exit_mmap+0xd6/0x300
[ 2489.384233] [<0000000000134d70>] mmput+0x90/0x118
[ 2489.384235] [<000000000013a55c>] do_exit+0x41c/0xd18
[ 2489.384238] [<000000000013c3c2>] do_group_exit+0x92/0xd8
[ 2489.384241] [<000000000013c432>] SyS_exit_group+0x2a/0x30
[ 2489.384244] [<00000000006b1a36>] system_call+0xd6/0x258
[ 2489.384246] [<000003ff7d343698>] 0x3ff7d343698
[ 2489.384248] INFO: lockdep is turned off.
[ 2489.384251] Last Breaking-Event-Address:
[ 2489.384253] [<0000000000283cf0>] free_pgd_range+0x330/0x460
[ 2489.384256]
[ 2489.384258] Kernel panic - not syncing: Fatal exception: panic_on_oops
I'll try to add a BUG_ON(pmd_huge(*pmd)) to free_pte_range() and see if that
catches anything, and I'll also check if debug_cow = 1 or use_zero_page = 0
makes any difference.
I worth minimizing kernel config on which you can see the bug. Things like
CONFIG_DEBUG_PAGEALLOC used to interfere with THP before.
You can also disable khugepaged, just in case.
One more thing: try add smp_wmb() in pgtable_trans_huge_withdraw() just
before return to make sure all CPUs sees _PAGE_INVALID.
I don't think it would make a difference. Again, just in case.
--
Kirill A. Shutemov
On Sat, 13 Feb 2016 01:15:10 +0200
"Kirill A. Shutemov" [off-list ref] wrote:
I'm trying to wrap my head around the issue and I don't think missing
serialization with gup_fast is the cause -- we just don't need it
anymore.
Previously, __split_huge_page_splitting() required serialization against
gup_fast to make sure nobody can obtain new reference to the page after
__split_huge_page_splitting() returns. This was a way to stabilize page
references before starting to distribute them from head page to tail
pages.
With new refcounting, we don't care about this. Splitting PMD is now
decoupled from splitting underlying compound page. It's okay to get new
pins after split_huge_pmd(). To stabilize page references during
split_huge_page() we rely on setting up migration entries once all
pmds are split into page table entries.
The theory that serialization against gup_fast is not a root cause of the
crashes is consistent no crashes on arm64. Problem is somewhere else.
Hmm, ok, I just relied on the commit message of commit fecffad25458, which
talks about "pmdp_clear_flush() will do IPI as needed for fast_gup", as well
as the comments in mm/gup.c, which also still talk about IPIs and THP
splitting.
If IPI serialization with fast_gup is not needed anymore for THP splitting,
please fix at least the comments in mm/gup.c.
quoted
quoted
(It also does some some other magic to the attach_count, which might hold off
finish_arch_post_lock_switch while some flushing is happening, but this should
be unrelated here)
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
One bit was freed up by the commit you've pointed to as a cause.
I wounder If it's possible that screw up something while removing it? I
don't see it, but who knows.
Could you check if revert of fecffad25458 helps?
I tried reverting fecffad25458, plus re-adding a call to pmdp_splitting_flush()
in __split_huge_pmd_locked(), and I could still reproduce the crashes, so I
guess it really isn't related to fast_gup vs. THP splitting.
And could you share how crashes looks like? I haven't seen backtraces yet.
quoted
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
From: Kirill A. Shutemov <hidden> Date: 2016-02-23 12:12:45
On Fri, Feb 12, 2016 at 06:16:40PM +0100, Gerald Schaefer wrote:
On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger [off-list ref] wrote:
quoted
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
I still worry about pmd_present(). It looks wrong to me. I wounder if
patch below makes a difference.
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
I'm not sure that the patch is correct wrt yound/old pmds and I have no
way to test it...
On Tue, Feb 23, 2016 at 2:32 AM, Kirill A. Shutemov
[off-list ref] wrote:
I still worry about pmd_present(). It looks wrong to me. I wounder if
patch below makes a difference.
Let's hope that's it, but in the meantime I do want to start the
discussion about what to do if it isn't. We're at rc5, and 4.5 is just
a few weeks away, and so far this issue hasn't gone anywhere.
So the *good* scenario is that your pmd_present() patch fixes it, and
we can all take a relieved breath.
But if not, what then? It looks like we have two options:
(a) do a (hopefully minimal) revert.
I say "hopefully minimal", but I suspect the revert is going to
have to undo pretty much all of the core THP changes. I'd hate to see
that, because I really liked the cleanups.
(b) mark THP as "depends on !S390" in the 4.5 release
The (b) option is obviously much simpler, but it's a regression. I
really don't like it, even if it generally shouldn't be the kind of
regression that is actually user-noticeable (apart from performance).
I also hate the fact that while the problem only seems to happen on
s390, we don't even understand it, so maybe it's a more generic issue
that for some reason just ends up being *much* more noticeable on one
odd architecture that happens to be a bit different.
I'm inclined to think of (b) as just a "give us more time to figure it
out" thing, but I'm also worried that it will then make people not
pursue this issue.
How big is a revert patch that makes THP work on s390 again? Can we do
a revert that keeps the infrastructure intact and makes it easy to
revisit the THP cleanups later? Or is the revert inevitably going to
be all the core patches in that series?
Linus
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
On Fri, Feb 12, 2016 at 06:16:40PM +0100, Gerald Schaefer wrote:
quoted
On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger [off-list ref] wrote:
quoted
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
I still worry about pmd_present(). It looks wrong to me. I wounder if
patch below makes a difference.
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
FWIW, and since the ARM arch-list is already on cc, I think there is
an issue with pmdp_invalidate() on ARM, since it also seems to clear
the trans_huge (and formerly trans_splitting) bit, which actually makes
the pmd !pmd_present(), but it violates the other requirement from the
comment:
"the pmd_trans_huge and pmd_trans_splitting must remain set at all times
on the pmd until the split is complete for this pmd"
quoted hunk
I'm not sure that the patch is correct wrt yound/old pmds and I have no
way to test it...
@@ -490,7 +490,7 @@ static inline int pud_bad(pud_t pud)staticinlineintpmd_present(pmd_tpmd){-returnpmd_val(pmd)!=_SEGMENT_ENTRY_INVALID;+return!(pmd_val(pmd)&_SEGMENT_ENTRY_INVALID);}staticinlineintpmd_none(pmd_tpmd)
No, that would not work well with young rw and ro pmds. We do now
have an extra free bit in the pmd on s390, after the removal of the
splitting bit, so we could try to implement pmd_present() with that
sw bit, but that would also require several not-so-trivial changes
to the other code in arch/s390/include/asm/pgtable.h.
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
From: Will Deacon <hidden> Date: 2016-02-23 18:47:15
[adding Steve, since he worked on THP for 32-bit ARM]
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
FWIW, and since the ARM arch-list is already on cc, I think there is
an issue with pmdp_invalidate() on ARM, since it also seems to clear
the trans_huge (and formerly trans_splitting) bit, which actually makes
the pmd !pmd_present(), but it violates the other requirement from the
comment:
"the pmd_trans_huge and pmd_trans_splitting must remain set at all times
on the pmd until the split is complete for this pmd"
I've only been testing this for arm64 (where I'm yet to see a problem),
but we use the generic pmdp_invalidate implementation from
mm/pgtable-generic.c there. On arm64, pmd_trans_huge will return true
after pmd_mknotpresent. On arm, it does look to be buggy, since it nukes
the entire entry... Steve?
Will
From: Steve Capper <hidden> Date: 2016-02-25 15:49:36
On 23 February 2016 at 18:47, Will Deacon [off-list ref] wrote:
[adding Steve, since he worked on THP for 32-bit ARM]
Apologies for my late reply...
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
FWIW, and since the ARM arch-list is already on cc, I think there is
an issue with pmdp_invalidate() on ARM, since it also seems to clear
the trans_huge (and formerly trans_splitting) bit, which actually makes
the pmd !pmd_present(), but it violates the other requirement from the
comment:
"the pmd_trans_huge and pmd_trans_splitting must remain set at all times
on the pmd until the split is complete for this pmd"
I've only been testing this for arm64 (where I'm yet to see a problem),
but we use the generic pmdp_invalidate implementation from
mm/pgtable-generic.c there. On arm64, pmd_trans_huge will return true
after pmd_mknotpresent. On arm, it does look to be buggy, since it nukes
the entire entry... Steve?
pmd_mknotpresent on arm looks inconsistent with the other
architectures and can be changed.
Having had a look at the usage, I can't see it causing an immediate
problem (that needs to be addressed by an emergency patch).
We don't have a notion of splitting pmds (so there is no splitting
information to lose), and the only usage I could see of
pmd_mknotpresent was:
pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
In mm/huge_memory.c, around line 3588.
So we invalidate the entry (which puts down a faulting entry from
pmd_mknotpresent and invalidates tlb), then immediately put down a
table entry with pmd_populate.
I have run a 32-bit ARM test kernel and exacerbated THP splits (that's
what took me time), and I didn't notice any problems with 4.5-rc5.
Cheers,
--
Steve
Will
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
From: Kirill A. Shutemov <hidden> Date: 2016-02-25 16:01:16
On Thu, Feb 25, 2016 at 03:49:33PM +0000, Steve Capper wrote:
On 23 February 2016 at 18:47, Will Deacon [off-list ref] wrote:
quoted
[adding Steve, since he worked on THP for 32-bit ARM]
Apologies for my late reply...
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
FWIW, and since the ARM arch-list is already on cc, I think there is
an issue with pmdp_invalidate() on ARM, since it also seems to clear
the trans_huge (and formerly trans_splitting) bit, which actually makes
the pmd !pmd_present(), but it violates the other requirement from the
comment:
"the pmd_trans_huge and pmd_trans_splitting must remain set at all times
on the pmd until the split is complete for this pmd"
I've only been testing this for arm64 (where I'm yet to see a problem),
but we use the generic pmdp_invalidate implementation from
mm/pgtable-generic.c there. On arm64, pmd_trans_huge will return true
after pmd_mknotpresent. On arm, it does look to be buggy, since it nukes
the entire entry... Steve?
pmd_mknotpresent on arm looks inconsistent with the other
architectures and can be changed.
Having had a look at the usage, I can't see it causing an immediate
problem (that needs to be addressed by an emergency patch).
We don't have a notion of splitting pmds (so there is no splitting
information to lose), and the only usage I could see of
pmd_mknotpresent was:
pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
In mm/huge_memory.c, around line 3588.
So we invalidate the entry (which puts down a faulting entry from
pmd_mknotpresent and invalidates tlb), then immediately put down a
table entry with pmd_populate.
I have run a 32-bit ARM test kernel and exacerbated THP splits (that's
what took me time), and I didn't notice any problems with 4.5-rc5.
If I read code correctly, your pmd_mknotpresent() makes the pmd
pmd_none(), right? If yes, it's a problem.
It introduces race I've described here:
https://marc.info/?l=linux-mm&m=144723658100512&w=4
Basically, if zap_pmd_range() would see pmd_none() between
pmdp_mknotpresent() and pmd_populate(), we're screwed.
The race window is small, but it's there.
--
Kirill A. Shutemov
From: Steve Capper <hidden> Date: 2016-02-25 16:08:31
On 25 February 2016 at 16:01, Kirill A. Shutemov [off-list ref] wrote:
On Thu, Feb 25, 2016 at 03:49:33PM +0000, Steve Capper wrote:
quoted
On 23 February 2016 at 18:47, Will Deacon [off-list ref] wrote:
quoted
[adding Steve, since he worked on THP for 32-bit ARM]
Apologies for my late reply...
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
FWIW, and since the ARM arch-list is already on cc, I think there is
an issue with pmdp_invalidate() on ARM, since it also seems to clear
the trans_huge (and formerly trans_splitting) bit, which actually makes
the pmd !pmd_present(), but it violates the other requirement from the
comment:
"the pmd_trans_huge and pmd_trans_splitting must remain set at all times
on the pmd until the split is complete for this pmd"
I've only been testing this for arm64 (where I'm yet to see a problem),
but we use the generic pmdp_invalidate implementation from
mm/pgtable-generic.c there. On arm64, pmd_trans_huge will return true
after pmd_mknotpresent. On arm, it does look to be buggy, since it nukes
the entire entry... Steve?
pmd_mknotpresent on arm looks inconsistent with the other
architectures and can be changed.
Having had a look at the usage, I can't see it causing an immediate
problem (that needs to be addressed by an emergency patch).
We don't have a notion of splitting pmds (so there is no splitting
information to lose), and the only usage I could see of
pmd_mknotpresent was:
pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
In mm/huge_memory.c, around line 3588.
So we invalidate the entry (which puts down a faulting entry from
pmd_mknotpresent and invalidates tlb), then immediately put down a
table entry with pmd_populate.
I have run a 32-bit ARM test kernel and exacerbated THP splits (that's
what took me time), and I didn't notice any problems with 4.5-rc5.
If I read code correctly, your pmd_mknotpresent() makes the pmd
pmd_none(), right? If yes, it's a problem.
It introduces race I've described here:
https://marc.info/?l=linux-mm&m=144723658100512&w=4
Basically, if zap_pmd_range() would see pmd_none() between
pmdp_mknotpresent() and pmd_populate(), we're screwed.
The race window is small, but it's there.
Ahhhh, okay, thank you Kirill.
I agree, I'll get a patch out.
Cheers,
--
Steve
From: Martin Schwidefsky <hidden> Date: 2016-02-24 08:22:23
On Tue, 23 Feb 2016 19:19:07 +0100
Gerald Schaefer [off-list ref] wrote:
On Tue, 23 Feb 2016 13:32:21 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Fri, Feb 12, 2016 at 06:16:40PM +0100, Gerald Schaefer wrote:
quoted
On Fri, 12 Feb 2016 16:57:27 +0100
Christian Borntraeger [off-list ref] wrote:
quoted
quoted
I'm also confused by pmd_none() is equal to !pmd_present() on s390. Hm?
Don't know, Gerald or Martin?
The implementation frequently changes depending on how many new bits Martin
needs to squeeze out :-)
We don't have a _PAGE_PRESENT bit for pmds, so pmd_present() just checks if the
entry is not empty. pmd_none() of course does the opposite, it checks if it is
empty.
I still worry about pmd_present(). It looks wrong to me. I wounder if
patch below makes a difference.
The theory is that the splitting bit effetely masked bogus pmd_present():
we had pmd_trans_splitting() in all code path and that prevented mm from
touching the pmd. Once pmd_trans_splitting() has gone, mm proceed with the
pmd where it shouldn't and here's a boom.
Well, I don't think pmd_present() == true is bogus for a trans_huge pmd under
splitting, after all there is a page behind the the pmd. Also, if it was
bogus, and it would need to be false, why should it be marked !pmd_present()
only at the pmdp_invalidate() step before the pmd_populate()? It clearly
is pmd_present() before that, on all architectures, and if there was any
problem/race with that, setting it to !pmd_present() at this stage would
only (marginally) reduce the race window.
BTW, PowerPC and Sparc seem to do the same thing in pmdp_invalidate(),
i.e. they do not set pmd_present() == false, only mark it so that it would
not generate a new TLB entry, just like on s390. After all, the function
is called pmdp_invalidate(), and I think the comment in mm/huge_memory.c
before that call is just a little ambiguous in its wording. When it says
"mark the pmd notpresent" it probably means "mark it so that it will not
generate a new TLB entry", which is also what the comment is really about:
prevent huge and small entries in the TLB for the same page at the same
time.
If I am not mistaken this is true for x86 as well. The generic implementation
for pmdp_invalidate sets a new pmd that has been modified with
pmd_mknotpresent. For x86 this function removes the _PAGE_PRESENT and
_PAGE_PROTNONE bits from the entry. The _PAGE_PSE bit stays set and that
makes pmd_present return true.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: Kirill A. Shutemov <hidden> Date: 2016-02-24 09:14:03
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
Could you check this?
From: Will Deacon <hidden> Date: 2016-02-23 20:22:28
On Tue, Feb 23, 2016 at 10:33:45PM +0300, Kirill A. Shutemov wrote:
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
FWIW, arm64 ignores the address parameter for set_pmd_at, so this would
only result in the TLBI nuking the wrong entries, which is going to be
tricky to observe in practice given that we install a table entry
immediately afterwards that maps the same pages. If s390 does more here
(I see some magic asm using the address), that could be the answer...
Will
From: Christian Borntraeger <hidden> Date: 2016-02-24 10:16:49
On 02/23/2016 09:22 PM, Will Deacon wrote:
On Tue, Feb 23, 2016 at 10:33:45PM +0300, Kirill A. Shutemov wrote:
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
FWIW, arm64 ignores the address parameter for set_pmd_at, so this would
only result in the TLBI nuking the wrong entries, which is going to be
tricky to observe in practice given that we install a table entry
immediately afterwards that maps the same pages. If s390 does more here
(I see some magic asm using the address), that could be the answer...
This patch does not change the address for set_pmd_at, it does that for the
pmdp_invalidate here (by keeping haddr at the start of the pmd)
---> pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
Without that fix we would clearly have stale tlb entries, no?
From: Will Deacon <hidden> Date: 2016-02-24 10:41:38
On Wed, Feb 24, 2016 at 11:16:34AM +0100, Christian Borntraeger wrote:
On 02/23/2016 09:22 PM, Will Deacon wrote:
quoted
On Tue, Feb 23, 2016 at 10:33:45PM +0300, Kirill A. Shutemov wrote:
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
FWIW, arm64 ignores the address parameter for set_pmd_at, so this would
only result in the TLBI nuking the wrong entries, which is going to be
tricky to observe in practice given that we install a table entry
immediately afterwards that maps the same pages. If s390 does more here
(I see some magic asm using the address), that could be the answer...
This patch does not change the address for set_pmd_at, it does that for the
pmdp_invalidate here (by keeping haddr at the start of the pmd)
---> pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
On arm64, pmdp_invalidate looks like:
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
pmd_t entry = *pmdp;
set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
flush_pmd_tlb_range(vma, address, address + hpage_pmd_size);
}
so that's the set_pmd_at call I was referring to.
On s390, that address ends up in __pmdp_idte[_local], but I don't know
what .insn rrf,0xb98e0000,%2,%3,0,{0,1} do ;)
Without that fix we would clearly have stale tlb entries, no?
Yes, but AFAIU the sequence on arm64 is:
1. trans huge mapping (block mapping in arm64 speak)
2. faulting entry (pmd_mknotpresent)
3. tlb invalidation
4. table entry mapping the same pages as (1).
so if the microarchitecture we're on can tolerate a mixture of block
mappings and page mappings mapping the same VA to the same PA, then the
lack of TLB maintenance would go unnoticed. There are certainly systems
where that could cause an issue, but I believe the one I've been testing
on would be ok.
Will
From: Christian Borntraeger <hidden> Date: 2016-02-24 10:52:05
On 02/24/2016 11:41 AM, Will Deacon wrote:
On Wed, Feb 24, 2016 at 11:16:34AM +0100, Christian Borntraeger wrote:
quoted
On 02/23/2016 09:22 PM, Will Deacon wrote:
quoted
On Tue, Feb 23, 2016 at 10:33:45PM +0300, Kirill A. Shutemov wrote:
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
FWIW, arm64 ignores the address parameter for set_pmd_at, so this would
only result in the TLBI nuking the wrong entries, which is going to be
tricky to observe in practice given that we install a table entry
immediately afterwards that maps the same pages. If s390 does more here
(I see some magic asm using the address), that could be the answer...
This patch does not change the address for set_pmd_at, it does that for the
pmdp_invalidate here (by keeping haddr at the start of the pmd)
---> pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
On arm64, pmdp_invalidate looks like:
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
pmd_t entry = *pmdp;
set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
flush_pmd_tlb_range(vma, address, address + hpage_pmd_size);
}
so that's the set_pmd_at call I was referring to.
On s390, that address ends up in __pmdp_idte[_local], but I don't know
what .insn rrf,0xb98e0000,%2,%3,0,{0,1} do ;)
It does invalidation of the pmd entry and tlb clearing for this entry.
quoted
Without that fix we would clearly have stale tlb entries, no?
Yes, but AFAIU the sequence on arm64 is:
1. trans huge mapping (block mapping in arm64 speak)
2. faulting entry (pmd_mknotpresent)
3. tlb invalidation
4. table entry mapping the same pages as (1).
so if the microarchitecture we're on can tolerate a mixture of block
mappings and page mappings mapping the same VA to the same PA, then the
lack of TLB maintenance would go unnoticed. There are certainly systems
where that could cause an issue, but I believe the one I've been testing
on would be ok.
So in essence you say it does not matter that you flush the wrong range in
flush_pmd_tlb_range as long as it will be flushed later on when the pages
really go away. Yes, then it really might be ok for arm64.
From: Will Deacon <hidden> Date: 2016-02-24 11:02:20
On Wed, Feb 24, 2016 at 11:51:47AM +0100, Christian Borntraeger wrote:
On 02/24/2016 11:41 AM, Will Deacon wrote:
quoted
On Wed, Feb 24, 2016 at 11:16:34AM +0100, Christian Borntraeger wrote:
quoted
Without that fix we would clearly have stale tlb entries, no?
Yes, but AFAIU the sequence on arm64 is:
1. trans huge mapping (block mapping in arm64 speak)
2. faulting entry (pmd_mknotpresent)
3. tlb invalidation
4. table entry mapping the same pages as (1).
so if the microarchitecture we're on can tolerate a mixture of block
mappings and page mappings mapping the same VA to the same PA, then the
lack of TLB maintenance would go unnoticed. There are certainly systems
where that could cause an issue, but I believe the one I've been testing
on would be ok.
So in essence you say it does not matter that you flush the wrong range in
flush_pmd_tlb_range as long as it will be flushed later on when the pages
really go away. Yes, then it really might be ok for arm64.
Indeed, although that's a property of the microarchitecture I'm using
rather than an architectural guarantee so the code should certainly be
fixed!
Will
On Wed, Feb 24, 2016 at 11:16:34AM +0100, Christian Borntraeger wrote:
quoted
On 02/23/2016 09:22 PM, Will Deacon wrote:
quoted
On Tue, Feb 23, 2016 at 10:33:45PM +0300, Kirill A. Shutemov wrote:
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
FWIW, arm64 ignores the address parameter for set_pmd_at, so this would
only result in the TLBI nuking the wrong entries, which is going to be
tricky to observe in practice given that we install a table entry
immediately afterwards that maps the same pages. If s390 does more here
(I see some magic asm using the address), that could be the answer...
This patch does not change the address for set_pmd_at, it does that for the
pmdp_invalidate here (by keeping haddr at the start of the pmd)
---> pmdp_invalidate(vma, haddr, pmd);
pmd_populate(mm, pmd, pgtable);
On arm64, pmdp_invalidate looks like:
void pmdp_invalidate(struct vm_area_struct *vma, unsigned long address,
pmd_t *pmdp)
{
pmd_t entry = *pmdp;
set_pmd_at(vma->vm_mm, address, pmdp, pmd_mknotpresent(entry));
flush_pmd_tlb_range(vma, address, address + hpage_pmd_size);
}
so that's the set_pmd_at call I was referring to.
On s390, that address ends up in __pmdp_idte[_local], but I don't know
what .insn rrf,0xb98e0000,%2,%3,0,{0,1} do ;)
It does invalidation of the pmd entry and tlb clearing for this entry.
quoted
quoted
Without that fix we would clearly have stale tlb entries, no?
Yes, but AFAIU the sequence on arm64 is:
1. trans huge mapping (block mapping in arm64 speak)
2. faulting entry (pmd_mknotpresent)
3. tlb invalidation
4. table entry mapping the same pages as (1).
so if the microarchitecture we're on can tolerate a mixture of block
mappings and page mappings mapping the same VA to the same PA, then the
lack of TLB maintenance would go unnoticed. There are certainly systems
where that could cause an issue, but I believe the one I've been testing
on would be ok.
So in essence you say it does not matter that you flush the wrong range in
flush_pmd_tlb_range as long as it will be flushed later on when the pages
really go away. Yes, then it really might be ok for arm64.
This is more or less same for ppc64 too. With ppc64 the actual flush
happened in pmdp_huge_split_prepare() and pmdp_invalidate() is mostly a
no-op w.r.t thp split in our case.
-aneesh
From: Martin Schwidefsky <hidden> Date: 2016-02-24 08:39:24
On Tue, 23 Feb 2016 22:33:45 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted hunk
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
Could you check this?
Test is running and it looks good so far. For the final assessment I defer
to Gerald and Sebastian.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
From: Sebastian Ott <hidden> Date: 2016-02-24 12:12:07
On Wed, 24 Feb 2016, Martin Schwidefsky wrote:
On Tue, 23 Feb 2016 22:33:45 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
quoted
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
Could you check this?
On Tue, 23 Feb 2016 22:33:45 +0300
"Kirill A. Shutemov" [off-list ref] wrote:
On Tue, Feb 23, 2016 at 07:19:07PM +0100, Gerald Schaefer wrote:
quoted
I'll check with Martin, maybe it is actually trivial, then we can
do a quick test it to rule that one out.
Oh. I found a bug in __split_huge_pmd_locked(). Although, not sure if it's
_the_ bug.
pmdp_invalidate() is called for the wrong address :-/
I guess that can be destructive on the architecture, right?
Thanks, that's it! We can no longer reproduce the crashes and calling
pmdp_invalidate() with a wrong address also perfectly explains the
memory corruption that I found in several dumps: 0x020 was ORed into
pte entries, which didn't make sense, and caused the list corruption
for example. 0x020 it is the invalid bit for pmd entries on s390 and
thus can be explained by this bug when a pte table lies before a pmd
table in memory.