Skip to contentSkip to conversation

mm: convert scan_control.priority int => byte

PATCH In review Greg Thelen 2 participants 5 messages May 29, 2018 – Jun 20, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm: convert scan_control.priority int => byte

Reclaim priorities range from 0..12(DEF_PRIORITY). scan_control.priority is a 4 byte int, which is overkill.

Since commit 6538b8ea886e ("x86_64: expand kernel stack to 16K") x86_64 stack overflows are not an issue. But it's inefficient to use 4 bytes for priority.

Use s8 (signed byte) rather than u8 to allow for loops like:

	do {
		...
	} while (--sc.priority >= 0);

This reduces sizeof(struct scan_control) from 96 => 88 bytes (x86_64), which saves some stack.

scan_control.priority field order is changed to occupy otherwise unused padding.

---
 mm/vmscan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
mm/vmscan.c +3 -3
index 9b697323a88c..541c334bd176 100644
@@ -83,9 +83,6 @@ struct scan_control {
 	 */
 	struct mem_cgroup *target_mem_cgroup;
 
-	/* Scan (total_size >> priority) pages at once */
-	int priority;
-
 	/* The highest zone to isolate pages for reclaim from */
 	enum zone_type reclaim_idx;
 
@@ -111,6 +108,9 @@ struct scan_control {
 	/* One of the zones is ready for compaction */
 	unsigned int compaction_ready:1;
 
+	/* Scan (total_size >> priority) pages at once */
+	s8 priority;
+
 	/* Incremented by the number of inactive pages that were scanned */
 	unsigned long nr_scanned;
 
Signature · 1 line
2.17.0.921.gf22659ad46-goog
#1 lore raw
Quoted 6 lines · Greg Thelen

Reclaim priorities range from 0..12(DEF_PRIORITY). scan_control.priority is a 4 byte int, which is overkill.

Since commit 6538b8ea886e ("x86_64: expand kernel stack to 16K") x86_64 stack overflows are not an issue. But it's inefficient to use 4 bytes for priority.

If you're looking to shave a few more bytes, allocation order can fit in a u8 too (can't be more than 6 bits, and realistically won't be more than 4 bits). reclaim_idx likewise will fit in a u8, and actually won't be more than 3 bits.

I am sceptical that nr_to_reclaim should really be an unsigned long; I don't think we should be trying to free 4 billion pages in a single call. nr_scanned might be over 4 billion (!) but nr_reclaimed can probably shrink to unsigned int along with nr_to_reclaim.

#2 lore raw
Quoted 4 lines · Matthew Wilcox
Quoted 3 lines · Greg Thelen

Reclaim priorities range from 0..12(DEF_PRIORITY). scan_control.priority is a 4 byte int, which is overkill.

Quoted 8 lines
Quoted 3 lines

Since commit 6538b8ea886e ("x86_64: expand kernel stack to 16K") x86_64 stack overflows are not an issue. But it's inefficient to use 4 bytes for priority.

If you're looking to shave a few more bytes, allocation order can fit in a u8 too (can't be more than 6 bits, and realistically won't be more than 4 bits). reclaim_idx likewise will fit in a u8, and actually won't be more than 3 bits.

Nod. Good tip. Included in ("[PATCH v2] mm: condense scan_control").

Quoted 4 lines

I am sceptical that nr_to_reclaim should really be an unsigned long; I don't think we should be trying to free 4 billion pages in a single call. nr_scanned might be over 4 billion (!) but nr_reclaimed can probably shrink to unsigned int along with nr_to_reclaim.

Agreed. For patch simplicity, I'll pass on this for now.

#3 lore raw

PATCH v2 1/1 mm: condense scan_control

In reply to Matthew Wilcox

Use smaller scan_control fields for order, priority, and reclaim_idx. Convert fields from int => s8. All easily fit within a byte:
* allocation order range: 0..MAX_ORDER(64?)
* priority range: 0..12(DEF_PRIORITY)
* reclaim_idx range: 0..6(__MAX_NR_ZONES)

Since commit 6538b8ea886e ("x86_64: expand kernel stack to 16K") x86_64 stack overflows are not an issue. But it's inefficient to use ints.

Use s8 (signed byte) rather than u8 to allow for loops like:

	do {
		...
	} while (--sc.priority >= 0);

Add BUILD_BUG_ON to verify that s8 is capable of storing max values.

This reduces sizeof(struct scan_control):
* 96 => 80 bytes (x86_64)
* 68 => 56 bytes (i386)

scan_control structure field order is changed to utilize padding. After this patch there is 1 bit of scan_control padding.

---
 mm/vmscan.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)
mm/vmscan.c +20 -12
index 9b697323a88c..42731faea306 100644
@@ -65,12 +65,6 @@ struct scan_control {
 	/* How many pages shrink_list() should reclaim */
 	unsigned long nr_to_reclaim;
 
-	/* This context's GFP mask */
-	gfp_t gfp_mask;
-
-	/* Allocation order */
-	int order;
-
 	/*
 	 * Nodemask of nodes allowed by the caller. If NULL, all nodes
 	 * are scanned.
@@ -83,12 +77,6 @@ struct scan_control {
 	 */
 	struct mem_cgroup *target_mem_cgroup;
 
-	/* Scan (total_size >> priority) pages at once */
-	int priority;
-
-	/* The highest zone to isolate pages for reclaim from */
-	enum zone_type reclaim_idx;
-
 	/* Writepage batching in laptop mode; RECLAIM_WRITE */
 	unsigned int may_writepage:1;
 
@@ -111,6 +99,18 @@ struct scan_control {
 	/* One of the zones is ready for compaction */
 	unsigned int compaction_ready:1;
 
+	/* Allocation order */
+	s8 order;
+
+	/* Scan (total_size >> priority) pages at once */
+	s8 priority;
+
+	/* The highest zone to isolate pages for reclaim from */
+	s8 reclaim_idx;
+
+	/* This context's GFP mask */
+	gfp_t gfp_mask;
+
 	/* Incremented by the number of inactive pages that were scanned */
 	unsigned long nr_scanned;
 
@@ -3047,6 +3047,14 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.may_swap = 1,
 	};
 
+	/*
+	 * scan_control uses s8 fields for order, priority, and reclaim_idx.
+	 * Confirm they are large enough for max values.
+	 */
+	BUILD_BUG_ON(MAX_ORDER > S8_MAX);
+	BUILD_BUG_ON(DEF_PRIORITY > S8_MAX);
+	BUILD_BUG_ON(MAX_NR_ZONES > S8_MAX);
+
 	/*
 	 * Do not enter reclaim if fatal signal was delivered while throttled.
 	 * 1 is returned so that the page allocator does not OOM kill at this
Signature · 1 line
2.17.0.921.gf22659ad46-goog
#4 lore raw

Re: [PATCH v2] mm: condense scan_control

Quoted 21 lines · Greg Thelen

Use smaller scan_control fields for order, priority, and reclaim_idx. Convert fields from int => s8. All easily fit within a byte:
* allocation order range: 0..MAX_ORDER(64?)
* priority range: 0..12(DEF_PRIORITY)
* reclaim_idx range: 0..6(__MAX_NR_ZONES)

Since commit 6538b8ea886e ("x86_64: expand kernel stack to 16K") x86_64 stack overflows are not an issue. But it's inefficient to use ints.

Use s8 (signed byte) rather than u8 to allow for loops like:

        do {
                ...
        } while (--sc.priority >= 0);

Add BUILD_BUG_ON to verify that s8 is capable of storing max values.

This reduces sizeof(struct scan_control):
* 96 => 80 bytes (x86_64)
* 68 => 56 bytes (i386)

Quoted 5 lines

scan_control structure field order is changed to utilize padding. After this patch there is 1 bit of scan_control padding.

Is there any interest in this? Less stack usage could mean less dcache and dtlb pressure. But I understand if the complexity is distasteful.

Quoted 71 lines
---
 mm/vmscan.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)
mm/vmscan.c +20 -13
index 9b697323a88c..42731faea306 100644
@@ -65,12 +65,6 @@ struct scan_control {
        /* How many pages shrink_list() should reclaim */
        unsigned long nr_to_reclaim;
 
-       /* This context's GFP mask */
-       gfp_t gfp_mask;
-
-       /* Allocation order */
-       int order;
-
        /*
         * Nodemask of nodes allowed by the caller. If NULL, all nodes
         * are scanned.
@@ -83,12 +77,6 @@ struct scan_control {
         */
        struct mem_cgroup *target_mem_cgroup;
 
-       /* Scan (total_size >> priority) pages at once */
-       int priority;
-
-       /* The highest zone to isolate pages for reclaim from */
-       enum zone_type reclaim_idx;
-
        /* Writepage batching in laptop mode; RECLAIM_WRITE */
        unsigned int may_writepage:1;
 
@@ -111,6 +99,18 @@ struct scan_control {
        /* One of the zones is ready for compaction */
        unsigned int compaction_ready:1;
 
+       /* Allocation order */
+       s8 order;
+
+       /* Scan (total_size >> priority) pages at once */
+       s8 priority;
+
+       /* The highest zone to isolate pages for reclaim from */
+       s8 reclaim_idx;
+
+       /* This context's GFP mask */
+       gfp_t gfp_mask;
+
        /* Incremented by the number of inactive pages that were scanned */
        unsigned long nr_scanned;
 
@@ -3047,6 +3047,14 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
                .may_swap = 1,
        };
 
+       /*
+        * scan_control uses s8 fields for order, priority, and reclaim_idx.
+        * Confirm they are large enough for max values.
+        */
+       BUILD_BUG_ON(MAX_ORDER > S8_MAX);
+       BUILD_BUG_ON(DEF_PRIORITY > S8_MAX);
+       BUILD_BUG_ON(MAX_NR_ZONES > S8_MAX);
+
        /*
         * Do not enter reclaim if fatal signal was delivered while throttled.
         * 1 is returned so that the page allocator does not OOM kill at this
--

2.17.0.921.gf22659ad46-goog