Re: [PATCH 1/3] zram: make max_zpage_size configurable

[Sergey Senozhatsky]

On (09/15/15 09:18), Vitaly Wool wrote:
>    > On (09/14/15 15:50), Vitaly Wool wrote:
>    > > It makes sense to have control over what compression ratios are
>    > > ok to store pages uncompressed and what not.
>    >
>    > um... I don't want this to be exported. this is very 'zram-internal'.
>    >
>    > besides, you remove the exsiting default value
>    > - static const size_t max_zpage_size = PAGE_SIZE / 4 * 3;
>    >
>    > so now people must provide this module param in order to make zram
>    > work the way it used to work for years?
>    >
> 
>    I do not remove it, I move it :) May I ask to review the patch again
>    please?

Oh, yep, missed that.

Please read Dan Streetman's reply. I agree with what he said/suggested.


And can you please switch your email client to 'plain text' mode only?
haven't checked, but I suspect that some of vger lists block !plain_text
emails. ... apart from the fact that it's hard to read such emails in
mutt, etc.

	-ss

>    >
>    > > Moreover, if we end up using zbud allocator for zram, any attempt to
>    > > allocate a whole page will fail, so we may want to avoid this as much
>    > > as possible.
>    >
>    > so how does it help?
>    >
>    > > So, let's have max_zpage_size configurable as a module parameter.
>    > >
>    > > Signed-off-by: Vitaly Wool <[2]vitalywool@xxxxxxxxx>
>    > > ---
>    > >  drivers/block/zram/zram_drv.c | 13 +++++++++++++
>    > >  drivers/block/zram/zram_drv.h | 16 ----------------
>    > >  2 files changed, 13 insertions(+), 16 deletions(-)
>    > >
>    > > diff --git a/drivers/block/zram/zram_drv.c
>    b/drivers/block/zram/zram_drv.c
>    > > index 9fa15bb..6d9f1d1 100644
>    > > --- a/drivers/block/zram/zram_drv.c
>    > > +++ b/drivers/block/zram/zram_drv.c
>    > > @@ -42,6 +42,7 @@ static const char *default_compressor = "lzo";
>    > >
>    > >  /* Module params (documentation at end) */
>    > >  static unsigned int num_devices = 1;
>    > > +static size_t max_zpage_size = PAGE_SIZE / 4 * 3;
>    > >
>    > >  static inline void deprecated_attr_warn(const char *name)
>    > >  {
>    > > @@ -1411,6 +1412,16 @@ static int __init zram_init(void)
>    > >               return ret;
>    > >       }
>    > >
>    > > +     /*
>    > > +      * max_zpage_size must be less than or equal to:
>    > > +      * ZS_MAX_ALLOC_SIZE. Otherwise, zs_malloc() would
>    > > +      * always return failure.
>    > > +      */
>    > > +     if (max_zpage_size > PAGE_SIZE) {
>    > > +             pr_err("Invalid max_zpage_size %ld\n", max_zpage_size);
>    >
>    > and how do people find out ZS_MAX_ALLOC_SIZE? this error message does
>    not
>    > help.
>    >
>    > > +             return -EINVAL;
>    > > +     }
>    > > +
>    > >       zram_major = register_blkdev(0, "zram");
>    > >       if (zram_major <= 0) {
>    > >               pr_err("Unable to get major number\n");
>    > > @@ -1444,6 +1455,8 @@ module_exit(zram_exit);
>    > >
>    > >  module_param(num_devices, uint, 0);
>    > >  MODULE_PARM_DESC(num_devices, "Number of pre-created zram devices");
>    > > +module_param(max_zpage_size, ulong, 0);
>    > > +MODULE_PARM_DESC(max_zpage_size, "Threshold for storing compressed
>    pages");
>    >
>    > unclear description.
>    >
>    >
>    > >
>    > >  MODULE_LICENSE("Dual BSD/GPL");
>    > >  MODULE_AUTHOR("Nitin Gupta <[3]ngupta@xxxxxxxxxx>");
>    > > diff --git a/drivers/block/zram/zram_drv.h
>    b/drivers/block/zram/zram_drv.h
>    > > index 8e92339..3a29c33 100644
>    > > --- a/drivers/block/zram/zram_drv.h
>    > > +++ b/drivers/block/zram/zram_drv.h
>    > > @@ -20,22 +20,6 @@
>    > >
>    > >  #include "zcomp.h"
>    > >
>    > > -/*-- Configurable parameters */
>    > > -
>    > > -/*
>    > > - * Pages that compress to size greater than this are stored
>    > > - * uncompressed in memory.
>    > > - */
>    > > -static const size_t max_zpage_size = PAGE_SIZE / 4 * 3;
>    > > -
>    > > -/*
>    > > - * NOTE: max_zpage_size must be less than or equal to:
>    > > - *   ZS_MAX_ALLOC_SIZE. Otherwise, zs_malloc() would
>    > > - * always return failure.
>    > > - */
>    > > -
>    > > -/*-- End of configurable params */
>    > > -
>    > >  #define SECTOR_SHIFT         9
>    > >  #define SECTORS_PER_PAGE_SHIFT       (PAGE_SHIFT - SECTOR_SHIFT)
>    > >  #define SECTORS_PER_PAGE     (1 << SECTORS_PER_PAGE_SHIFT)
>    > > --
>    > > 1.9.1
>    > >
>    > > --
>    > > To unsubscribe, send a message with 'unsubscribe linux-mm' in
>    > > the body to [4]majordomo@xxxxxxxxxx  For more info on Linux MM,
>    > > see: [5]http://www.linux-mm.org/ .
>    > > Don't email: <a href=mailto:"[6]dont@xxxxxxxxx";> [7]email@xxxxxxxxx
>    </a>
>    > >
> 
> References
> 
>    Visible links
>    1. mailto:sergey.senozhatsky.work@xxxxxxxxx
>    2. mailto:vitalywool@xxxxxxxxx
>    3. mailto:ngupta@xxxxxxxxxx
>    4. mailto:majordomo@xxxxxxxxx
>    5. http://www.linux-mm.org/
>    6. mailto:dont@xxxxxxxxx
>    7. mailto:email@xxxxxxxxx
--
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/