zram/xvmalloc: 64K page fixes and optimizations

PATCH Superseded Robert Jennings 4 participants 26 messages Jan 26, 2011 Staging

Share & tools

Robert Jennings

author

2011-01-26 17:21 UTC #0 lore raw

cover zram/xvmalloc: 64K page fixes and optimizations

The xvmalloc allocator is non-functional when running with a 64K page size. The first two patches fix 64K page related issues.

The remaining patches provide some small optimizations for zram and xvmalloc.

Regards,
Robert Jennings

Robert Jennings

author

2011-01-26 17:23 UTC #1 lore raw

PATCH 1/7 zram/vmalloc: Correct tunings to enable use with 64K pages

xvmalloc will not currently function with 64K pages. Newly allocated pages will be inserted at an offset beyond the end of the first-level index. This tuning is needed to properly size the allocator for 64K pages.

The default 3 byte shift results in a second level list size which can not be indexed using the 64 bits of the flbitmap in the xv_pool structure. The shift must increase to 4 bytes between second level list entries to fit the size of the first level bitmap.

Here are a few statistics for structure sizes on 32- and 64-bit CPUs with 4KB and 64KB page sizes.

bits_per_long 32 64 64
page_size 4,096 4,096 65,535

xv_align                    4         8         8
fl_delta                    3         3         4

num_free_lists 508 508 4,094
xv_pool size 4,144b 8,216b 66,040b
per object overhead 32 64 64
zram struct 0.5GB disk 512KB 1024KB 64KB

This patch maintains the current tunings for 4K pages, adds an optimal sizing for 64K pages and adds a safe tuning for any other page sizes.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc_int.h |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

drivers/staging/zram/xvmalloc_int.h +16 -2

index e23ed5c..051a49b 100644
@@ -19,7 +19,11 @@
 /* User configurable params */
 
 /* Must be power of two */
+#ifdef CONFIG_64BIT
+#define XV_ALIGN_SHIFT 3
+#else
 #define XV_ALIGN_SHIFT	2
+#endif
 #define XV_ALIGN	(1 << XV_ALIGN_SHIFT)
 #define XV_ALIGN_MASK	(XV_ALIGN - 1)

@@ -27,8 +31,18 @@
 #define XV_MIN_ALLOC_SIZE	32
 #define XV_MAX_ALLOC_SIZE	(PAGE_SIZE - XV_ALIGN)
 
-/* Free lists are separated by FL_DELTA bytes */
-#define FL_DELTA_SHIFT	3
+/*
+ * Free lists are separated by FL_DELTA bytes
+ * This value is 3 for 4k pages and 4 for 64k pages, for any
+ * other page size, a conservative (PAGE_SHIFT - 9) is used.
+ */
+#if PAGE_SHIFT == 12
+#define FL_DELTA_SHIFT 3
+#elif PAGE_SHIFT == 16
+#define FL_DELTA_SHIFT 4
+#else
+#define FL_DELTA_SHIFT (PAGE_SHIFT - 9)
+#endif
 #define FL_DELTA	(1 << FL_DELTA_SHIFT)
 #define FL_DELTA_MASK	(FL_DELTA - 1)
 #define NUM_FREE_LISTS	((XV_MAX_ALLOC_SIZE - XV_MIN_ALLOC_SIZE) \

Signature · 1 line

1.6.0.2

Pekka Enberg

2011-01-26 17:48 UTC #2 lore raw

Re: [PATCH 1/7] zram/vmalloc: Correct tunings to enable use with 64K pages

Quoted 49 lines · Robert Jennings

xvmalloc will not currently function with 64K pages. Newly allocated pages will be inserted at an offset beyond the end of the first-level index. This tuning is needed to properly size the allocator for 64K pages.
The default 3 byte shift results in a second level list size which can not be indexed using the 64 bits of the flbitmap in the xv_pool structure. The shift must increase to 4 bytes between second level list entries to fit the size of the first level bitmap.
Here are a few statistics for structure sizes on 32- and 64-bit CPUs with 4KB and 64KB page sizes.
bits_per_long 32 64 64 page_size 4,096 4,096 65,535 xv_align 4 8 8 fl_delta 3 3 4 num_free_lists 508 508 4,094 xv_pool size 4,144b 8,216b 66,040b per object overhead 32 64 64 zram struct 0.5GB disk 512KB 1024KB 64KB
This patch maintains the current tunings for 4K pages, adds an optimal sizing for 64K pages and adds a safe tuning for any other page sizes.
Signed-off-by Robert Jennings <[email protected]>
---
drivers/staging/zram/xvmalloc_int.h | 18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/staging/zram/xvmalloc_int.h +4 -0
index e23ed5c..051a49b 100644
@@ -19,7 +19,11 @@
  /* User configurable params */
 
  /* Must be power of two */
