Cleaning up soft-dirty bit usage

RFC In review Cyrill Gorcunov 3 participants 6 messages Apr 3, 2014 – Apr 7, 2014

Subsystems (2)

Memory management x86

lore

Share & tools

lore

Cyrill Gorcunov

author

2014-04-03 19:09 UTC #0 lore raw

cover Cleaning up soft-dirty bit usage

Hi! I've been trying to clean up soft-dirty bit usage. I can't cleanup "ridiculous macros in pgtable-2level.h" completely because I need to define _PAGE_FILE,_PAGE_PROTNONE,_PAGE_NUMA bits in sequence manner like

#define _PAGE_BIT_FILE (_PAGE_BIT_PRESENT + 1) /* _PAGE_BIT_RW */
#define _PAGE_BIT_NUMA (_PAGE_BIT_PRESENT + 2) /* _PAGE_BIT_USER */
#define _PAGE_BIT_PROTNONE (_PAGE_BIT_PRESENT + 3) /* _PAGE_BIT_PWT */

which can't be done right now because numa code needs to save original pte bits for example in __split_huge_page_map, if I'm not missing something obvious.

Also if we ever redefine the bits above we will need to update PAT code which uses _PAGE_GLOBAL + _PAGE_PRESENT to make pte_present return true or false.

Another weird thing I found is the following sequence:

mprotect_fixup

    change_protection (passes @prot_numa = 0 which finally ends up in)
      ...
      change_pte_range(..., prot_numa)

			if (!prot_numa) {
				...
			} else {
				... this seems to be dead code branch ...
			}

    is it intentional, and @prot_numa argument is supposed to be passed
    with prot_numa = 1 one day, or it's leftover from old times?

Note I've not yet tested the series building it now, hopefully finish testing in a couple of hours.

Linus, by saying "define the bits we use when PAGE_PRESENT==0 separately and explicitly" you meant complete rework of the bits, right? Not simply group them in once place in a header?

Cyrill

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Cyrill Gorcunov

author

2014-04-03 19:09 UTC #1 lore raw

RFC 1/3 mm: pgtable -- Drop unneeded preprocessor ifdef

_PAGE_BIT_FILE (bit 6) is always less than _PAGE_BIT_PROTNONE (bit 9) so drop redundant #ifdef.

CC Linus Torvalds <[email protected]>

CC Mel Gorman <[email protected]>

CC Peter Anvin <[email protected]>

CC Ingo Molnar <[email protected]>

CC Steven Noonan <[email protected]>

CC Rik van Riel <[email protected]>

CC David Vrabel <[email protected]>

CC Andrew Morton <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Pavel Emelyanov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 arch/x86/include/asm/pgtable-2level.h |   10 ----------
 arch/x86/include/asm/pgtable_64.h     |    5 -----
 2 files changed, 15 deletions(-)

arch/x86/include/asm/pgtable-2level.h +0 ~~-10~~

Index: linux-2.6.git/arch/x86/include/asm/pgtable-2level.h
===================================================================
@@ -115,13 +115,8 @@ static __always_inline pte_t pgoff_to_pt
  */
 #define PTE_FILE_MAX_BITS	29
 #define PTE_FILE_SHIFT1		(_PAGE_BIT_PRESENT + 1)
-#if _PAGE_BIT_FILE < _PAGE_BIT_PROTNONE
 #define PTE_FILE_SHIFT2		(_PAGE_BIT_FILE + 1)
 #define PTE_FILE_SHIFT3		(_PAGE_BIT_PROTNONE + 1)
-#else
-#define PTE_FILE_SHIFT2		(_PAGE_BIT_PROTNONE + 1)
-#define PTE_FILE_SHIFT3		(_PAGE_BIT_FILE + 1)
-#endif
 #define PTE_FILE_BITS1		(PTE_FILE_SHIFT2 - PTE_FILE_SHIFT1 - 1)
 #define PTE_FILE_BITS2		(PTE_FILE_SHIFT3 - PTE_FILE_SHIFT2 - 1)

@@ -153,13 +148,8 @@ static __always_inline pte_t pgoff_to_pt
 #endif /* CONFIG_MEM_SOFT_DIRTY */
 
 /* Encode and de-code a swap entry */
-#if _PAGE_BIT_FILE < _PAGE_BIT_PROTNONE
 #define SWP_TYPE_BITS (_PAGE_BIT_FILE - _PAGE_BIT_PRESENT - 1)
 #define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 1)
-#else
-#define SWP_TYPE_BITS (_PAGE_BIT_PROTNONE - _PAGE_BIT_PRESENT - 1)
-#define SWP_OFFSET_SHIFT (_PAGE_BIT_FILE + 1)
-#endif
 
 #define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS)

arch/x86/include/asm/pgtable_64.h +0 -5

