Re: [PATCH v3 8/9] zram: use crypto API for compression

[Joonsoo Kim <iamjoonsoo.kim@lge.com>]

On Mon, Sep 21, 2015 at 02:19:03PM +0900, Sergey Senozhatsky wrote:
> On (09/18/15 14:19), Joonsoo Kim wrote:
> [..]
> > -static struct zcomp_backend *find_backend(const char *compress)
> > +static const char *find_backend(const char *compress)
> >  {
> >  	int i = 0;
> >  	while (backends[i]) {
> > -		if (sysfs_streq(compress, backends[i]->name))
> > +		if (sysfs_streq(compress, backends[i]) &&
> > +			crypto_has_comp(compress, 0, 0))
> 
> ok, just for note. zcomp does sysfs_streq(), because sysfs data
> usually contain a trailing new line, crypto_has_comp() does strcmp().

Okay. Will check.

> 
> 
> >  int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm,
> > -		const unsigned char *src, size_t *dst_len)
> > +		const unsigned char *src, unsigned int *dst_len)
> >  {
> > -	return comp->backend->compress(src, zstrm->buffer, dst_len,
> > -			zstrm->private);
> > +	*dst_len = PAGE_SIZE << 1;
> > +
> 
> hm... wouldn't it be better to say crypto api that we have a PAGE_SIZE
> buffer instead of PAGE_SIZE << 1, so in case of buffer overrun (or
> whatever is the correct term here) it will stop compression earlier
> (well, possibly)? zram will drop compressed data larger than PAGE_SIZE
> anyway, so if passing a smaller buffer size can save us CPU time then
> let's do it.

It can be implemented and maybe good way to go. But, in this patchset,
it isn't needed. It is better to do in separate patch.

> 
> > +	return crypto_comp_compress(zstrm->tfm, src, PAGE_SIZE,
> > +					zstrm->buffer, dst_len);
> >  }
> >  
> >  int zcomp_decompress(struct zcomp *comp, struct zcomp_strm *zstrm,
> >  		const unsigned char *src,
> > -		size_t src_len, unsigned char *dst)
> > +		unsigned int src_len, unsigned char *dst)
> >  {
> > -	return comp->backend->decompress(src, src_len, dst);
> > +	unsigned int size = PAGE_SIZE;
> > +
> > +	return crypto_comp_decompress(zstrm->tfm, src, src_len,	dst, &size);
> 
> 							^^^^^^^^ tab?
> 
> >  }
> >  
> >  void zcomp_destroy(struct zcomp *comp)
> > @@ -359,7 +365,7 @@ void zcomp_destroy(struct zcomp *comp)
> >  struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  {
> >  	struct zcomp *comp;
> > -	struct zcomp_backend *backend;
> > +	const char *backend;
> 
> rebase against the current linux-next. this and the next patch do not
> apply cleanly. the function was touched recently: '+int error'.

Okay.

> 
> >  
> >  	backend = find_backend(compress);
> >  	if (!backend)
> > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> > index 4c09c01..4f9df8e 100644
> > --- a/drivers/block/zram/zcomp.h
> > +++ b/drivers/block/zram/zcomp.h
> > @@ -11,38 +11,22 @@
> >  #define _ZCOMP_H_
> >  
> >  #include <linux/mutex.h>
> > +#include <linux/crypto.h>
> >  
> >  struct zcomp_strm {
> > +	struct crypto_comp *tfm;
> > +
> >  	/* compression/decompression buffer */
> >  	void *buffer;
> > -	/*
> > -	 * The private data of the compression stream, only compression
> > -	 * stream backend can touch this (e.g. compression algorithm
> > -	 * working memory)
> > -	 */
> > -	void *private;
> > +
> >  	/* used in multi stream backend, protected by backend strm_lock */
> >  	struct list_head list;
> >  };
> >  
> > -/* static compression backend */
> > -struct zcomp_backend {
> > -	int (*compress)(const unsigned char *src, unsigned char *dst,
> > -			size_t *dst_len, void *private);
> > -
> > -	int (*decompress)(const unsigned char *src, size_t src_len,
> > -			unsigned char *dst);
> > -
> > -	void *(*create)(void);
> > -	void (*destroy)(void *private);
> > -
> > -	const char *name;
> > -};
> > -
> >  /* dynamic per-device compression frontend */
> >  struct zcomp {
> >  	void *stream;
> > -	struct zcomp_backend *backend;
> > +	const char *backend;
> 
> 	^^ what for?

Will remove.

Thanks.