+#ifdef CONFIG_64BIT
+#define XV_ALIGN_SHIFT 3
+#else
  #define XV_ALIGN_SHIFT 2
+#endif
  #define XV_ALIGN       (1 << XV_ALIGN_SHIFT)
  #define XV_ALIGN_MASK  (XV_ALIGN - 1)
 
@@ -27,8 +31,18 @@
  #define XV_MIN_ALLOC_SIZE      32
  #define XV_MAX_ALLOC_SIZE      (PAGE_SIZE - XV_ALIGN)

drivers/staging/zram/xvmalloc_int.h line 40

Quoted 10 lines

drivers/staging/zram/xvmalloc_int.h +7 -2

 
-/* Free lists are separated by FL_DELTA bytes */
-#define FL_DELTA_SHIFT 3
+/*
+ * Free lists are separated by FL_DELTA bytes
+ * This value is 3 for 4k pages and 4 for 64k pages, for any
+ * other page size, a conservative (PAGE_SHIFT - 9) is used.
+ */
+#if PAGE_SHIFT == 12
+#define FL_DELTA_SHIFT 3

This is handled by the else branch already, no?

Quoted 5 lines

drivers/staging/zram/xvmalloc_int.h +5 -0

+#elif PAGE_SHIFT == 16
+#define FL_DELTA_SHIFT 4
+#else
+#define FL_DELTA_SHIFT (PAGE_SHIFT - 9)
+#endif

Robert Jennings

author

2011-01-26 18:50 UTC #3 lore raw

Re: [PATCH 1/7] zram/vmalloc: Correct tunings to enable use with 64K pages

Quoted 35 lines · Pekka Enberg

Quoted 34 lines · On Wed, Jan 26, 2011 at 7:22 PM, Robert Jennings> <[email protected]>…
xvmalloc will not currently function with 64K pages. Newly allocated pages will be inserted at an offset beyond the end of the first-level index. This tuning is needed to properly size the allocator for 64K pages.
The default 3 byte shift results in a second level list size which can not be indexed using the 64 bits of the flbitmap in the xv_pool structure. The shift must increase to 4 bytes between second level list entries to fit the size of the first level bitmap.
Here are a few statistics for structure sizes on 32- and 64-bit CPUs with 4KB and 64KB page sizes.
bits_per_long 32 64 64
page_size 4,096 4,096 65,535
xv_align                    4         8         8
fl_delta                    3         3         4
num_free_lists 508 508 4,094
xv_pool size 4,144b 8,216b 66,040b
per object overhead 32 64 64
zram struct 0.5GB disk 512KB 1024KB 64KB
This patch maintains the current tunings for 4K pages, adds an optimal sizing for 64K pages and adds a safe tuning for any other page sizes.
Signed-off-by Robert Jennings <[email protected]>
---
 drivers/staging/zram/xvmalloc_int.h |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/staging/zram/xvmalloc_int.h +0 -0
index e23ed5c..051a49b 100644

<snip>

Quoted 4 lines

drivers/staging/zram/xvmalloc_int.h +0 -0

@@ -27,8 +31,18 @@
 #define XV_MIN_ALLOC_SIZE	32
 #define XV_MAX_ALLOC_SIZE	(PAGE_SIZE - XV_ALIGN)

Quoted 11 lines

drivers/staging/zram/xvmalloc_int.h line 40

Quoted 9 lines

drivers/staging/zram/xvmalloc_int.h +7 -2

-/* Free lists are separated by FL_DELTA bytes */
-#define FL_DELTA_SHIFT	3
+/*
+ * Free lists are separated by FL_DELTA bytes
+ * This value is 3 for 4k pages and 4 for 64k pages, for any
+ * other page size, a conservative (PAGE_SHIFT - 9) is used.
+ */
+#if PAGE_SHIFT == 12
+#define FL_DELTA_SHIFT 3

This is handled by the else branch already, no?

Yes, it does not need to be there. I will repost.

Robert Jennings

author

2011-01-26 17:23 UTC #4 lore raw

PATCH 2/7 zram: Prevent overflow in logical block size

In reply to Robert Jennings

On a 64K page kernel, the value PAGE_SIZE passed to blk_queue_logical_block_size would overflow the logical block size argument (resulting in setting it to 0).

Take the minimum of PAGE_SIZE or 4096 and use this for the block device logical block size.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/zram_drv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

drivers/staging/zram/zram_drv.c +2 -1

index d0e9e02..d5e0275 100644
@@ -621,7 +621,8 @@ static int create_device(struct zram *zram, int device_id)
 	 * and n*PAGE_SIZED sized I/O requests.
 	 */
 	blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
