Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

On 2017/2/7 10:54, Minchan Kim wrote:
> On Tue, Feb 07, 2017 at 10:20:57AM +0800, zhouxianrong wrote:
>
> < snip >
>
> > > > 3. the below should be modified.
> > > >
> > > > static inline bool zram_meta_get(struct zram *zram)
> > > > @@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> > > >
> > > > 	/* Free all pages that are still in this zram device */
> > > > 	for (index = 0; index < num_pages; index++) {
> > > > -		unsigned long handle = meta->table[index].handle;
> > > > +		unsigned long handle;
> > > > +
> > > > +		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
> > > > +		handle = meta->table[index].handle;
> > > >
> > > > -		if (!handle)
> > > > +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
> > > > +			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> > > > 			continue;
> > > > +		}
> > > >
> > > > +		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> > > > 		zs_free(meta->mem_pool, handle);
> > >
> > > Could you explain why we need this modification?
> > >
> > > > 	}
> > > >
> > > > @@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> > > > static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
> > > > {
> > > > 	size_t num_pages;
> > > > -	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> > > > +	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> > >
> > > Ditto
> > >
> > > >
> > >
> > > .
> >
> > because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing.
> > if zram_meta_get was modified, zram_meta_alloc did so.
>
> Right. Thanks but I don't see why we need the locking in there and modification of
> zram_meta_alloc.
>
> Isn't it enough with this?

i am afraid someone do reset_store, so did lock.

yes, i am wrong, zram_meta_alloc should not be modified here. because meta->table has already cleared

	meta->table = vzalloc(num_pages * sizeof(*meta->table));




> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index c20b05a84f21..a25d34a8af19 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -425,8 +425,11 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>  	/* Free all pages that are still in this zram device */
>  	for (index = 0; index < num_pages; index++) {
>  		unsigned long handle = meta->table[index].handle;
> -
> -		if (!handle)
> +		/*
> +		 * No memory is allocated for same element filled pages.
> +		 * Simply clear same page flag.
> +		 */
> +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME))
>  			continue;
>
>  		zs_free(meta->mem_pool, handle);
>
> .