Hi Mel, On Wed, 6 Feb 2013 11:21:11 +0000 Mel Gorman <mgorman@xxxxxxx> wrote: > On Tue, Feb 05, 2013 at 10:12:05AM -0800, Martin Schwidefsky wrote: > > The s390 architecture is unique in respect to dirty page detection, > > it uses the change bit in the per-page storage key to track page > > modifications. All other architectures track dirty bits by means > > of page table entries. This property of s390 has caused numerous > > problems in the past, e.g. see git commit ef5d437f71afdf4a > > "mm: fix XFS oops due to dirty pages without buffers on s390". > > > > To avoid future issues in regard to per-page dirty bits convert > > s390 to a fault based software dirty bit detection mechanism. All > > user page table entries which are marked as clean will be hardware > > read-only, even if the pte is supposed to be writable. A write by > > the user process will trigger a protection fault which will cause > > the user pte to be marked as dirty and the hardware read-only bit > > is removed. > > > > With this change the dirty bit in the storage key is irrelevant > > for Linux as a host, but the storage key is still required for > > KVM guests. The effect is that page_test_and_clear_dirty and the > > related code can be removed. The referenced bit in the storage > > key is still used by the page_test_and_clear_young primitive to > > provide page age information. > > > > For page cache pages of mappings with mapping_cap_account_dirty > > there will not be any change in behavior as the dirty bit tracking > > already uses read-only ptes to control the amount of dirty pages. > > Only for swap cache pages and pages of mappings without > > mapping_cap_account_dirty there can be additional protection faults. > > To avoid an excessive number of additional faults the mk_pte > > primitive checks for PageDirty if the pgprot value allows for writes > > and pre-dirties the pte. That avoids all additional faults for > > tmpfs and shmem pages until these pages are added to the swap cache. > > > > Signed-off-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx> > > I have a few clarifications below just to make sure I'm reading it right > but I think it looks fine and the change to mm/rmap.c is welcome. It is welcome to me as well. That XFS problem really convinced me that this is the right way to go. > > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > > index 098adbb..2b3d3b6 100644 > > --- a/arch/s390/include/asm/pgtable.h > > +++ b/arch/s390/include/asm/pgtable.h > > @@ -29,6 +29,7 @@ > > #ifndef __ASSEMBLY__ > > #include <linux/sched.h> > > #include <linux/mm_types.h> > > +#include <linux/page-flags.h> > > #include <asm/bug.h> > > #include <asm/page.h> > > > > @@ -221,13 +222,15 @@ extern unsigned long MODULES_END; > > /* Software bits in the page table entry */ > > #define _PAGE_SWT 0x001 /* SW pte type bit t */ > > #define _PAGE_SWX 0x002 /* SW pte type bit x */ > > -#define _PAGE_SWC 0x004 /* SW pte changed bit (for KVM) */ > > -#define _PAGE_SWR 0x008 /* SW pte referenced bit (for KVM) */ > > -#define _PAGE_SPECIAL 0x010 /* SW associated with special page */ > > +#define _PAGE_SWC 0x004 /* SW pte changed bit */ > > +#define _PAGE_SWR 0x008 /* SW pte referenced bit */ > > +#define _PAGE_SWW 0x010 /* SW pte write bit */ > > +#define _PAGE_SPECIAL 0x020 /* SW associated with special page */ > > #define __HAVE_ARCH_PTE_SPECIAL > > > > /* Set of bits not changed in pte_modify */ > > -#define _PAGE_CHG_MASK (PAGE_MASK | _PAGE_SPECIAL | _PAGE_SWC | _PAGE_SWR) > > +#define _PAGE_CHG_MASK (PAGE_MASK | _PAGE_SPECIAL | _PAGE_CO | \ > > + _PAGE_SWC | _PAGE_SWR) > > > > If I'm reading it right, the _PAGE_CO is bit is what allows you to force > the hardware to trap even if the PTE says the page is writable. This is > what necessitates the shuffling of _PAGE_SPECIAL so you have a software > write bit and a hardware write bit. No, the _PAGE_CO bit is the change-bit-override. This allows the hardware to avoid to set the dirty bit in the storage key for a write access over a pte. It is a optimization, the code would work without _PAGE_CO as well. The basic idea is to introduce a software bit which indicates the software view of writable vs. non-writable (the _PAGE_SWW bit). The hardware _PAGE_RO is used to disallow writes while the pte is clean. > For existing distributions they might not be able to use this patch for > bugs like ""mm: fix XFS oops due to dirty pages without buffers on s390" > because this shuffling of bits will break KABIi but that's not your problem. I am not really sure if we should backport that change to the existing distributions. It is a non-trivial change. > > /* Six different types of pages. */ > > #define _PAGE_TYPE_EMPTY 0x400 > > @@ -321,6 +324,7 @@ extern unsigned long MODULES_END; > > > > /* Bits in the region table entry */ > > #define _REGION_ENTRY_ORIGIN ~0xfffUL/* region/segment table origin */ > > +#define _REGION_ENTRY_RO 0x200 /* region protection bit */ > > #define _REGION_ENTRY_INV 0x20 /* invalid region table entry */ > > #define _REGION_ENTRY_TYPE_MASK 0x0c /* region/segment table type mask */ > > #define _REGION_ENTRY_TYPE_R1 0x0c /* region first table type */ > > @@ -382,9 +386,10 @@ extern unsigned long MODULES_END; > > */ > > #define PAGE_NONE __pgprot(_PAGE_TYPE_NONE) > > #define PAGE_RO __pgprot(_PAGE_TYPE_RO) > > -#define PAGE_RW __pgprot(_PAGE_TYPE_RW) > > +#define PAGE_RW __pgprot(_PAGE_TYPE_RO | _PAGE_SWW) > > +#define PAGE_RWC __pgprot(_PAGE_TYPE_RW | _PAGE_SWW | _PAGE_SWC) > > > > -#define PAGE_KERNEL PAGE_RW > > +#define PAGE_KERNEL PAGE_RWC > > #define PAGE_COPY PAGE_RO > > > > /* > > And this combination of page bits looks consistent. The details are > heavily buried in the arch code so I hope however deals with this area > in the future spots the changelog. Of course, that guy is likely to be > you anyway :) Yep, I will be that guy. With the move of the complexity to handle s390 to the arch implementation even more than before. > > @@ -631,23 +636,23 @@ static inline pgste_t pgste_update_all(pte_t *ptep, pgste_t pgste) > > bits = skey & (_PAGE_CHANGED | _PAGE_REFERENCED); > > /* Clear page changed & referenced bit in the storage key */ > > if (bits & _PAGE_CHANGED) > > - page_set_storage_key(address, skey ^ bits, 1); > > + page_set_storage_key(address, skey ^ bits, 0); > > else if (bits) > > page_reset_referenced(address); > > /* Transfer page changed & referenced bit to guest bits in pgste */ > > pgste_val(pgste) |= bits << 48; /* RCP_GR_BIT & RCP_GC_BIT */ > > /* Get host changed & referenced bits from pgste */ > > bits |= (pgste_val(pgste) & (RCP_HR_BIT | RCP_HC_BIT)) >> 52; > > - /* Clear host bits in pgste. */ > > + /* Transfer page changed & referenced bit to kvm user bits */ > > + pgste_val(pgste) |= bits << 45; /* KVM_UR_BIT & KVM_UC_BIT */ > > + /* Clear relevant host bits in pgste. */ > > pgste_val(pgste) &= ~(RCP_HR_BIT | RCP_HC_BIT); > > pgste_val(pgste) &= ~(RCP_ACC_BITS | RCP_FP_BIT); > > /* Copy page access key and fetch protection bit to pgste */ > > pgste_val(pgste) |= > > (unsigned long) (skey & (_PAGE_ACC_BITS | _PAGE_FP_BIT)) << 56; > > - /* Transfer changed and referenced to kvm user bits */ > > - pgste_val(pgste) |= bits << 45; /* KVM_UR_BIT & KVM_UC_BIT */ > > - /* Transfer changed & referenced to pte sofware bits */ > > - pte_val(*ptep) |= bits << 1; /* _PAGE_SWR & _PAGE_SWC */ > > + /* Transfer referenced bit to pte */ > > + pte_val(*ptep) |= (bits & _PAGE_REFERENCED) << 1; > > #endif > > return pgste; > > > > @@ -660,20 +665,25 @@ static inline pgste_t pgste_update_young(pte_t *ptep, pgste_t pgste) > > > > if (!pte_present(*ptep)) > > return pgste; > > + /* Get referenced bit from storage key */ > > young = page_reset_referenced(pte_val(*ptep) & PAGE_MASK); > > - /* Transfer page referenced bit to pte software bit (host view) */ > > - if (young || (pgste_val(pgste) & RCP_HR_BIT)) > > + if (young) > > + pgste_val(pgste) |= RCP_GR_BIT; > > + /* Get host referenced bit from pgste */ > > + if (pgste_val(pgste) & RCP_HR_BIT) { > > + pgste_val(pgste) &= ~RCP_HR_BIT; > > + young = 1; > > + } > > + /* Transfer referenced bit to kvm user bits and pte */ > > + if (young) { > > + pgste_val(pgste) |= KVM_UR_BIT; > > pte_val(*ptep) |= _PAGE_SWR; > > - /* Clear host referenced bit in pgste. */ > > - pgste_val(pgste) &= ~RCP_HR_BIT; > > - /* Transfer page referenced bit to guest bit in pgste */ > > - pgste_val(pgste) |= (unsigned long) young << 50; /* set RCP_GR_BIT */ > > + } > > #endif > > return pgste; > > - > > } > > > > -static inline void pgste_set_pte(pte_t *ptep, pgste_t pgste, pte_t entry) > > +static inline void pgste_set_key(pte_t *ptep, pgste_t pgste, pte_t entry) > > { > > #ifdef CONFIG_PGSTE > > unsigned long address; > > @@ -687,10 +697,23 @@ static inline void pgste_set_pte(pte_t *ptep, pgste_t pgste, pte_t entry) > > /* Set page access key and fetch protection bit from pgste */ > > nkey |= (pgste_val(pgste) & (RCP_ACC_BITS | RCP_FP_BIT)) >> 56; > > if (okey != nkey) > > - page_set_storage_key(address, nkey, 1); > > + page_set_storage_key(address, nkey, 0); > > #endif > > } > > > > +static inline void pgste_set_pte(pte_t *ptep, pte_t entry) > > +{ > > + if (!MACHINE_HAS_ESOP && (pte_val(entry) & _PAGE_SWW)) { > > + /* > > + * Without enhanced suppression-on-protection force > > + * the dirty bit on for all writable ptes. > > + */ > > + pte_val(entry) |= _PAGE_SWC; > > + pte_val(entry) &= ~_PAGE_RO; > > + } > > + *ptep = entry; > > +} > > + > > /** > > * struct gmap_struct - guest address space > > * @mm: pointer to the parent mm_struct > > So establishing a writable PTE clears the hardware RO override as > well and the changed bit is set so it'll be considered dirty. Yes, that case for old machines running KVM is a bit unfortunate. I need to force the changed bit so that the read-only bit can be cleared. KVM will not work with read-only ptes if the enhanced suppression-on- protection is not available. > Reading down through the other page tables updates, I couldn't spot a place > where you were inconsistent with the handling of the hardware _PAGE_CO > and _PAGE_RO. Writes were allowed when the change bit was already set. > I couldn't follow it all, particularly around the KVM bits so take it with > a grain of salt but for me anyway; > > Acked-by: Mel Gorman <mgorman@xxxxxxx> Much appreciated. Thanks Mel. -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html