-	blk_queue_logical_block_size(zram->disk->queue, PAGE_SIZE);
+	blk_queue_logical_block_size(zram->disk->queue,
+			(unsigned short) min_t(unsigned int, PAGE_SIZE, 4096));
 	blk_queue_io_min(zram->disk->queue, PAGE_SIZE);
 	blk_queue_io_opt(zram->disk->queue, PAGE_SIZE);

Signature · 1 line

1.6.0.2

Pekka Enberg

2011-01-26 17:42 UTC #5 lore raw

Re: [PATCH 2/7] zram: Prevent overflow in logical block size

Quoted 17 lines · Robert Jennings

On a 64K page kernel, the value PAGE_SIZE passed to blk_queue_logical_block_size would overflow the logical block size argument (resulting in setting it to 0).
Take the minimum of PAGE_SIZE or 4096 and use this for the block device logical block size.
Signed-off-by Robert Jennings <[email protected]>
---
drivers/staging/zram/zram_drv.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/staging/zram/zram_drv.c +0 -0
index d0e9e02..d5e0275 100644

drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 625

Quoted 6 lines

drivers/staging/zram/zram_drv.c +2 -1

@@ -621,7 +621,8 @@ static int create_device(struct zram *zram, int device_id)
         * and n*PAGE_SIZED sized I/O requests.
         */
        blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
-       blk_queue_logical_block_size(zram->disk->queue, PAGE_SIZE);
+       blk_queue_logical_block_size(zram->disk->queue,
+                       (unsigned short) min_t(unsigned int, PAGE_SIZE, 4096));

I don't get it. No architecture supports PAGE_SIZE less than 4K so that expression always ends up being 4096, no?

Quoted 12 lines

drivers/staging/zram/zram_drv.c +0 -1
        blk_queue_io_min(zram->disk->queue, PAGE_SIZE);
        blk_queue_io_opt(zram->disk->queue, PAGE_SIZE);
 
--
1.6.0.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Robert Jennings

author

2011-01-26 19:09 UTC #6 lore raw

Re: [PATCH 2/7] zram: Prevent overflow in logical block size

Quoted 13 lines · Pekka Enberg

Quoted 12 lines · Robert Jennings
On a 64K page kernel, the value PAGE_SIZE passed to blk_queue_logical_block_size would overflow the logical block size argument (resulting in setting it to 0).
Take the minimum of PAGE_SIZE or 4096 and use this for the block device logical block size.
Signed-off-by Robert Jennings <[email protected]>
---
 drivers/staging/zram/zram_drv.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Quoted 14 lines

drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 625
Quoted 11 lines
drivers/staging/zram/zram_drv.c +2 -1
index d0e9e02..d5e0275 100644
@@ -621,7 +621,8 @@ static int create_device(struct zram *zram, int device_id)
 	 * and n*PAGE_SIZED sized I/O requests.
 	 */
 	blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
-	blk_queue_logical_block_size(zram->disk->queue, PAGE_SIZE);
+	blk_queue_logical_block_size(zram->disk->queue,
+			(unsigned short) min_t(unsigned int, PAGE_SIZE, 4096));
I don't get it. No architecture supports PAGE_SIZE less than 4K so that expression always ends up being 4096, no?

Yes, I will cut out the min_t and just hard-code this to 4096.

Pekka Enberg

2011-01-26 19:14 UTC #7 lore raw

Re: [PATCH 2/7] zram: Prevent overflow in logical block size

Quoted 29 lines · Robert Jennings

Quoted 13 lines · Pekka Enberg
Quoted 12 lines · Robert Jennings
On a 64K page kernel, the value PAGE_SIZE passed to blk_queue_logical_block_size would overflow the logical block size argument (resulting in setting it to 0).
Take the minimum of PAGE_SIZE or 4096 and use this for the block device logical block size.
Signed-off-by Robert Jennings <[email protected]>
---
drivers/staging/zram/zram_drv.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Quoted 14 lines
drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 625
Quoted 11 lines
drivers/staging/zram/zram_drv.c +2 -1
index d0e9e02..d5e0275 100644
@@ -621,7 +621,8 @@ static int create_device(struct zram *zram, int device_id)
       * and n*PAGE_SIZED sized I/O requests.
       */
      blk_queue_physical_block_size(zram->disk->queue, PAGE_SIZE);
-    blk_queue_logical_block_size(zram->disk->queue, PAGE_SIZE);
+    blk_queue_logical_block_size(zram->disk->queue,
+                    (unsigned short) min_t(unsigned int, PAGE_SIZE, 4096));
I don't get it. No architecture supports PAGE_SIZE less than 4K so that expression always ends up being 4096, no?

Quoted 1 line

Yes, I will cut out the min_t and just hard-code this to 4096.

