From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Joonsoo Kim <js1304@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Minchan Kim <minchan@kernel.org>, Nitin Gupta <ngupta@vflare.org>,
linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
Herbert Xu <herbert@gondor.apana.org.au>,
"David S. Miller" <davem@davemloft.net>,
Stephan Mueller <smueller@chronox.de>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v2 8/8] zram: use decompress_noctx
Date: Thu, 27 Aug 2015 14:47:14 +0900 [thread overview]
Message-ID: <20150827054714.GA30236@swordfish> (raw)
In-Reply-To: <20150827040712.GF1545@swordfish>
On (08/27/15 13:07), Sergey Senozhatsky wrote:
[..]
> > -struct zcomp_strm *zcomp_strm_find(struct zcomp *comp)
> > +struct zcomp_strm *zcomp_strm_find(struct zcomp *comp, bool decomp)
> >
I think we can hide zcomp_strm_find()/_release (make them static),
and instead introduce:
a) zcomp_decompress_begin()/zcomp_decompress_end()
calls zcomp_strm_find()/zcomp_strm_release() for tfms that
require context for decompression; may return NULL, may sleep.
b) zcomp_compress_begin()/zcomp_compress_end()
always calls zcomp_strm_find()/zcomp_strm_release();
never return NULL; may sleep.
-ss
> > {
> > + /* We don't need decompression context so zstrm neither */
> > + if (decomp && test_bit(ZCOMP_DECOMPRESS_NOCTX, &comp->flags))
> > + return NULL;
> > +
> > return comp->strm_find(comp);
> > }
>
> No. zcomp_strm_find() should never return NULL. And no, I don't like
> "if (decomp)" change.
>
>
> >
> > @@ -307,6 +311,11 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > {
> > *dst_len = PAGE_SIZE << 1;
> >
> > + if (!zstrm) {
> > + return crypto_comp_compress_noctx(comp->alg, src, PAGE_SIZE,
> > + dst, dst_len);
> > + }
> > +
> > return crypto_comp_compress(zstrm->tfm, src, PAGE_SIZE, dst, dst_len);
> > }
> >
> > @@ -316,12 +325,18 @@ int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > {
> > unsigned int size = PAGE_SIZE;
> >
> > + if (!zstrm) {
> > + return crypto_comp_decompress_noctx(comp->alg, src, src_len,
> > + dst, &size);
> > + }
> > +
> > return crypto_comp_decompress(zstrm->tfm, src, src_len, dst, &size);
> > }
> >
> > void zcomp_destroy(struct zcomp *comp)
> > {
> > comp->destroy(comp);
> > + crypto_put_comp(comp->alg);
> > kfree(comp);
> > }
> >
> > @@ -344,12 +359,23 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> > return ERR_PTR(-ENOMEM);
> >
> > comp->name = compress;
> > + comp->alg = crypto_get_comp(compress, 0, 0);
> > + if (!comp->alg) {
> > + kfree(comp);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (crypto_has_compress_noctx(comp->alg))
> > + set_bit(ZCOMP_COMPRESS_NOCTX, &comp->flags);
>
> do you use ZCOMP_COMPRESS_NOCTX algs in this patch set?
>
>
> > + if (crypto_has_decompress_noctx(comp->alg))
> > + set_bit(ZCOMP_DECOMPRESS_NOCTX, &comp->flags);
> >
> > if (max_strm > 1)
> > zcomp_strm_multi_create(comp, max_strm);
> > else
> > zcomp_strm_single_create(comp);
> > if (!comp->stream) {
> > + crypto_put_comp(comp->alg);
> > kfree(comp);
> > return ERR_PTR(-ENOMEM);
> > }
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index c47db4d..6c137c8 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -13,6 +13,11 @@
> > #include <linux/crypto.h>
> > #include <linux/mutex.h>
> >
> > +enum {
> > + ZCOMP_COMPRESS_NOCTX,
> > + ZCOMP_DECOMPRESS_NOCTX,
> > +};
>
> Can it be handled via crypto api? check if ->foo_noctx() is !NULL ?
>
> > struct zcomp_strm {
> > struct crypto_comp *tfm;
> >
> > @@ -27,6 +32,8 @@ struct zcomp_strm {
> > struct zcomp {
> > void *stream;
> > const char *name;
> > + struct crypto_alg *alg;
> > + unsigned long flags;
> >
> > struct zcomp_strm *(*strm_find)(struct zcomp *comp);
> > void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm);
> > @@ -39,7 +46,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf);
> > struct zcomp *zcomp_create(const char *comp, int max_strm);
> > void zcomp_destroy(struct zcomp *comp);
> >
> > -struct zcomp_strm *zcomp_strm_find(struct zcomp *comp);
> > +struct zcomp_strm *zcomp_strm_find(struct zcomp *comp, bool decomp);
> > void zcomp_strm_release(struct zcomp *comp, struct zcomp_strm *zstrm);
> >
> > int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index b3044d3..8283bd3 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -623,7 +623,7 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> > /* Use a temporary buffer to decompress the page */
> > uncmem = kmalloc(PAGE_SIZE, GFP_NOIO);
> >
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, true);
>
> No, I don't like this.
>
> > user_mem = kmap_atomic(page);
> > if (!is_partial_io(bvec))
> > uncmem = user_mem;
> > @@ -647,7 +647,8 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> > ret = 0;
> > out_cleanup:
> > kunmap_atomic(user_mem);
> > - zcomp_strm_release(zram->comp, zstrm);
> > + if (zstrm)
> > + zcomp_strm_release(zram->comp, zstrm);
> > if (is_partial_io(bvec))
> > kfree(uncmem);
> > return ret;
> > @@ -676,14 +677,14 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > ret = -ENOMEM;
> > goto out;
> > }
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, true);
> > ret = zram_decompress_page(zram, zstrm, uncmem, index);
> > if (ret)
> > goto out;
> > }
> >
> > if (!zstrm)
> > - zstrm = zcomp_strm_find(zram->comp);
> > + zstrm = zcomp_strm_find(zram->comp, false);
>
> No. I don't like this.
>
> -ss
>
prev parent reply other threads:[~2015-08-27 5:46 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-20 6:34 [PATCH v2 0/8] zram: introduce crypto compress noctx API and use it on zram Joonsoo Kim
2015-08-20 6:34 ` [PATCH v2 1/8] crypto: support (de)compression API that doesn't require tfm object Joonsoo Kim
2015-08-20 6:47 ` Herbert Xu
2015-08-20 7:52 ` Joonsoo Kim
2015-08-20 7:50 ` Herbert Xu
2015-08-20 8:05 ` Joonsoo Kim
2015-08-20 6:34 ` [PATCH v2 2/8] crypto/lzo: support decompress_noctx Joonsoo Kim
2015-08-27 1:54 ` Sergey Senozhatsky
2015-08-20 6:34 ` [PATCH v2 3/8] crypyo/lz4: " Joonsoo Kim
2015-08-27 1:56 ` Sergey Senozhatsky
2015-08-20 6:35 ` [PATCH v2 4/8] crypto/lz4hc: " Joonsoo Kim
2015-08-27 1:57 ` Sergey Senozhatsky
2015-08-20 6:35 ` [PATCH v2 5/8] crypto/842: " Joonsoo Kim
2015-08-20 6:35 ` [PATCH v2 6/8] zram: change zcomp_compress interface Joonsoo Kim
2015-08-27 2:14 ` Sergey Senozhatsky
2015-08-20 6:35 ` [PATCH v2 7/8] zram: use crypto API for compression Joonsoo Kim
2015-08-27 4:01 ` Sergey Senozhatsky
2015-08-28 12:02 ` [PATCH 0/2] prepare zram for crypto api-powered zcomp Sergey Senozhatsky
2015-08-28 12:02 ` [PATCH 1/2] zram: make stream find and release functions static Sergey Senozhatsky
2015-08-28 12:02 ` [PATCH 2/2] zram: pass zstrm down to decompression path Sergey Senozhatsky
2015-09-01 1:22 ` [PATCH 0/2] prepare zram for crypto api-powered zcomp Minchan Kim
2015-09-01 1:41 ` Sergey Senozhatsky
2015-09-01 2:06 ` Minchan Kim
2015-09-01 2:12 ` Sergey Senozhatsky
2015-09-07 5:32 ` Joonsoo Kim
2015-08-20 6:35 ` [PATCH v2 8/8] zram: use decompress_noctx Joonsoo Kim
2015-08-27 4:07 ` Sergey Senozhatsky
2015-08-27 5:47 ` Sergey Senozhatsky [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150827054714.GA30236@swordfish \
--to=sergey.senozhatsky.work@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=iamjoonsoo.kim@lge.com \
--cc=js1304@gmail.com \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=ngupta@vflare.org \
--cc=smueller@chronox.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).