linux-kernel - Re: [PATCH 2/2] zram: reduce metadata overhead

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50B4E8AF.7050808@redhat.com>
Date:	Tue, 27 Nov 2012 17:22:07 +0100
From:	Jerome Marchand <jmarchan@...hat.com>
To:	Nitin Gupta <ngupta@...are.org>
CC:	Greg KH <greg@...ah.com>,
	Seth Jennings <sjenning@...ux.vnet.ibm.com>,
	Minchan Kim <minchan.kim@...il.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	Sam Hansen <solid.se7en@...il.com>, Tomas M <tomas@...x.org>,
	Mihail Kasadjikov <hamer.mk@...il.com>,
	Linux Driver Project <devel@...uxdriverproject.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] zram: reduce metadata overhead

On 11/27/2012 08:26 AM, Nitin Gupta wrote:
> For every allocated object, zram maintains the the handle, size,
> flags and count fields. Of these, only the handle is required
> since zsmalloc now provides the object size given the handle.
> The flags field was needed only to mark a given page as zero-filled.
> Instead of this field, we now use an invalid value (-1) to mark such

Since the handle is unsigned, is this really an invalid value?

> pages. Lastly, the count field was unused, so was simply removed.
> 
> Signed-off-by: Nitin Gupta <ngupta@...are.org>
> ---
>  drivers/staging/zram/zram_drv.c |   80 ++++++++++++++-------------------------
>  drivers/staging/zram/zram_drv.h |   18 +--------
>  2 files changed, 31 insertions(+), 67 deletions(-)
> 

> [...]

> @@ -222,8 +202,8 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>  
>  	page = bvec->bv_page;
>  
> -	if (unlikely(!zram->table[index].handle) ||
> -			zram_test_flag(zram, index, ZRAM_ZERO)) {
> +	if (unlikely(!zram->handle[index]) ||
> +			(zram->handle[index] == zero_page_handle)) {

This kind of comparison appears often. To my taste, an is_zero_page()
helper would be welcome.

>  		handle_zero_page(bvec);
>  		return 0;
>  	}

> [...]

> @@ -573,8 +551,8 @@ int zram_init_device(struct zram *zram)
>  	}
>  
>  	num_pages = zram->disksize >> PAGE_SHIFT;
> -	zram->table = vzalloc(num_pages * sizeof(*zram->table));
> -	if (!zram->table) {
> +	zram->handle = vzalloc(num_pages * sizeof(*zram->handle));
> +	if (!zram->handle) {
>  		pr_err("Error allocating zram address table\n");
>  		ret = -ENOMEM;
>  		goto fail_no_table;

Since the table goes away, for readability sake, we'd better remove all
reference to it in messages, labels and comment and write "handle"
instead.

Otherwise, the patch looks good to me.

Jerome

> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
> index df2eec4..8aa733c 100644
> --- a/drivers/staging/zram/zram_drv.h
> +++ b/drivers/staging/zram/zram_drv.h
> @@ -54,24 +54,10 @@ static const size_t max_zpage_size = PAGE_SIZE / 4 * 3;
>  #define ZRAM_SECTOR_PER_LOGICAL_BLOCK	\
>  	(1 << (ZRAM_LOGICAL_BLOCK_SHIFT - SECTOR_SHIFT))
>  
> -/* Flags for zram pages (table[page_no].flags) */
> -enum zram_pageflags {
> -	/* Page consists entirely of zeros */
> -	ZRAM_ZERO,
> -
> -	__NR_ZRAM_PAGEFLAGS,
> -};
> +static const unsigned long zero_page_handle = (unsigned long)(-1);
>  
>  /*-- Data structures */
>  
> -/* Allocated for each disk page */
> -struct table {
> -	unsigned long handle;
> -	u16 size;	/* object size (excluding header) */
> -	u8 count;	/* object ref count (not yet used) */
> -	u8 flags;
> -} __aligned(4);
> -
>  struct zram_stats {
>  	u64 compr_size;		/* compressed size of pages stored */
>  	u64 num_reads;		/* failed + successful */
> @@ -90,7 +76,7 @@ struct zram {
>  	struct zs_pool *mem_pool;
>  	void *compress_workmem;
>  	void *compress_buffer;
> -	struct table *table;
> +	unsigned long *handle;	/* memory handle for each disk page */
>  	spinlock_t stat64_lock;	/* protect 64-bit stats */
>  	struct rw_semaphore lock; /* protect compression buffers and table
>  				   * against concurrent read and writes */
> 

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