Please make a ZRAM_BLOCK_SIZE constant or something for that.

Robert Jennings

author

2011-01-26 17:24 UTC #8 lore raw

PATCH 3/7 zram: Speed insertion of new pages with cached idx

In reply to Robert Jennings

Calculate the first- and second-level indices for new page when the pool is initialized rather than calculating them on each insertion.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc.c     |   13 +++++++++++--
 drivers/staging/zram/xvmalloc_int.h |    4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

drivers/staging/zram/xvmalloc.c +11 -2

index 3fdbb8a..a507f95 100644
@@ -184,8 +184,13 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
 	u32 flindex, slindex;
 	struct block_header *nextblock;
 
-	slindex = get_index_for_insert(block->size);
-	flindex = slindex / BITS_PER_LONG;
+	if (block->size >= (PAGE_SIZE - XV_ALIGN)) {
+		slindex = pagesize_slindex;
+		flindex = pagesize_flindex;
+	} else {
+		slindex = get_index_for_insert(block->size);
+		flindex = slindex / BITS_PER_LONG;
+	}
 
 	block->link.prev_page = 0;
 	block->link.prev_offset = 0;

@@ -316,6 +321,10 @@ struct xv_pool *xv_create_pool(void)
 	if (!pool)
 		return NULL;
 
+	/* cache the first/second-level indices for PAGE_SIZE allocations */
+	pagesize_slindex = get_index_for_insert(PAGE_SIZE);
+	pagesize_flindex = pagesize_slindex / BITS_PER_LONG;
+
 	spin_lock_init(&pool->lock);
 
 	return pool;

drivers/staging/zram/xvmalloc_int.h +4 -0

index 051a49b..68db384 100644
@@ -52,6 +52,10 @@
 
 /* End of user params */
 
