| 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
| ||
|
Message-ID: <20140224230756.GA24325@bbox>
Date: Tue, 25 Feb 2014 08:07:56 +0900
From: Minchan Kim <minchan@...nel.org>
To: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc: Jerome Marchand <jmarchan@...hat.com>,
Nitin Gupta <ngupta@...are.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv6 3/6] zram: factor out single stream compression
Hello Sergey,
On Mon, Feb 24, 2014 at 11:31:52AM +0300, Sergey Senozhatsky wrote:
> Hello Minchan,
>
> thanks for your review.
>
> On (02/24/14 11:31), Minchan Kim wrote:
> > Hello Sergey,
> >
> > On Fri, Feb 21, 2014 at 02:50:40PM +0300, Sergey Senozhatsky wrote:
> > > This is preparation patch to add multi stream support to zcomp.
> > >
> > > Introduce struct zcomp_strm_single and a set of functions to manage zcomp_strm
> > > stream access. zcomp_strm_single implements single compession stream, same way
> > > as current zcomp implementation. This moves zcomp_strm stream control and
> > > locking from zcomp, so compressing backend zcomp is not aware of required
> > > locking (single and multi streams require different locking schemes).
> > >
> > > The following set of functions added:
> > > - zcomp_strm_single_get()/zcomp_strm_single_put()
> > > get and put compression stream, implement required locking
> > > - zcomp_strm_single_create()/zcomp_strm_single_destroy()
> > > create and destroy zcomp_strm_single
> > >
> > > New ->strm_get() and ->strm_put() callbacks added to zcomp, which are set to
> > > zcomp_strm_single_get() and zcomp_strm_single_put() during initialisation.
> > > Instead of direct locking and zcomp_strm access from zcomp_strm_get() and
> > > zcomp_strm_put(), zcomp now calls ->strm_get() and ->strm_put()
> > > correspondingly.
> > >
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> >
> > It's actually not what I expect.
> > What I want was to separate implementation to different files
> > whether it enalbles CONFIG_ZRAM_ZCOMP_MULTI or not so that
> > popular users who want to use zram as only swap with small
> > memory system have little side effect about performance and
> > code size.
>
> am I right to guess that you multi stream implementation replaces single
> stream. in other words, CONFIG_ZRAM_ZCOMP_MULTI turns zcomp into just a
> multi stream backend?
>
> the reasoning behind this indirection is that it allows us to have
> CONFIG_ZRAM_ZCOMP_MULTI as additional functionality. if user selects
> CONFIG_ZRAM_ZCOMP_MULTI then there is a possibility for user to have both
> single (e.g. if he uses zram as a swap device) and multi implemetation
> (e.g. if he also uses it as a compressed block device with fs) on his
> system. in other words, user may create N zram devices: one swap device
> (with single stream inplementation) and N-1 multi stream.
>
> so CONFIG_ZRAM_ZCOMP_MULTI is additional functionality, not the replacing
> one. otherwise, there is a small foot print (IMHO. just several function
> pointers, other than that it's just a single stream mutex-based implementation).
> sounds sane?
Sounds good to me. That was from my laziness that I just didn't read your
entire patchset. Then, I think we don't need to separate it with new CONFIG
option(and I know you wanted ;-) ) because it just increase small memory
footprint but it could remove maintainace headache and confusing from zram
users.
add/remove: 4/0 grow/shrink: 2/0 up/down: 772/0 (772)
function old new delta
zcomp_strm_multi_get - 189 +189
max_comp_streams_store 14 155 +141
zcomp_strm_alloc - 127 +127
zcomp_create 313 439 +126
zcomp_strm_multi_put - 95 +95
zcomp_strm_multi_destroy - 94 +94
So, let's remove new CONFIG and go with only option but it would work
with mutex if stream is only one so there would be no regression but
a little code size overhead but it's good deal, IMO.
I will review other patches in v6.
Thanks.
>
> >
> > > ---
> > > drivers/block/zram/zcomp.c | 63 ++++++++++++++++++++++++++++++++++++++++------
> > > drivers/block/zram/zcomp.h | 7 ++++--
> > > 2 files changed, 60 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > > index db72f3d..9661226 100644
> > > --- a/drivers/block/zram/zcomp.c
> > > +++ b/drivers/block/zram/zcomp.c
> > > @@ -15,6 +15,14 @@
> > >
> > > #include "zcomp.h"
> > >
> > > +/*
> > > + * single zcomp_strm backend private part
> > > + */
> > > +struct zcomp_strm_single {
> > > + struct mutex strm_lock;
> > > + struct zcomp_strm *zstrm;
> > > +};
> > > +
> > > extern struct zcomp_backend zcomp_lzo;
> > >
> > > static struct zcomp_backend *find_backend(const char *compress)
> > > @@ -55,17 +63,58 @@ static struct zcomp_strm *zcomp_strm_alloc(struct zcomp *comp)
> > > return zstrm;
> > > }
> > >
> > > +static struct zcomp_strm *zcomp_strm_single_get(struct zcomp *comp)
> > > +{
> > > + struct zcomp_strm_single *zp = comp->private;
> > > + mutex_lock(&zp->strm_lock);
> > > + return zp->zstrm;
> > > +}
> > > +
> > > +static void zcomp_strm_single_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > > +{
> > > + struct zcomp_strm_single *zp = comp->private;
> > > + mutex_unlock(&zp->strm_lock);
> > > +}
> > > +
> > > +static void zcomp_strm_single_destroy(struct zcomp *comp)
> > > +{
> > > + struct zcomp_strm_single *zp = comp->private;
> > > + zcomp_strm_free(comp, zp->zstrm);
> > > + kfree(zp);
> > > +}
> > > +
> > > +static int zcomp_strm_single_create(struct zcomp *comp)
> > > +{
> > > + struct zcomp_strm_single *zp;
> > > +
> > > + comp->destroy = zcomp_strm_single_destroy;
> > > + comp->strm_get = zcomp_strm_single_get;
> > > + comp->strm_put = zcomp_strm_single_put;
> > > + zp = kmalloc(sizeof(struct zcomp_strm_single), GFP_KERNEL);
> > > + comp->private = zp;
> > > + if (!zp)
> > > + return -ENOMEM;
> > > +
> > > + mutex_init(&zp->strm_lock);
> > > + zp->zstrm = zcomp_strm_alloc(comp);
> > > + if (!zp->zstrm) {
> > > + zcomp_strm_single_destroy(comp);
> > > + return -ENOMEM;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > struct zcomp_strm *zcomp_strm_get(struct zcomp *comp)
> > > {
> > > - mutex_lock(&comp->strm_lock);
> > > - return comp->zstrm;
> > > + return comp->strm_get(comp);
> > > }
> > >
> > > void zcomp_strm_put(struct zcomp *comp, struct zcomp_strm *zstrm)
> > > {
> > > - mutex_unlock(&comp->strm_lock);
> > > + comp->strm_put(comp, zstrm);
> > > }
> > >
> > > +/* compress page */
> > > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > > const unsigned char *src, size_t *dst_len)
> > > {
> > > @@ -73,6 +122,7 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > > zstrm->private);
> > > }
> > >
> > > +/* decompress page */
> > > int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > > size_t src_len, unsigned char *dst)
> > > {
> > > @@ -81,7 +131,7 @@ int zcomp_decompress(struct zcomp *comp, const unsigned char *src,
> > >
> > > void zcomp_destroy(struct zcomp *comp)
> > > {
> > > - zcomp_strm_free(comp, comp->zstrm);
> > > + comp->destroy(comp);
> > > kfree(comp);
> > > }
> > >
> > > @@ -105,10 +155,7 @@ struct zcomp *zcomp_create(const char *compress)
> > > return NULL;
> > >
> > > comp->backend = backend;
> > > - mutex_init(&comp->strm_lock);
> > > -
> > > - comp->zstrm = zcomp_strm_alloc(comp);
> > > - if (!comp->zstrm) {
> > > + if (zcomp_strm_single_create(comp) != 0) {
> > > zcomp_destroy(comp);
> > > return NULL;
> > > }
> > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > > index 5106f8e..8dc1d7f 100644
> > > --- a/drivers/block/zram/zcomp.h
> > > +++ b/drivers/block/zram/zcomp.h
> > > @@ -34,9 +34,12 @@ struct zcomp_backend {
> > >
> > > /* dynamic per-device compression frontend */
> > > struct zcomp {
> > > - struct mutex strm_lock;
> > > - struct zcomp_strm *zstrm;
> > > + void *private;
> > > struct zcomp_backend *backend;
> > > +
> > > + struct zcomp_strm *(*strm_get)(struct zcomp *comp);
> > > + void (*strm_put)(struct zcomp *comp, struct zcomp_strm *zstrm);
> > > + void (*destroy)(struct zcomp *comp);
> >
> > I don't think we need indirection for get/put/destroy.
> > zram_drv.c just calls zcomp_strm_get and zcomp.c could implement it
> >
> > zcomp_strm_get()
> > {
> > mutex_lock
> > return strm;
> > }
> >
> > and zcomp_multi.c can do it
> >
> > zcomp_strm_get()
> > {
> > spin_lock
> > spin_unlock
> > wait_event
> > return strm;
> > }
>
> so we have only one option -- it either only single stream based zram or
> only multi stream based zram. I can move in this direction.
>
> my implemtation allowed two options:
>
> -- single stream zram
> or
> -- (CONFIG_ZRAM_ZCOMP_MULTI selected) single stream and multi stream,
> depending of user set max_comp_streams.
>
> > It seems that you live in my opposite country(ie, you start to dump patches
> > when I am about leaving office so ping-pong gap of patch is at least
> > one day round. It makes us collaboration very hard so eaieist method I can
> > think is just I can implement my thought by myself but I don't want it.
> > You thought this idea firstly and I want that you have all credit although
> > it waste our time)
> >
> > If I made you annoying, I'm really sorry to you.
> > Again, thanks for looking at this, Sergey!
> >
>
> I really appreciate and value all your input and review. Thank you. And
> sorry if it consumes a lot of your time.
>
> -ss
>
> > --
> > Kind regards,
> > Minchan Kim
> >
> --
> 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/
--
Kind regards,
Minchan Kim
--
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/
Powered by blists - more mailing lists