Index: linux-2.6.git/arch/x86/include/asm/pgtable_64.h
===================================================================
@@ -143,13 +143,8 @@ static inline int pgd_large(pgd_t pgd) {
 #define pte_unmap(pte) ((void)(pte))/* NOP */
 
 /* Encode and de-code a swap entry */
-#if _PAGE_BIT_FILE < _PAGE_BIT_PROTNONE
 #define SWP_TYPE_BITS (_PAGE_BIT_FILE - _PAGE_BIT_PRESENT - 1)
 #define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 1)
-#else
-#define SWP_TYPE_BITS (_PAGE_BIT_PROTNONE - _PAGE_BIT_PRESENT - 1)
-#define SWP_OFFSET_SHIFT (_PAGE_BIT_FILE + 1)
-#endif
 
 #define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS)

Cyrill Gorcunov

author

2014-04-03 19:09 UTC #2 lore raw

RFC 2/3 mm: pgtable -- Require X86_64 for soft-dirty tracker

In reply to Cyrill Gorcunov

Tracking dirty status on 2 level pages requires very ugly macros and taking into account how old the machines who can operate without PAE mode only are, lets drop soft dirty tracker from them for code simplicity (note I can't drop all the macros from 2 level pages by now since _PAGE_BIT_PROTNONE and _PAGE_BIT_FILE are still used even without tracker).

Linus proposed to completely rip off softdirty support on x86-32 (even with PAE) and since for CRIU we're not planning to support native x86-32 mode, lets do that.

(Softdirty tracker is relatively new feature which mostly used
by CRIU so I don't expect if such API change would cause problems on userspace).

CC Linus Torvalds <[email protected]>

CC Mel Gorman <[email protected]>

CC Peter Anvin <[email protected]>

CC Ingo Molnar <[email protected]>

CC Steven Noonan <[email protected]>

CC Rik van Riel <[email protected]>

CC David Vrabel <[email protected]>

CC Andrew Morton <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Pavel Emelyanov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 arch/x86/Kconfig                      |    2 -
 arch/x86/include/asm/pgtable-2level.h |   49 ----------------------------------
 2 files changed, 1 insertion(+), 50 deletions(-)

arch/x86/Kconfig +1 -1

Index: linux-2.6.git/arch/x86/Kconfig
===================================================================
@@ -104,7 +104,7 @@ config X86
 	select HAVE_ARCH_SECCOMP_FILTER
 	select BUILDTIME_EXTABLE_SORT
 	select GENERIC_CMOS_UPDATE
-	select HAVE_ARCH_SOFT_DIRTY
+	select HAVE_ARCH_SOFT_DIRTY if X86_64
 	select CLOCKSOURCE_WATCHDOG
 	select GENERIC_CLOCKEVENTS
 	select ARCH_CLOCKSOURCE_DATA if X86_64

arch/x86/include/asm/pgtable-2level.h +0 ~~-49~~

61 lines not on this page · raw message

Cyrill Gorcunov

author

2014-04-03 19:09 UTC #3 lore raw

RFC 3/3 mm: pgtable -- Use _PAGE_SOFT_DIRTY for swap entries

In reply to Cyrill Gorcunov

Since we support soft-dirty on x86-64 now we can release _PAGE_PSE bit used to track dirty swap entries and reuse ealready existing _PAGE_SOFT_DIRTY.

Thus for all soft-dirty needs we use same pte bit.

CC Linus Torvalds <[email protected]>

CC Mel Gorman <[email protected]>

CC Peter Anvin <[email protected]>

CC Ingo Molnar <[email protected]>

CC Steven Noonan <[email protected]>

CC Rik van Riel <[email protected]>

CC David Vrabel <[email protected]>

CC Andrew Morton <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Pavel Emelyanov <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 arch/x86/include/asm/pgtable_64.h    |   12 ++++++++++--
 arch/x86/include/asm/pgtable_types.h |   19 ++++---------------
 2 files changed, 14 insertions(+), 17 deletions(-)

arch/x86/include/asm/pgtable_64.h +10 -2

Index: linux-2.6.git/arch/x86/include/asm/pgtable_64.h
===================================================================
@@ -142,9 +142,17 @@ static inline int pgd_large(pgd_t pgd) {
 #define pte_offset_map(dir, address) pte_offset_kernel((dir), (address))
 #define pte_unmap(pte) ((void)(pte))/* NOP */
 
-/* Encode and de-code a swap entry */
+/*
+ * Encode and de-code a swap entry. When soft-dirty memory tracker is
+ * enabled we need to borrow _PAGE_BIT_SOFT_DIRTY bit for own needs,
+ * which limits the max size of swap partiotion about to 1T.
+ */
 #define SWP_TYPE_BITS (_PAGE_BIT_FILE - _PAGE_BIT_PRESENT - 1)
-#define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 1)
+#ifdef CONFIG_MEM_SOFT_DIRTY
+# define SWP_OFFSET_SHIFT (_PAGE_BIT_SOFT_DIRTY + 1)
+#else
+# define SWP_OFFSET_SHIFT (_PAGE_BIT_PROTNONE + 1)
+#endif
 
 #define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS)

arch/x86/include/asm/pgtable_types.h +4 ~~-15~~

Index: linux-2.6.git/arch/x86/include/asm/pgtable_types.h
===================================================================
@@ -59,29 +59,18 @@
  * The same hidden bit is used by kmemcheck, but since kmemcheck
  * works on kernel pages while soft-dirty engine on user space,
  * they do not conflict with each other.
+ *
+ * Because soft-dirty is limited to x86-64 only we can reuse this
+ * bit to track swap entries as well.
  */
 
 #define _PAGE_BIT_SOFT_DIRTY	_PAGE_BIT_HIDDEN
 
 #ifdef CONFIG_MEM_SOFT_DIRTY
 #define _PAGE_SOFT_DIRTY	(_AT(pteval_t, 1) << _PAGE_BIT_SOFT_DIRTY)
+#define _PAGE_SWP_SOFT_DIRTY	_PAGE_SOFT_DIRTY
 #else
 #define _PAGE_SOFT_DIRTY	(_AT(pteval_t, 0))
-#endif
-
-/*
- * Tracking soft dirty bit when a page goes to a swap is tricky.
- * We need a bit which can be stored in pte _and_ not conflict
- * with swap entry format. On x86 bits 6 and 7 are *not* involved
- * into swap entry computation, but bit 6 is used for nonlinear
- * file mapping, so we borrow bit 7 for soft dirty tracking.
- *
- * Please note that this bit must be treated as swap dirty page
- * mark if and only if the PTE has present bit clear!
- */
-#ifdef CONFIG_MEM_SOFT_DIRTY
-#define _PAGE_SWP_SOFT_DIRTY	_PAGE_PSE
-#else
 #define _PAGE_SWP_SOFT_DIRTY	(_AT(pteval_t, 0))
 #endif

Kirill A. Shutemov

2014-04-07 13:07 UTC #4 lore raw

In reply to Cyrill Gorcunov

Quoted 5 lines · Cyrill Gorcunov

Hi! I've been trying to clean up soft-dirty bit usage. I can't cleanup "ridiculous macros in pgtable-2level.h" completely because I need to define _PAGE_FILE,_PAGE_PROTNONE,_PAGE_NUMA bits in sequence manner like

Quoted 7 lines

#define _PAGE_BIT_FILE (_PAGE_BIT_PRESENT + 1) /* _PAGE_BIT_RW */
#define _PAGE_BIT_NUMA (_PAGE_BIT_PRESENT + 2) /* _PAGE_BIT_USER */
#define _PAGE_BIT_PROTNONE (_PAGE_BIT_PRESENT + 3) /* _PAGE_BIT_PWT */
which can't be done right now because numa code needs to save original pte bits for example in __split_huge_page_map, if I'm not missing something obvious.

Sorry, I didn't get this. How __split_huge_page_map() does depend on pte bits order?

Quoted 7 lines

Also if we ever redefine the bits above we will need to update PAT code which uses _PAGE_GLOBAL + _PAGE_PRESENT to make pte_present return true or false.
Another weird thing I found is the following sequence:

Quoted 13 lines

   mprotect_fixup
    change_protection (passes @prot_numa = 0 which finally ends up in)
      ...
      change_pte_range(..., prot_numa)

			if (!prot_numa) {
				...
			} else {
				... this seems to be dead code branch ...
			}

    is it intentional, and @prot_numa argument is supposed to be passed
    with prot_numa = 1 one day, or it's leftover from old times?

I see one more user of change_protection() -- change_prot_numa(), which has .prot_numa == 1.

Signature · 1 line

 Kirill A. Shutemov

Cyrill Gorcunov

2014-04-07 13:24 UTC #5 lore raw

Quoted 10 lines · Kirill A. Shutemov

Quoted 9 lines · Cyrill Gorcunov
Hi! I've been trying to clean up soft-dirty bit usage. I can't cleanup "ridiculous macros in pgtable-2level.h" completely because I need to define _PAGE_FILE,_PAGE_PROTNONE,_PAGE_NUMA bits in sequence manner like
#define _PAGE_BIT_FILE (_PAGE_BIT_PRESENT + 1) /* _PAGE_BIT_RW */
#define _PAGE_BIT_NUMA (_PAGE_BIT_PRESENT + 2) /* _PAGE_BIT_USER */
#define _PAGE_BIT_PROTNONE (_PAGE_BIT_PRESENT + 3) /* _PAGE_BIT_PWT */

Quoted 6 lines

Quoted 3 lines
which can't be done right now because numa code needs to save original pte bits for example in __split_huge_page_map, if I'm not missing something obvious.
Sorry, I didn't get this. How __split_huge_page_map() does depend on pte bits order?

__split_huge_page_map
...
for (i = 0; i < HPAGE_PMD_NR; i++, haddr += PAGE_SIZE) {
...
here we modify with pte bits
entry = pte_mknuma(entry); --> clean _PAGE_PRESENT and set _PAGE_NUMA

pte bits must remain valid and meaningful, for example we might have set _PAGE_RW here

Quoted 5 lines

Quoted 2 lines
is it intentional, and @prot_numa argument is supposed to be passed with prot_numa = 1 one day, or it's leftover from old times?
I see one more user of change_protection() -- change_prot_numa(), which has .prot_numa == 1.

Yeah, thanks, managed to miss this.