+/* Cache of the first/second-level indices for PAGE_SIZE allocations */
+static u32 pagesize_slindex;
+static u32 pagesize_flindex;
+
 enum blockflags {
 	BLOCK_FREE,
 	PREV_FREE,

Signature · 1 line

1.6.0.2

Pekka Enberg

2011-01-26 17:53 UTC #9 lore raw

Re: [PATCH 3/7] zram: Speed insertion of new pages with cached idx

Quoted 30 lines · Robert Jennings

Calculate the first- and second-level indices for new page when the pool is initialized rather than calculating them on each insertion.

Signed-off-by Robert Jennings <[email protected]>

---
drivers/staging/zram/xvmalloc.c | 13 +++++++++++-- drivers/staging/zram/xvmalloc_int.h | 4 ++++

 2 files changed, 15 insertions(+), 2 deletions(-)

drivers/staging/zram/xvmalloc.c +7 -2

index 3fdbb8a..a507f95 100644
@@ -184,8 +184,13 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
        u32 flindex, slindex;
        struct block_header *nextblock;
 
-       slindex = get_index_for_insert(block->size);
-       flindex = slindex / BITS_PER_LONG;
+       if (block->size >= (PAGE_SIZE - XV_ALIGN)) {
+               slindex = pagesize_slindex;
+               flindex = pagesize_flindex;
+       } else {
+               slindex = get_index_for_insert(block->size);
+               flindex = slindex / BITS_PER_LONG;
+       }
 
        block->link.prev_page = 0;
        block->link.prev_offset = 0;

drivers/staging/zram/xvmalloc.c struct xv_pool *xv_create_pool(void) · line 326

Quoted 6 lines

drivers/staging/zram/xvmalloc.c +3 -0

@@ -316,6 +321,10 @@ struct xv_pool *xv_create_pool(void)
        if (!pool)
                return NULL;
 
+       /* cache the first/second-level indices for PAGE_SIZE allocations */
+       pagesize_slindex = get_index_for_insert(PAGE_SIZE);
+       pagesize_flindex = pagesize_slindex / BITS_PER_LONG;

Why is this in xv_create_pool(). AFAICT, it can be called multiple times if there's more than one zram device. Do we really need variables for these? They look like something GCC constant propagation should take care of if they would be defines or static inline functions.

Quoted 9 lines

drivers/staging/zram/xvmalloc.c +1 -0
+
        spin_lock_init(&pool->lock);
 
        return pool;
drivers/staging/zram/xvmalloc_int.h +0 -0
index 051a49b..68db384 100644

drivers/staging/zram/xvmalloc_int.h line 57

Quoted 6 lines

drivers/staging/zram/xvmalloc_int.h +3 -0

@@ -52,6 +52,10 @@
 
  /* End of user params */
 
+/* Cache of the first/second-level indices for PAGE_SIZE allocations */
+static u32 pagesize_slindex;
+static u32 pagesize_flindex;

This doesn't look right. You shouldn't put variable declarations in header files.

Quoted 13 lines

drivers/staging/zram/xvmalloc_int.h +1 -1
+
  enum blockflags {
        BLOCK_FREE,
        PREV_FREE,
--
1.6.0.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Robert Jennings

author

2011-01-26 19:50 UTC #10 lore raw

Re: [PATCH 3/7] zram: Speed insertion of new pages with cached idx

Quoted 34 lines · * Pekka Enberg ([email protected]) wrote:

Quoted 33 lines · On Wed, Jan 26, 2011 at 7:23 PM, Robert Jennings> <[email protected]>…

Calculate the first- and second-level indices for new page when the pool is initialized rather than calculating them on each insertion.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc.c     |   13 +++++++++++--
 drivers/staging/zram/xvmalloc_int.h |    4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

drivers/staging/zram/xvmalloc.c +7 -2

index 3fdbb8a..a507f95 100644
@@ -184,8 +184,13 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
 	u32 flindex, slindex;
 	struct block_header *nextblock;
 
-	slindex = get_index_for_insert(block->size);
-	flindex = slindex / BITS_PER_LONG;
+	if (block->size >= (PAGE_SIZE - XV_ALIGN)) {
+		slindex = pagesize_slindex;
+		flindex = pagesize_flindex;
+	} else {
+		slindex = get_index_for_insert(block->size);
+		flindex = slindex / BITS_PER_LONG;
+	}
 
 	block->link.prev_page = 0;
 	block->link.prev_offset = 0;

@@ -316,6 +321,10 @@ struct xv_pool *xv_create_pool(void)
 	if (!pool)
 		return NULL;

Quoted 9 lines

drivers/staging/zram/xvmalloc.c struct xv_pool *xv_create_pool(void) · line 326
Quoted 3 lines
drivers/staging/zram/xvmalloc.c +3 -0
+	/* cache the first/second-level indices for PAGE_SIZE allocations */
+	pagesize_slindex = get_index_for_insert(PAGE_SIZE);
+	pagesize_flindex = pagesize_slindex / BITS_PER_LONG;
Why is this in xv_create_pool(). AFAICT, it can be called multiple times if there's more than one zram device. Do we really need variables for these? They look like something GCC constant propagation should take care of if they would be defines or static inline functions.

It should have been a define rather than in xv_create_pool but as I read more about GCC constant propagation and look at the get_index_for_insert I believe that this patch is unnecessary. For sizes near PAGE_SIZE (>XV_MAX_ALLOC_SIZE) I believe GCC constant propagation should do exactly what I though I was trying to do. I will drop this patch. Thank you for your reviews.

Robert Jennings

author

2011-01-26 17:25 UTC #11 lore raw

PATCH 4/7 zram/xvmalloc: free bit block insertion optimization

In reply to Robert Jennings

There is no need to set the bits in the first- and second-level indices to indicate a free page when we know that a free page existed at this level.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/staging/zram/xvmalloc.c +2 -0

index a507f95..b3622f1 100644
@@ -205,6 +205,8 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
 		nextblock->link.prev_page = page;
 		nextblock->link.prev_offset = offset;
 		put_ptr_atomic(nextblock, KM_USER1);
+		/* If there was a next page then the free bits are set. */
+		return;
 	}
 
 	__set_bit(slindex % BITS_PER_LONG, &pool->slbitmap[flindex]);

Signature · 1 line

1.6.0.2

Pekka Enberg

2011-01-26 17:55 UTC #12 lore raw

Re: [PATCH 4/7] zram/xvmalloc: free bit block insertion optimization

Quoted 5 lines · Robert Jennings

There is no need to set the bits in the first- and second-level indices to indicate a free page when we know that a free page existed at this level.
Signed-off-by Robert Jennings <[email protected]>

Why is it not necessary? I don't know that part of the code well enough to tell if this patch is safe or not.

Quoted 26 lines

---
drivers/staging/zram/xvmalloc.c | 2 ++

 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/staging/zram/xvmalloc.c +2 -1

index a507f95..b3622f1 100644
@@ -205,6 +205,8 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
                nextblock->link.prev_page = page;
                nextblock->link.prev_offset = offset;
                put_ptr_atomic(nextblock, KM_USER1);
+               /* If there was a next page then the free bits are set. */
+               return;
        }
 
        __set_bit(slindex % BITS_PER_LONG, &pool->slbitmap[flindex]);
--

1.6.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Robert Jennings

author

2011-01-26 18:42 UTC #13 lore raw

Re: [PATCH 4/7] zram/xvmalloc: free bit block insertion optimization

Quoted 9 lines · * Pekka Enberg ([email protected]) wrote:

