* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
@ 2016-03-09 5:02 Ganapatrao Kulkarni
2016-03-09 10:06 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Ganapatrao Kulkarni @ 2016-03-09 5:02 UTC (permalink / raw)
To: linux-arm-kernel
Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
access and dirty pte bits") introduced support for handling hardware
updates of the access flag and dirty status.
ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
however by design it suppose to set PTE_DIRTY
only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
test and set accordingly.
This patch fixes BUG,
kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
Internal error: Oops - BUG: 0 [#1] SMP
on thunderx numa board, when ARM64_HW_AFDBM and NUMA_BALANCING are enabled.
note: this patch is not tested on platform which supports AFDBM.
Signed-off-by: Ganapatrao Kulkarni <gkulkarni@caviumnetworks.com>
---
arch/arm64/include/asm/pgtable.h | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index f506086..d396892 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -613,20 +613,24 @@ static inline pmd_t pmdp_get_and_clear(struct mm_struct *mm,
#define __HAVE_ARCH_PTEP_SET_WRPROTECT
static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long address, pte_t *ptep)
{
- pteval_t pteval;
+ pteval_t pteval, pteval2;
unsigned long tmp;
asm volatile("// ptep_set_wrprotect\n"
- " prfm pstl1strm, %2\n"
- "1: ldxr %0, %2\n"
- " tst %0, %4 // check for hw dirty (!PTE_RDONLY)\n"
- " csel %1, %3, xzr, eq // set PTE_DIRTY|PTE_RDONLY if dirty\n"
- " orr %0, %0, %1 // if !dirty, PTE_RDONLY is already set\n"
- " and %0, %0, %5 // clear PTE_WRITE/PTE_DBM\n"
- " stxr %w1, %0, %2\n"
+ " prfm pstl1strm, %3\n"
+ "1: ldxr %0, %3\n"
+ " and %2, %0, %4 //extract bits PTE_WRITE and PTE_RDONLY\n"
+ " cmp %2, %5 // compare wth PTE_WRITE\n"
+ " b.ne 2f\n"
+ " orr %0, %0, %8 // Set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)\n"
+ "2: tst %0, %6 // check for !PTE_RDONLY\n"
+ " csel %1, %6, xzr, eq // select PTE_RDONLY if !PTE_RDONLY\n"
+ " orr %0, %0, %1 // set PTE_RDONLY if !PTE_RDONLY\n"
+ " and %0, %0, %7 // clear PTE_WRITE/PTE_DBM\n"
+ " stxr %w1, %0, %3\n"
" cbnz %w1, 1b\n"
- : "=&r" (pteval), "=&r" (tmp), "+Q" (pte_val(*ptep))
- : "r" (PTE_DIRTY|PTE_RDONLY), "L" (PTE_RDONLY), "L" (~PTE_WRITE)
+ : "=&r" (pteval), "=&r" (tmp), "=&r" (pteval2), "+Q" (pte_val(*ptep))
+ : "r" (PTE_WRITE|PTE_RDONLY), "r" (PTE_WRITE), "r" (PTE_RDONLY), "L" (~PTE_WRITE), "L" (PTE_DIRTY)
: "cc");
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-09 5:02 [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY) Ganapatrao Kulkarni
@ 2016-03-09 10:06 ` Catalin Marinas
2016-03-09 11:47 ` Ganapatrao Kulkarni
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2016-03-09 10:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 09, 2016 at 10:32:48AM +0530, Ganapatrao Kulkarni wrote:
> Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
> access and dirty pte bits") introduced support for handling hardware
> updates of the access flag and dirty status.
>
> ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
> however by design it suppose to set PTE_DIRTY
> only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
> test and set accordingly.
The reasoning behind the original code is that if !PTE_RDONLY, you have
no way to tell whether the page was written or not since it is already
writable, independent of the DBM. So by clearing the DBM bit (making the
page read-only), we need to ensure that a potential dirty state is
transferred to the software PTE_DIRTY bit.
By checking PTE_DBM && !PTE_RDONLY, you kind of imply that you can have
a page with !PTE_DBM && !PTE_RDONLY. Given that PTE_DBM is actually
PTE_WRITE, PTE_RDONLY must always be set when !PTE_DBM. The bug may be
elsewhere not setting these bits correctly.
> This patch fixes BUG,
> kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
> Internal error: Oops - BUG: 0 [#1] SMP
Which bug is this? It's a PageWriteback() check in the for-next/core
branch. What kernel version are you using?
BTW, in 4.5-rc2 we pushed commit ac15bd63bbb2 ("arm64: Honour !PTE_WRITE
in set_pte_at() for kernel mappings"), though not sure that's what you
are hitting.
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-09 10:06 ` Catalin Marinas
@ 2016-03-09 11:47 ` Ganapatrao Kulkarni
2016-03-09 16:03 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Ganapatrao Kulkarni @ 2016-03-09 11:47 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 9, 2016 at 3:36 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Mar 09, 2016 at 10:32:48AM +0530, Ganapatrao Kulkarni wrote:
>> Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
>> access and dirty pte bits") introduced support for handling hardware
>> updates of the access flag and dirty status.
>>
>> ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
>> however by design it suppose to set PTE_DIRTY
>> only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
>> test and set accordingly.
>
> The reasoning behind the original code is that if !PTE_RDONLY, you have
> no way to tell whether the page was written or not since it is already
> writable, independent of the DBM. So by clearing the DBM bit (making the
> page read-only), we need to ensure that a potential dirty state is
> transferred to the software PTE_DIRTY bit.
>
> By checking PTE_DBM && !PTE_RDONLY, you kind of imply that you can have
> a page with !PTE_DBM && !PTE_RDONLY. Given that PTE_DBM is actually
> PTE_WRITE, PTE_RDONLY must always be set when !PTE_DBM. The bug may be
> elsewhere not setting these bits correctly.
but i do see this macro,
#define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
i dont see this issue, if i comment out arm64 implementation of
ptep_set_wrprotect()
>
>> This patch fixes BUG,
>> kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
>> Internal error: Oops - BUG: 0 [#1] SMP
>
> Which bug is this? It's a PageWriteback() check in the for-next/core
> branch. What kernel version are you using?
i am using 4.4.0
>
> BTW, in 4.5-rc2 we pushed commit ac15bd63bbb2 ("arm64: Honour !PTE_WRITE
> in set_pte_at() for kernel mappings"), though not sure that's what you
> are hitting.
i have tried this patch, but issue still exist. crash log below
root at ubuntu:/home/ganapat/test# [ 733.853009] kernel BUG at
fs/ext4/inode.c:2394!
[ 733.857533] Internal error: Oops - BUG: 0 [#1] SMP
[ 733.862313] Modules linked in: ipt_MASQUERADE
nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT
nf_reject_ipv4 xt
_CHECKSUM iptable_mangle xt_tcpudp bridge stp llc ip6table_filter
ip6_tables iptable_filter ip_tables ebtable_nat ebtables x_tables
ghash_ce sha2_ce sha1_ce joydev input_leds ax88179_178a us
bnet gpio_keys thunderx_edac_lmc thunderx_edac_ccpi edac_core
i2c_octeon nicvf shpchp uio_pdrv_genirq uio rtc_efi nls_iso8859_1
nicpf thunder_bgx hid_generic usbhid hid ahci libahci mdio_oct
eon
[ 733.912389] CPU: 38 PID: 573 Comm: kworker/u192:1 Not tainted 4.4.0+ #15
[ 733.919076] Hardware name: www.cavium.com ThunderX Unknown/ThunderX
Unknown, BIOS 0.3 Jan 21 2016
[ 733.927940] Workqueue: writeback wb_workfn (flush-8:0)
[ 733.933071] task: ffff8000f9548000 ti: ffff8000f9550000 task.ti:
ffff8000f9550000
[ 733.940543] PC is at mpage_prepare_extent_to_map+0x25c/0x264
[ 733.946190] LR is at mpage_prepare_extent_to_map+0x134/0x264
[ 733.951837] pc : [<ffff8000002c2d98>] lr : [<ffff8000002c2c70>]
pstate: 60400145
[ 733.959218] sp : ffff8000f9553830
[ 733.962520] x29: ffff8000f9553830 x28: 0000000000000000
[ 733.967824] x27: ffff8000f9553a00 x26: ffff8000f95538b8
[ 733.973128] x25: 0000000000000000 x24: ffff801f6ae88a38
[ 733.978431] x23: ffff8000f95538c0 x22: ffffffffffffffff
[ 733.983734] x21: 0000000000004800 x20: ffff8000f95538b8
[ 733.989037] x19: ffff7c03c001eac0 x18: 0000ffffcac831f0
[ 733.994340] x17: 0000ffffaf7a9040 x16: ffff8000000c0d68
[ 733.999643] x15: 000000002fd55332 x14: 0000000000000006
[ 734.004946] x13: ffff811f65be2da0 x12: 0000000000000000
[ 734.010250] x11: 0000000000000040 x10: 0000000000000000
[ 734.015553] x9 : 0000000000000220 x8 : 0000000000000100
[ 734.020856] x7 : 0000000000001000 x6 : 0000000000001000
[ 734.026158] x5 : 0000000000000000 x4 : 0000000000000001
[ 734.031461] x3 : 000000000000014a x2 : 5fffe0000001023d
[ 734.036764] x1 : ffff7c03c001eac0 x0 : 5fffe0000001023d
[ 734.549044] Call trace:
[ 734.551481] [<ffff8000002c2d98>] mpage_prepare_extent_to_map+0x25c/0x264
[ 734.558171] [<ffff8000002c6c48>] ext4_writepages+0x2f8/0x9e4
[ 734.563820] [<ffff8000001c44ec>] do_writepages+0x40/0x6c
[ 734.569120] [<ffff800000264ecc>] __writeback_single_inode+0x5c/0x2dc
[ 734.575460] [<ffff800000265644>] writeback_sb_inodes+0x20c/0x3cc
[ 734.581453] [<ffff8000002658a8>] __writeback_inodes_wb+0xa4/0xe8
[ 734.587447] [<ffff800000265ad4>] wb_writeback+0x1e8/0x278
[ 734.592833] [<ffff800000266348>] wb_workfn+0x2a4/0x394
[ 734.597960] [<ffff8000000d7470>] process_one_work+0x16c/0x390
[ 734.603693] [<ffff8000000d77d0>] worker_thread+0x13c/0x42c
[ 734.609168] [<ffff8000000dddfc>] kthread+0xe8/0xfc
[ 734.613948] [<ffff800000085c10>] ret_from_fork+0x10/0x40
[ 734.619248] Code: 17ffffdc aa1303e0 97fbd172 17ffffb8 (d4210000)
[ 734.640344] ---[ end trace 0b4626b567403558 ]---
>
> --
> Catalin
thanks
Ganapat
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-09 11:47 ` Ganapatrao Kulkarni
@ 2016-03-09 16:03 ` Catalin Marinas
2016-03-09 17:43 ` Ganapatrao Kulkarni
0 siblings, 1 reply; 7+ messages in thread
From: Catalin Marinas @ 2016-03-09 16:03 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 09, 2016 at 05:17:39PM +0530, Ganapatrao Kulkarni wrote:
> On Wed, Mar 9, 2016 at 3:36 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Wed, Mar 09, 2016 at 10:32:48AM +0530, Ganapatrao Kulkarni wrote:
> >> Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
> >> access and dirty pte bits") introduced support for handling hardware
> >> updates of the access flag and dirty status.
> >>
> >> ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
> >> however by design it suppose to set PTE_DIRTY
> >> only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
> >> test and set accordingly.
> >
> > The reasoning behind the original code is that if !PTE_RDONLY, you have
> > no way to tell whether the page was written or not since it is already
> > writable, independent of the DBM. So by clearing the DBM bit (making the
> > page read-only), we need to ensure that a potential dirty state is
> > transferred to the software PTE_DIRTY bit.
> >
> > By checking PTE_DBM && !PTE_RDONLY, you kind of imply that you can have
> > a page with !PTE_DBM && !PTE_RDONLY. Given that PTE_DBM is actually
> > PTE_WRITE, PTE_RDONLY must always be set when !PTE_DBM. The bug may be
> > elsewhere not setting these bits correctly.
>
> but i do see this macro,
> #define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
This was added in commit b847415ce96e ("arm64: Fix the pte_hw_dirty()
check when AF/DBM is enabled") for the pte_modify() case which is not
called on the actual PTE but a local variable. A pte passed to this
function as !PTE_DBM && !PTE_RDONLY should not be assumed dirty since
PTE_RDONLY will be set later by set_pte_at() when the actual page table
write occurs.
ptep_set_wrprotect() is run directly on the actual PTE, so here a
!PTE_RDONLY only means potentially dirty, independent of the PTE_DBM
bit. I consider the additional PTE_DBM check superfluous in this case
but we need to understand when we would actually get a pte with both
PTE_DBM and PTE_RDONLY cleared.
The only way I see this happening is if the pte doesn't have PTE_VALID
set, IOW it probably has PTE_PROT_NONE set which is used by the NUMA
balancing. So calling set_pte_at() on a !PTE_VALID && !PTE_DBM pte does
not currently set PTE_RDONLY and ptep_set_wrprotect() wrongly assumes it
is dirty.
> i dont see this issue, if i comment out arm64 implementation of
> ptep_set_wrprotect()
Because the default implementation discards any existing hw dirty
information by clearing the PTE_DBM bit and setting PTE_RDONLY via the
set_pte_at (of course, apart from the atomicity issues).
> >> This patch fixes BUG,
> >> kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
> >> Internal error: Oops - BUG: 0 [#1] SMP
> >
> > Which bug is this? It's a PageWriteback() check in the for-next/core
> > branch. What kernel version are you using?
>
> i am using 4.4.0
I guess with additional NUMA patches since it only fails when you enable
the NUMA_BALANCING configuration.
> > BTW, in 4.5-rc2 we pushed commit ac15bd63bbb2 ("arm64: Honour !PTE_WRITE
> > in set_pte_at() for kernel mappings"), though not sure that's what you
> > are hitting.
>
> i have tried this patch, but issue still exist. crash log below
>
> root at ubuntu:/home/ganapat/test# [ 733.853009] kernel BUG at
> fs/ext4/inode.c:2394!
Is this the BUG_ON in page_buffers(!PagePrivate(page))? I can see in the
code above this that wrongly marking a page as dirty could have some
side effects.
Can you give this patch a try, on top of commit ac15bd63bbb2?
-------------8<----------------------
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 7c73b365fcfa..b409a983f870 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -201,7 +201,7 @@ extern void __sync_icache_dcache(pte_t pteval, unsigned long addr);
static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte)
{
- if (pte_valid(pte)) {
+ if (pte_present(pte)) {
if (pte_sw_dirty(pte) && pte_write(pte))
pte_val(pte) &= ~PTE_RDONLY;
else
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-09 16:03 ` Catalin Marinas
@ 2016-03-09 17:43 ` Ganapatrao Kulkarni
2016-03-10 3:04 ` Ganapatrao Kulkarni
0 siblings, 1 reply; 7+ messages in thread
From: Ganapatrao Kulkarni @ 2016-03-09 17:43 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 9, 2016 at 9:33 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Mar 09, 2016 at 05:17:39PM +0530, Ganapatrao Kulkarni wrote:
>> On Wed, Mar 9, 2016 at 3:36 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > On Wed, Mar 09, 2016 at 10:32:48AM +0530, Ganapatrao Kulkarni wrote:
>> >> Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
>> >> access and dirty pte bits") introduced support for handling hardware
>> >> updates of the access flag and dirty status.
>> >>
>> >> ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
>> >> however by design it suppose to set PTE_DIRTY
>> >> only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
>> >> test and set accordingly.
>> >
>> > The reasoning behind the original code is that if !PTE_RDONLY, you have
>> > no way to tell whether the page was written or not since it is already
>> > writable, independent of the DBM. So by clearing the DBM bit (making the
>> > page read-only), we need to ensure that a potential dirty state is
>> > transferred to the software PTE_DIRTY bit.
>> >
>> > By checking PTE_DBM && !PTE_RDONLY, you kind of imply that you can have
>> > a page with !PTE_DBM && !PTE_RDONLY. Given that PTE_DBM is actually
>> > PTE_WRITE, PTE_RDONLY must always be set when !PTE_DBM. The bug may be
>> > elsewhere not setting these bits correctly.
>>
>> but i do see this macro,
>> #define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
>
> This was added in commit b847415ce96e ("arm64: Fix the pte_hw_dirty()
> check when AF/DBM is enabled") for the pte_modify() case which is not
> called on the actual PTE but a local variable. A pte passed to this
> function as !PTE_DBM && !PTE_RDONLY should not be assumed dirty since
> PTE_RDONLY will be set later by set_pte_at() when the actual page table
> write occurs.
>
> ptep_set_wrprotect() is run directly on the actual PTE, so here a
> !PTE_RDONLY only means potentially dirty, independent of the PTE_DBM
> bit. I consider the additional PTE_DBM check superfluous in this case
> but we need to understand when we would actually get a pte with both
> PTE_DBM and PTE_RDONLY cleared.
>
> The only way I see this happening is if the pte doesn't have PTE_VALID
> set, IOW it probably has PTE_PROT_NONE set which is used by the NUMA
> balancing. So calling set_pte_at() on a !PTE_VALID && !PTE_DBM pte does
> not currently set PTE_RDONLY and ptep_set_wrprotect() wrongly assumes it
> is dirty.
>
>> i dont see this issue, if i comment out arm64 implementation of
>> ptep_set_wrprotect()
>
> Because the default implementation discards any existing hw dirty
> information by clearing the PTE_DBM bit and setting PTE_RDONLY via the
> set_pte_at (of course, apart from the atomicity issues).
>
>> >> This patch fixes BUG,
>> >> kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
>> >> Internal error: Oops - BUG: 0 [#1] SMP
>> >
>> > Which bug is this? It's a PageWriteback() check in the for-next/core
>> > branch. What kernel version are you using?
>>
>> i am using 4.4.0
>
> I guess with additional NUMA patches since it only fails when you enable
> the NUMA_BALANCING configuration.
>
>> > BTW, in 4.5-rc2 we pushed commit ac15bd63bbb2 ("arm64: Honour !PTE_WRITE
>> > in set_pte_at() for kernel mappings"), though not sure that's what you
>> > are hitting.
>>
>> i have tried this patch, but issue still exist. crash log below
>>
>> root at ubuntu:/home/ganapat/test# [ 733.853009] kernel BUG at
>> fs/ext4/inode.c:2394!
>
> Is this the BUG_ON in page_buffers(!PagePrivate(page))? I can see in the
> code above this that wrongly marking a page as dirty could have some
> side effects.
>
> Can you give this patch a try, on top of commit ac15bd63bbb2?
thanks, this fixes the issue, i have tried making pte_valid same as pte_present
however, i have overlooked that set_pte_at is using pte_valid_user(in 4.4)
>
> -------------8<----------------------
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 7c73b365fcfa..b409a983f870 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -201,7 +201,7 @@ extern void __sync_icache_dcache(pte_t pteval, unsigned long addr);
> static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pte)
> {
> - if (pte_valid(pte)) {
> + if (pte_present(pte)) {
> if (pte_sw_dirty(pte) && pte_write(pte))
> pte_val(pte) &= ~PTE_RDONLY;
> else
Ganapat
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-09 17:43 ` Ganapatrao Kulkarni
@ 2016-03-10 3:04 ` Ganapatrao Kulkarni
2016-03-10 18:39 ` Catalin Marinas
0 siblings, 1 reply; 7+ messages in thread
From: Ganapatrao Kulkarni @ 2016-03-10 3:04 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Mar 9, 2016 at 11:13 PM, Ganapatrao Kulkarni
<gpkulkarni@gmail.com> wrote:
> On Wed, Mar 9, 2016 at 9:33 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> On Wed, Mar 09, 2016 at 05:17:39PM +0530, Ganapatrao Kulkarni wrote:
>>> On Wed, Mar 9, 2016 at 3:36 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>> > On Wed, Mar 09, 2016 at 10:32:48AM +0530, Ganapatrao Kulkarni wrote:
>>> >> Commit 2f4b829c625e ("arm64: Add support for hardware updates of the
>>> >> access and dirty pte bits") introduced support for handling hardware
>>> >> updates of the access flag and dirty status.
>>> >>
>>> >> ptep_set_wrprotect is setting PTR_DIRTY if !PTE_RDONLY,
>>> >> however by design it suppose to set PTE_DIRTY
>>> >> only if (PTE_DBM && !PTE_RDONLY). This patch addes code to
>>> >> test and set accordingly.
>>> >
>>> > The reasoning behind the original code is that if !PTE_RDONLY, you have
>>> > no way to tell whether the page was written or not since it is already
>>> > writable, independent of the DBM. So by clearing the DBM bit (making the
>>> > page read-only), we need to ensure that a potential dirty state is
>>> > transferred to the software PTE_DIRTY bit.
>>> >
>>> > By checking PTE_DBM && !PTE_RDONLY, you kind of imply that you can have
>>> > a page with !PTE_DBM && !PTE_RDONLY. Given that PTE_DBM is actually
>>> > PTE_WRITE, PTE_RDONLY must always be set when !PTE_DBM. The bug may be
>>> > elsewhere not setting these bits correctly.
>>>
>>> but i do see this macro,
>>> #define pte_hw_dirty(pte) (pte_write(pte) && !(pte_val(pte) & PTE_RDONLY))
>>
>> This was added in commit b847415ce96e ("arm64: Fix the pte_hw_dirty()
>> check when AF/DBM is enabled") for the pte_modify() case which is not
>> called on the actual PTE but a local variable. A pte passed to this
>> function as !PTE_DBM && !PTE_RDONLY should not be assumed dirty since
>> PTE_RDONLY will be set later by set_pte_at() when the actual page table
>> write occurs.
>>
>> ptep_set_wrprotect() is run directly on the actual PTE, so here a
>> !PTE_RDONLY only means potentially dirty, independent of the PTE_DBM
>> bit. I consider the additional PTE_DBM check superfluous in this case
>> but we need to understand when we would actually get a pte with both
>> PTE_DBM and PTE_RDONLY cleared.
>>
>> The only way I see this happening is if the pte doesn't have PTE_VALID
>> set, IOW it probably has PTE_PROT_NONE set which is used by the NUMA
>> balancing. So calling set_pte_at() on a !PTE_VALID && !PTE_DBM pte does
>> not currently set PTE_RDONLY and ptep_set_wrprotect() wrongly assumes it
>> is dirty.
>>
>>> i dont see this issue, if i comment out arm64 implementation of
>>> ptep_set_wrprotect()
>>
>> Because the default implementation discards any existing hw dirty
>> information by clearing the PTE_DBM bit and setting PTE_RDONLY via the
>> set_pte_at (of course, apart from the atomicity issues).
>>
>>> >> This patch fixes BUG,
>>> >> kernel BUG at /build/linux-StrpB2/linux-4.4.0/fs/ext4/inode.c:2394!
>>> >> Internal error: Oops - BUG: 0 [#1] SMP
>>> >
>>> > Which bug is this? It's a PageWriteback() check in the for-next/core
>>> > branch. What kernel version are you using?
>>>
>>> i am using 4.4.0
>>
>> I guess with additional NUMA patches since it only fails when you enable
>> the NUMA_BALANCING configuration.
>>
>>> > BTW, in 4.5-rc2 we pushed commit ac15bd63bbb2 ("arm64: Honour !PTE_WRITE
>>> > in set_pte_at() for kernel mappings"), though not sure that's what you
>>> > are hitting.
>>>
>>> i have tried this patch, but issue still exist. crash log below
>>>
>>> root at ubuntu:/home/ganapat/test# [ 733.853009] kernel BUG at
>>> fs/ext4/inode.c:2394!
>>
>> Is this the BUG_ON in page_buffers(!PagePrivate(page))? I can see in the
>> code above this that wrongly marking a page as dirty could have some
>> side effects.
>>
>> Can you give this patch a try, on top of commit ac15bd63bbb2?
>
> thanks, this fixes the issue, i have tried making pte_valid same as pte_present
> however, i have overlooked that set_pte_at is using pte_valid_user(in 4.4)
>
>
>>
>> -------------8<----------------------
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 7c73b365fcfa..b409a983f870 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -201,7 +201,7 @@ extern void __sync_icache_dcache(pte_t pteval, unsigned long addr);
>> static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
>> pte_t *ptep, pte_t pte)
>> {
>> - if (pte_valid(pte)) {
>> + if (pte_present(pte)) {
>> if (pte_sw_dirty(pte) && pte_write(pte))
>> pte_val(pte) &= ~PTE_RDONLY;
>> else
>
this diff works for me.
Tested-by: Ganapatrao Kulkarni <gkulkarni@cavium.com>
> Ganapat
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY)
2016-03-10 3:04 ` Ganapatrao Kulkarni
@ 2016-03-10 18:39 ` Catalin Marinas
0 siblings, 0 replies; 7+ messages in thread
From: Catalin Marinas @ 2016-03-10 18:39 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 10, 2016 at 08:34:46AM +0530, Ganapatrao Kulkarni wrote:
> > On Wed, Mar 9, 2016 at 9:33 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> >> index 7c73b365fcfa..b409a983f870 100644
> >> --- a/arch/arm64/include/asm/pgtable.h
> >> +++ b/arch/arm64/include/asm/pgtable.h
> >> @@ -201,7 +201,7 @@ extern void __sync_icache_dcache(pte_t pteval, unsigned long addr);
> >> static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> >> pte_t *ptep, pte_t pte)
> >> {
> >> - if (pte_valid(pte)) {
> >> + if (pte_present(pte)) {
> >> if (pte_sw_dirty(pte) && pte_write(pte))
> >> pte_val(pte) &= ~PTE_RDONLY;
> >> else
>
> this diff works for me.
>
> Tested-by: Ganapatrao Kulkarni <gkulkarni@cavium.com>
Thanks. I'll push it out during the merging window and cc stable (though
it needs a slightly different workaround for 4.4 anyway).
--
Catalin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-03-10 18:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-09 5:02 [PATCH] arm64: Fix the ptep_set_wrprotect() to set PTE_DIRTY if (PTE_DBM && !PTE_RDONLY) Ganapatrao Kulkarni
2016-03-09 10:06 ` Catalin Marinas
2016-03-09 11:47 ` Ganapatrao Kulkarni
2016-03-09 16:03 ` Catalin Marinas
2016-03-09 17:43 ` Ganapatrao Kulkarni
2016-03-10 3:04 ` Ganapatrao Kulkarni
2016-03-10 18:39 ` Catalin Marinas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).