Re: [PATCH v3 3/4] zram: get rid of lockdep warning

From: Minchan Kim
Date: Mon Jan 21 2013 - 18:29:19 EST

Next message: Sedat Dilek: "Re: jbd2: don't wake kjournald unnecessarily"
Previous message: Matt Sealey: "Re: One of these things (CONFIG_HZ) is not like the others.."
In reply to: Jerome Marchand: "Re: [PATCH v3 3/4] zram: get rid of lockdep warning"
Next in thread: Jerome Marchand: "Re: [PATCH v3 1/4] zram: force disksize setting before using zram"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jan 21, 2013 at 08:47:28PM +0100, Jerome Marchand wrote:
> On 01/21/2013 06:21 AM, Minchan Kim wrote:
> > Lockdep complains about recursive deadlock of zram->init_lock.
> > [1] made it false positive because we can't request IO to zram
> > before setting disksize. Anyway, we should shut lockdep up to
> > avoid many reporting from user.
> >
> > Cc: Jerome Marchand <jmarchan@xxxxxxxxxx>
> > Cc: Nitin Gupta <ngupta@xxxxxxxxxx>
> > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > ---
> > drivers/staging/zram/zram_drv.c | 115 +++++++++++++++++++------------------
> > drivers/staging/zram/zram_drv.h | 12 +++-
> > drivers/staging/zram/zram_sysfs.c | 10 +++-
> > 3 files changed, 79 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> > index e95e37c..1f6938a 100644
> > --- a/drivers/staging/zram/zram_drv.c
> > +++ b/drivers/staging/zram/zram_drv.c
> > @@ -462,19 +462,12 @@ error:
> > void __zram_reset_device(struct zram *zram)
> > {
> > size_t index;
> > + struct zram_meta meta;
> >
> > if (!zram->init_done)
> > goto out;
> >
> > zram->init_done = 0;
> > -
> > - /* Free various per-device buffers */
> > - kfree(zram->compress_workmem);
> > - free_pages((unsigned long)zram->compress_buffer, 1);
> > -
> > - zram->compress_workmem = NULL;
> > - zram->compress_buffer = NULL;
> > -
> > /* Free all pages that are still in this zram device */
> > for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
> > unsigned long handle = zram->table[index].handle;
> > @@ -484,11 +477,11 @@ void __zram_reset_device(struct zram *zram)
> > zs_free(zram->mem_pool, handle);
> > }
> >
> > - vfree(zram->table);
> > - zram->table = NULL;
> > -
> > - zs_destroy_pool(zram->mem_pool);
> > - zram->mem_pool = NULL;
> > + meta.compress_workmem = zram->compress_workmem;
> > + meta.compress_buffer = zram->compress_buffer;
> > + meta.table = zram->table;
> > + meta.mem_pool = zram->mem_pool;
> > + zram_meta_free(&meta);
> >
> > /* Reset stats */
> > memset(&zram->stats, 0, sizeof(zram->stats));
> > @@ -505,12 +498,59 @@ void zram_reset_device(struct zram *zram)
> > up_write(&zram->init_lock);
> > }
> >
> > -/* zram->init_lock should be held */
> > -int zram_init_device(struct zram *zram)
> > +void zram_meta_free(struct zram_meta *meta)
> > +{
> > + zs_destroy_pool(meta->mem_pool);
> > + kfree(meta->compress_workmem);
> > + free_pages((unsigned long)meta->compress_buffer, 1);
> > + vfree(meta->table);
> > + kfree(meta);
>
> At all callsite, meta is a local variable. We don't want to free it.

Argh, It was error from previous version which uses zram_meta dynamically.
I will filx in next spin.

>
> > +}
> > +
> > +int zram_meta_alloc(struct zram_meta *meta, u64 disksize)
> > {
> > - int ret;
> > size_t num_pages;
> >
> > + meta->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > + if (!meta->compress_workmem) {
> > + pr_err("Error allocating compressor working memory!\n");
> > + goto out;
> > + }
> > +
> > + meta->compress_buffer =
> > + (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, 1);
> > + if (!meta->compress_buffer) {
> > + pr_err("Error allocating compressor buffer space\n");
> > + goto free_workmem;
> > + }
> > +
> > + num_pages = disksize >> PAGE_SHIFT;
> > + meta->table = vzalloc(num_pages * sizeof(*meta->table));
> > + if (!meta->table) {
> > + pr_err("Error allocating zram address table\n");
> > + goto free_buffer;
> > + }
> > +
> > + meta->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
> > + if (!meta->mem_pool) {
> > + pr_err("Error creating memory pool\n");
> > + goto free_table;
> > + }
> > +
> > + return 0;
> > +
> > +free_table:
> > + vfree(meta->table);
> > +free_buffer:
> > + free_pages((unsigned long)meta->compress_buffer, 1);
> > +free_workmem:
> > + kfree(meta->compress_workmem);
> > +out:
> > + return -ENOMEM;
> > +}
> > +
> > +void zram_init_device(struct zram *zram, struct zram_meta *meta)
> > +{
> > if (zram->disksize > 2 * (totalram_pages << PAGE_SHIFT)) {
> > pr_info(
> > "There is little point creating a zram of greater than "
> > @@ -525,51 +565,16 @@ int zram_init_device(struct zram *zram)
> > );
> > }
> >
> > - zram->compress_workmem = kzalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
> > - if (!zram->compress_workmem) {
> > - pr_err("Error allocating compressor working memory!\n");
> > - ret = -ENOMEM;
> > - goto fail_no_table;
> > - }
> > -
> > - zram->compress_buffer =
> > - (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
> > - if (!zram->compress_buffer) {
> > - pr_err("Error allocating compressor buffer space\n");
> > - ret = -ENOMEM;
> > - goto fail_no_table;
> > - }
> > -
> > - num_pages = zram->disksize >> PAGE_SHIFT;
> > - zram->table = vzalloc(num_pages * sizeof(*zram->table));
> > - if (!zram->table) {
> > - pr_err("Error allocating zram address table\n");
> > - ret = -ENOMEM;
> > - goto fail_no_table;
> > - }
> > -
> > /* zram devices sort of resembles non-rotational disks */
> > queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
> >
> > - zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
> > - if (!zram->mem_pool) {
> > - pr_err("Error creating memory pool\n");
> > - ret = -ENOMEM;
> > - goto fail;
> > - }
> > -
> > + zram->mem_pool = meta->mem_pool;
> > + zram->compress_workmem = meta->compress_workmem;
> > + zram->compress_buffer = meta->compress_buffer;
> > + zram->table = meta->table;
> > zram->init_done = 1;
> >
> > pr_debug("Initialization done!\n");
> > - return 0;
> > -
> > -fail_no_table:
> > - /* To prevent accessing table entries during cleanup */
> > - zram->disksize = 0;
> > -fail:
> > - __zram_reset_device(zram);
>
> Here goes the last call to __zram_reset_device() outside of zram_reset_device().
> I guess we should get rid of it.

Yeb. Will include it.
Thanks for the review!

>
> Jerome
>
> > - pr_err("Initialization failed: err=%d\n", ret);
> > - return ret;
> > }
> >
> > static void zram_slot_free_notify(struct block_device *bdev,
> > diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
> > index 5b671d1..fbe7db0 100644
> > --- a/drivers/staging/zram/zram_drv.h
> > +++ b/drivers/staging/zram/zram_drv.h
> > @@ -83,11 +83,19 @@ struct zram_stats {
> > u32 bad_compress; /* % of pages with compression ratio>=75% */
> > };
> >
> > +struct zram_meta {
> > + void *compress_workmem;
> > + void *compress_buffer;
> > + struct table *table;
> > + struct zs_pool *mem_pool;
> > +};
> > +
> > struct zram {
> > struct zs_pool *mem_pool;
> > void *compress_workmem;
> > void *compress_buffer;
> > struct table *table;
> > +
> > spinlock_t stat64_lock; /* protect 64-bit stats */
> > struct rw_semaphore lock; /* protect compression buffers and table
> > * against concurrent read and writes */
> > @@ -111,7 +119,9 @@ unsigned int zram_get_num_devices(void);
> > extern struct attribute_group zram_disk_attr_group;
> > #endif
> >
> > -extern int zram_init_device(struct zram *zram);
> > extern void zram_reset_device(struct zram *zram);
> > +extern int zram_meta_alloc(struct zram_meta *meta, u64 disksize);
> > +extern void zram_meta_free(struct zram_meta *meta);
> > +extern void zram_init_device(struct zram *zram, struct zram_meta *meta);
> >
> > #endif
> > diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
> > index 369db12..31433ef 100644
> > --- a/drivers/staging/zram/zram_sysfs.c
> > +++ b/drivers/staging/zram/zram_sysfs.c
> > @@ -56,22 +56,28 @@ static ssize_t disksize_store(struct device *dev,
> > struct device_attribute *attr, const char *buf, size_t len)
> > {
> > u64 disksize;
> > + struct zram_meta meta;
> > struct zram *zram = dev_to_zram(dev);
> >
> > disksize = memparse(buf, NULL);
> > if (!disksize)
> > return -EINVAL;
> >
> > + disksize = PAGE_ALIGN(disksize);
> > + if (zram_meta_alloc(&meta, disksize))
> > + return -ENOMEM;
> > +
> > down_write(&zram->init_lock);
> > if (zram->init_done) {
> > up_write(&zram->init_lock);
> > + zram_meta_free(&meta);
> > pr_info("Cannot change disksize for initialized device\n");
> > return -EBUSY;
> > }
> >
> > - zram->disksize = PAGE_ALIGN(disksize);
> > + zram->disksize = disksize;
> > set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> > - zram_init_device(zram);
> > + zram_init_device(zram, &meta);
> > up_write(&zram->init_lock);
> >
> > return len;
> >
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>

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

Next message: Sedat Dilek: "Re: jbd2: don't wake kjournald unnecessarily"
Previous message: Matt Sealey: "Re: One of these things (CONFIG_HZ) is not like the others.."
In reply to: Jerome Marchand: "Re: [PATCH v3 3/4] zram: get rid of lockdep warning"
Next in thread: Jerome Marchand: "Re: [PATCH v3 1/4] zram: force disksize setting before using zram"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]