Quoted 5 lines · Robert Jennings
There is no need to set the bits in the first- and second-level indices to indicate a free page when we know that a free page existed at this level.
Signed-off-by Robert Jennings <[email protected]>
Why is it not necessary? I don't know that part of the code well enough to tell if this patch is safe or not.

This change is in a conditional block which is entered only when there is an existing data block on the freelist where the insert has taken place.

The new block is pushed onto the freelist stack and this conditional block is updating links in the prior stack head to point to the new stack head. After this conditional block the first-/second-level indices are updated to indicate that there is a free block at this location.

This patch adds an immediate return from the conditional block to avoid setting bits again to indicate a free block on this freelist. They would already be set because there was an existing free block on this freelist.

Quoted 17 lines

---
drivers/staging/zram/xvmalloc.c | 2 ++

 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/staging/zram/xvmalloc.c +2 -0

index a507f95..b3622f1 100644
@@ -205,6 +205,8 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
                nextblock->link.prev_page = page;
                nextblock->link.prev_offset = offset;
                put_ptr_atomic(nextblock, KM_USER1);
+               /* If there was a next page then the free bits are set. */
+               return;
        }
 
        __set_bit(slindex % BITS_PER_LONG, &pool->slbitmap[flindex]);

Pekka Enberg

Reviewed-by

2011-01-26 18:51 UTC #14 lore raw

Re: [PATCH 4/7] zram/xvmalloc: free bit block insertion optimization

Quoted 14 lines · Robert Jennings

Quoted 9 lines · * Pekka Enberg ([email protected]) wrote:
Quoted 5 lines · Robert Jennings
There is no need to set the bits in the first- and second-level indices to indicate a free page when we know that a free page existed at this level.
Signed-off-by Robert Jennings <[email protected]>
Why is it not necessary? I don't know that part of the code well enough to tell if this patch is safe or not.
This change is in a conditional block which is entered only when there is an existing data block on the freelist where the insert has taken place.

Quoted 8 lines

The new block is pushed onto the freelist stack and this conditional block is updating links in the prior stack head to point to the new stack head. After this conditional block the first-/second-level indices are updated to indicate that there is a free block at this location.
This patch adds an immediate return from the conditional block to avoid setting bits again to indicate a free block on this freelist. They would already be set because there was an existing free block on this freelist.

Some of that information could be put in the changelog.

Reviewed-by Pekka Enberg <[email protected]>

Nitin Gupta

Reviewed-by on v1 4/7

2011-01-26 18:44 UTC #15 lore raw

Re: [PATCH 4/7] zram/xvmalloc: free bit block insertion optimization

In reply to Robert Jennings

Quoted 6 lines · Robert Jennings

There is no need to set the bits in the first- and second-level indices to indicate a free page when we know that a free page existed at this level.
Signed-off-by Robert Jennings<[email protected]>

Reviewed-by Nitin Gupta <[email protected]>

Thanks for all the fixes and cleanups.
Nitin

Robert Jennings

author

2011-01-26 17:29 UTC #16 lore raw

PATCH 5/7 zram/xvmalloc: wrap debug code in #ifdef DEBUG

In reply to Robert Jennings

This wraps the code, noted by comments as being debug code, with #ifdef DEBUG so that it is removed from running in non-debug kernels.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

drivers/staging/zram/xvmalloc.c +3 -1

index b3622f1..172514e 100644
@@ -219,7 +219,6 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
 static void remove_block_head(struct xv_pool *pool,
 			struct block_header *block, u32 slindex)
 {
-	struct block_header *tmpblock;
 	u32 flindex = slindex / BITS_PER_LONG;
 
 	pool->freelist[slindex].page = block->link.next_page;

@@ -232,6 +231,8 @@ static void remove_block_head(struct xv_pool *pool,
 		if (!pool->slbitmap[flindex])
 			__clear_bit(flindex, &pool->flbitmap);
 	} else {
+#ifdef DEBUG
+		struct block_header *tmpblock;
 		/*
 		 * DEBUG ONLY: We need not reinitialize freelist head previous
 		 * pointer to 0 - we never depend on its value. But just for

@@ -242,6 +243,7 @@ static void remove_block_head(struct xv_pool *pool,
 		tmpblock->link.prev_page = 0;
 		tmpblock->link.prev_offset = 0;
 		put_ptr_atomic(tmpblock, KM_USER1);
+#endif
 	}
 }

Signature · 1 line

1.6.0.2

Pekka Enberg

2011-01-26 17:44 UTC #17 lore raw

Re: [PATCH 5/7] zram/xvmalloc: wrap debug code in #ifdef DEBUG

Quoted 21 lines · Robert Jennings

This wraps the code, noted by comments as being debug code, with #ifdef DEBUG so that it is removed from running in non-debug kernels.

Signed-off-by Robert Jennings <[email protected]>

---
drivers/staging/zram/xvmalloc.c | 4 +++-

 1 files changed, 3 insertions(+), 1 deletions(-)

drivers/staging/zram/xvmalloc.c +0 -1

index b3622f1..172514e 100644
@@ -219,7 +219,6 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
  static void remove_block_head(struct xv_pool *pool,
                        struct block_header *block, u32 slindex)
  {
-       struct block_header *tmpblock;
        u32 flindex = slindex / BITS_PER_LONG;
 
        pool->freelist[slindex].page = block->link.next_page;

drivers/staging/zram/xvmalloc.c static void remove_block_head(struct xv_pool *pool, · line 234

Quoted 4 lines

drivers/staging/zram/xvmalloc.c +1 -0

@@ -232,6 +231,8 @@ static void remove_block_head(struct xv_pool *pool,
                if (!pool->slbitmap[flindex])
                        __clear_bit(flindex, &pool->flbitmap);
        } else {
+#ifdef DEBUG

Shouldn't that be something people can enable from Kconfig?

Quoted 21 lines

drivers/staging/zram/xvmalloc.c +2 -1

+               struct block_header *tmpblock;
                /*
                 * DEBUG ONLY: We need not reinitialize freelist head previous
                 * pointer to 0 - we never depend on its value. But just for

@@ -242,6 +243,7 @@ static void remove_block_head(struct xv_pool *pool,
                tmpblock->link.prev_page = 0;
                tmpblock->link.prev_offset = 0;
                put_ptr_atomic(tmpblock, KM_USER1);
+#endif
        }
  }
 
--

1.6.0.2

Robert Jennings

author

2011-01-26 18:27 UTC #18 lore raw

Re: [PATCH 5/7] zram/xvmalloc: wrap debug code in #ifdef DEBUG

Quoted 20 lines · * Pekka Enberg ([email protected]) wrote:

Quoted 19 lines · Robert Jennings
This wraps the code, noted by comments as being debug code, with #ifdef DEBUG so that it is removed from running in non-debug kernels.
Signed-off-by Robert Jennings <[email protected]>
---
drivers/staging/zram/xvmalloc.c | 4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/staging/zram/xvmalloc.c +0 -1
index b3622f1..172514e 100644
@@ -219,7 +219,6 @@ static void insert_block(struct xv_pool *pool, struct page *page, u32 offset,
  static void remove_block_head(struct xv_pool *pool,
                        struct block_header *block, u32 slindex)
  {
-       struct block_header *tmpblock;
        u32 flindex = slindex / BITS_PER_LONG;
 

Quoted 8 lines

drivers/staging/zram/xvmalloc.c static void remove_block_head(struct xv_pool *pool, · line 234

Quoted 6 lines

drivers/staging/zram/xvmalloc.c +1 -0

        pool->freelist[slindex].page = block->link.next_page;

@@ -232,6 +231,8 @@ static void remove_block_head(struct xv_pool *pool,
                if (!pool->slbitmap[flindex])
                        __clear_bit(flindex, &pool->flbitmap);
        } else {
+#ifdef DEBUG

Shouldn't that be something people can enable from Kconfig?

It should be, I'll correct this and re-post.

Robert Jennings

author

2011-01-26 17:29 UTC #19 lore raw

PATCH 6/7 zram/xvmalloc: Close 32byte hole on 64-bit CPUs

In reply to Robert Jennings

By swapping the total_pages statistic with the lock we close a hole in the structure for 64-bit CPUs.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/xvmalloc_int.h |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

drivers/staging/zram/xvmalloc_int.h +2 -5

index 68db384..e51eea4 100644
@@ -93,12 +93,9 @@ struct block_header {
 struct xv_pool {
 	ulong flbitmap;
 	ulong slbitmap[MAX_FLI];
-	spinlock_t lock;
-
+	u64 total_pages;	/* stats */
 	struct freelist_entry freelist[NUM_FREE_LISTS];
-
-	/* stats */
-	u64 total_pages;
+	spinlock_t lock;
 };
 
 #endif

Signature · 1 line

1.6.0.2

Pekka Enberg

Reviewed-by on v1 6/7

2011-01-26 17:43 UTC #20 lore raw

Re: [PATCH 6/7] zram/xvmalloc: Close 32byte hole on 64-bit CPUs

Quoted 4 lines · Robert Jennings

By swapping the total_pages statistic with the lock we close a hole in the structure for 64-bit CPUs.
Signed-off-by Robert Jennings <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Robert Jennings

author

2011-01-26 17:27 UTC #21 lore raw

PATCH 7/7 zram: mark the disk as non-rotating media

In reply to Robert Jennings

Adding QUEUE_FLAG_NONROT for the in the request_queue flags. When used as a swap device, scan_swap_map will avoid the extra work of optimizing for rotating media.

Signed-off-by Robert Jennings <[email protected]>

---
 drivers/staging/zram/zram_drv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/staging/zram/zram_drv.c +1 -0

index d5e0275..450c618 100644
@@ -593,6 +593,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out;
 	}
 
+	zram->queue->queue_flags ^= QUEUE_FLAG_NONROT;
 	blk_queue_make_request(zram->queue, zram_make_request);
 	zram->queue->queuedata = zram;

Signature · 1 line

1.6.0.2

Pekka Enberg

Reviewed-by on v1 7/7

2011-01-26 17:45 UTC #22 lore raw

Re: [PATCH 7/7] zram: mark the disk as non-rotating media

Quoted 5 lines · Robert Jennings

Adding QUEUE_FLAG_NONROT for the in the request_queue flags. When used as a swap device, scan_swap_map will avoid the extra work of optimizing for rotating media.
Signed-off-by Robert Jennings <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Dan Carpenter

2011-01-26 17:48 UTC #23 lore raw

Re: [PATCH 7/7] zram: mark the disk as non-rotating media

In reply to Robert Jennings

Quoted 1 line · Robert Jennings

Adding QUEUE_FLAG_NONROT for the in the request_queue flags. When used

                           ^^^^^^^^^^^^^^

"to the" perhaps?

Quoted 13 lines

as a swap device, scan_swap_map will avoid the extra work of optimizing for rotating media.
Signed-off-by Robert Jennings <[email protected]>
---
 drivers/staging/zram/zram_drv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/staging/zram/zram_drv.c +0 -0
index d5e0275..450c618 100644

drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 596

Quoted 4 lines

drivers/staging/zram/zram_drv.c +1 -0

@@ -593,6 +593,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out;
 	}
 
+	zram->queue->queue_flags ^= QUEUE_FLAG_NONROT;

QUEUE_FLAG_NONROT is 14. We want to set the 14 bit. This won't work.

Use queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->queue);

regards,
dan carpenter

Robert Jennings

author

2011-01-26 17:53 UTC #24 lore raw

Re: [PATCH 7/7] zram: mark the disk as non-rotating media

In reply to Robert Jennings

Quoted 4 lines · * Dan Carpenter ([email protected]) wrote:

Quoted 1 line · Robert Jennings
Adding QUEUE_FLAG_NONROT for the in the request_queue flags. When used
                           ^^^^^^^^^^^^^^
"to the" perhaps?

Yeah, that was edited a few times and never re-read. Thanks.

Quoted 16 lines

Quoted 16 lines
as a swap device, scan_swap_map will avoid the extra work of optimizing for rotating media.
Signed-off-by Robert Jennings <[email protected]>
---
 drivers/staging/zram/zram_drv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/staging/zram/zram_drv.c +0 -0
index d5e0275..450c618 100644
@@ -593,6 +593,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out;
 	}
 

Quoted 5 lines

drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 596
Quoted 1 line
drivers/staging/zram/zram_drv.c +1 -0
+	zram->queue->queue_flags ^= QUEUE_FLAG_NONROT;
QUEUE_FLAG_NONROT is 14. We want to set the 14 bit. This won't work.
Use queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->queue);

I'll correct this. Thank you.

-Rob Jennings

Robert Jennings

author

2011-01-26 18:26 UTC #25 lore raw

Re: [PATCH 7/7] zram: mark the disk as non-rotating media

Quoted 29 lines · * Robert Jennings ([email protected]) wrote:

Quoted 4 lines · * Dan Carpenter ([email protected]) wrote:
Quoted 1 line · Robert Jennings
Adding QUEUE_FLAG_NONROT for the in the request_queue flags. When used
                           ^^^^^^^^^^^^^^
"to the" perhaps?
Yeah, that was edited a few times and never re-read. Thanks.
Quoted 21 lines
Quoted 13 lines
as a swap device, scan_swap_map will avoid the extra work of optimizing for rotating media.
Signed-off-by Robert Jennings <[email protected]>
---
 drivers/staging/zram/zram_drv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/staging/zram/zram_drv.c +0 -0
index d5e0275..450c618 100644
drivers/staging/zram/zram_drv.c static int create_device(struct zram *zram, int device_id) · line 596
Quoted 4 lines
drivers/staging/zram/zram_drv.c +1 -0
@@ -593,6 +593,7 @@ static int create_device(struct zram *zram, int device_id)
 		goto out;
 	}
 
+	zram->queue->queue_flags ^= QUEUE_FLAG_NONROT;
QUEUE_FLAG_NONROT is 14. We want to set the 14 bit. This won't work.
Use queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->queue);

I need to drop this patch entirely. QUEUE_FLAG_NONROT is set in zram_init_device and is unnecessary here.