Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support
On 2/1/2016 10:11 AM, Joonsoo Kim wrote: On Fri, Jan 29, 2016 at 06:09:01PM +0800, Herbert Xu wrote: On Thu, Jan 28, 2016 at 12:19:42PM +0900, Joonsoo Kim wrote: I have tested asynchronous compression APIs in zram and I saw regression. Atomic allocation and setting up SG lists are culprit for this regression. Moreover, zram optimizes linearisation So which is it, atomic allocations or setting up SG lists? There is nothing in acomp that requires you to do an atomic allocation. Atomic allocation are called for linearisation when needed. Zram's compressed content is usually stored in two physically separate pages so linearisation is needed. See scomp_map(). Setting up SG lists means that to use acomp, sg_init_table(), sg_set_page() are need to be called by zram unlike the case just passing the pointer based buffer. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Hello Herbert & Joonsoo, Please can you advise how to get the acomp patch accepted? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support
On 2/4/2016 11:29 AM, Herbert Xu wrote: On Thu, Feb 04, 2016 at 11:28:50AM +0800, Herbert Xu wrote: On Thu, Feb 04, 2016 at 11:25:27AM +0800, Li, Weigang wrote: Please can you advise how to get the acomp patch accepted? Can you do a posting of these patches without scomp so we can evaluate the effects? Of course you can keep the driver-side scomp interface as otherwise the implementation would be unnecessarily complicated. Cheers, Seems I need go back to my first acomp patch.. Assuming we shall still keep the comp i/f, and the linearisation of sg-list in acomp to fit the "comp" API? What do you mean by the driver-side scomp? Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support
On 1/27/2016 4:09 PM, Herbert Xu wrote: On Wed, Jan 27, 2016 at 04:03:55PM +0800, Herbert Xu wrote: On Wed, Jan 27, 2016 at 03:59:05PM +0800, Li, Weigang wrote: The acomp is also SG-based, while scomp only accepts flat buffer. Right, but do we need a pointer-based scomp at all? IPComp would certainly be better off with an SG-based interface. Any other users of compression are presumably dealing with large amounts of data where an SG interface would make more sense. A pointer interface makes sense for shash because you may be hashing 16 bytes at a time. Nobody sane is going to be compressing 16 bytes, or are they? Note that I'm fine with keeping an scomp interface underneath for those algorithms where the best way to handle SG input is to linearise things. But I would prefer that this interface is not exposed to kernel users unless it is absolutely required. Cheers, Thanks for your comments, Herbert. I Agree, SG-list based compression API makes more sense. Maybe Joonsoo can comment on this. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support
On 1/27/2016 3:41 PM, Herbert Xu wrote: On Tue, Jan 26, 2016 at 05:15:06PM +0900, Joonsoo Kim wrote: From: Weigang Li <weigang...@intel.com> Now, asynchronous compression APIs are supported. There is no asynchronous compression driver now but this APIs can be used as front-end to synchronous compression algorithm. In this case, scatterlist would be linearlized when needed so it would cause some overhead. Signed-off-by: Weigang Li <weigang...@intel.com> Signed-off-by: Joonsoo Kim <iamjoonsoo@lge.com> I think we should be able to use this for the synchronous case too, like we do with skcipher and ahash. The main difference that I can see right now is that acomp always allocates a context through the request object while scomp does not. This difference is entirely artificial as we could also make the context conditional for acomp. The reason we had the shash/ahash division is because the shash interface offers a direct pointer interface while ahash is SG-based. Otherwise ahash is just as able as shash to handle synchronous requests. At this point in time I don't see such a fundamental distinction between acomp and scomp. Cheers, The acomp is also SG-based, while scomp only accepts flat buffer. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] crypto: add asynchronous compression support
On 11/20/2015 2:19 PM, Herbert Xu wrote: On Fri, Nov 20, 2015 at 03:04:47PM +0900, Joonsoo Kim wrote: Linearization would be enough to use sg-list but it has a problem. Linearization needs sleepable function such as vmap() and it makes sync (de)compression in atomic context impossible. Currently, zram did sync (de)compression in atomic context. It uses map_vm_area() which isn't sleepable function to linearize two separate pages. This is possible because zram already knows that maximum number of spread pages is just two and have allocated vm area in advance. But, if we implement linearization in general API, we can't be sure of request input size so we need sleepable function, vmap(). And, this sleep could degrade performance. Obviously you would only perform linearisation where it's needed. And if you are in an atomic context, then clearly linearisation can only be attempted using kmalloc/alloc_pages with GFP_ATOMIC. I don't understand your concern with zram because either zram is already feeding us linear buffers in which case linearisation is never needed. Or it can only be used with algorithms that support SG lists or partial updates, which we can easily mark using a flag. Intermediate buffer size could vary greatly so it would be allocated and Freed whenever requested. This could affect performance. That's for the crypto user to figure out. Either they should supply a completely linear buffer if they want to be able to support every algorithm in an efficient manner, or they will either have to eat the cost of linearisation or only use algorithms that can deal with SG lists efficiently. We have the same problem with network drivers and it's dealt with in exactly the same way. An skb can be an SG list and will be linearised when necessary. I think that supporting unified API has more loss than gain. I disagree. I have seen no valid reason so far for adding two compression interfaces. Cheers, Thanks Herbert. If we assume the sg-list can be linearized - no "holes" in the sg-list, all chunks in middle of the list are of PAGE_SIZE, it seems ok to support sg-list in the s/w implementation, linearize the sg-list and compress it as one chunk. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] crypto: add asynchronous compression support
On 10/16/2015 11:13 PM, Herbert Xu wrote: > On Fri, Oct 16, 2015 at 11:11:00PM +0800, Weigang Li wrote: >> This patch set introduces Asynchronous Compression API. >> What is proposed is a new crypto type called crypto_acomp_type, >> plus new struct acomp_alg and struct crypto_acomp, together with number >> of helper functions to register acomp type algorithms and allocate tfm >> instances. This is to make it similar to how the existing crypto API >> works for the ablkcipher, and akcipher types. >> The operations the new interface will provide are: >> >> int (*compress)(struct acompress_request *req); >> int (*decompress)(struct acompress_request *req); >> >> The benefits it gives interface are: >> - the new interface allows for asynchronous implementations and >>scatterlist buffer that can use hardware to offload the compression >>operations, the new asynchronous API can be called by the linux kernel >>components (i.e., btrfs) who want to use hardware acceleration for data >>compression. >> >> New helper functions have been added to allocate crypto_acomp instances >> and invoke the operations to make it easier to use. >> >> Signed-off-by: Weigang Li <weigang...@intel.com> > > Thanks for the patch! Joonsoo Kim is also working on the compression > interface for zram. Could you two collaborate and come up with one > interface rather than two? > > Cheers, > Hello Herbert, After sync-up with Joonsoo Kim, we think it may be not feasible for a s/w implementation of the sg-list based asynchronous interface, we propose separate interfaces (patches) for acomp & ccomp. The reasons are: 1. to support sg-list in the ccomp (like what shash/ahash did), the partial update is required, some algorithms do not support partial update (i.e., lzo), that means: 2. the format of output buffer (sg-list) will be different, e.g., the lzo need contain the "length" info for each block in the output sg-list in order to de-compression, while zlib doesn't need, then it is difficult to have a single async sg-list i/f. 3. to compress a sg-list buffer, the lzo also requires an intermediate buffer to save the output of a block, and copy it back to the sg-list output buffer, it will introduce the complexity and cost, we don't see value for sg-list support in a s/w compression. Any suggestions? -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] crypto: add asynchronous compression support
After sync with Joonsoo Kim offline, he agreed to merge this acomp patch with his ccomp patch, thanks Joonsoo! -Original Message- From: Herbert Xu [mailto:herb...@gondor.apana.org.au] Sent: Friday, October 16, 2015 11:14 PM To: Li, Weigang Cc: linux-crypto@vger.kernel.org; Struk, Tadeusz; Joonsoo Kim; Sergey Senozhatsky Subject: Re: [PATCH] crypto: add asynchronous compression support On Fri, Oct 16, 2015 at 11:11:00PM +0800, Weigang Li wrote: > This patch set introduces Asynchronous Compression API. > What is proposed is a new crypto type called crypto_acomp_type, plus > new struct acomp_alg and struct crypto_acomp, together with number of > helper functions to register acomp type algorithms and allocate tfm > instances. This is to make it similar to how the existing crypto API > works for the ablkcipher, and akcipher types. > The operations the new interface will provide are: > > int (*compress)(struct acompress_request *req); > int (*decompress)(struct acompress_request *req); > > The benefits it gives interface are: > - the new interface allows for asynchronous implementations and > scatterlist buffer that can use hardware to offload the compression > operations, the new asynchronous API can be called by the linux kernel > components (i.e., btrfs) who want to use hardware acceleration for data > compression. > > New helper functions have been added to allocate crypto_acomp > instances and invoke the operations to make it easier to use. > > Signed-off-by: Weigang Li <weigang...@intel.com> Thanks for the patch! Joonsoo Kim is also working on the compression interface for zram. Could you two collaborate and come up with one interface rather than two? Cheers, -- Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] crypto: add asynchronous compression support
> -Original Message- > From: Herbert Xu [mailto:herb...@gondor.apana.org.au] > Sent: Wednesday, October 21, 2015 3:34 PM > To: Sergey Senozhatsky > Cc: Minchan Kim; Joonsoo Kim; Dan Streetman; Seth Jennings; Li, Weigang; > Struk, Tadeusz > Subject: Re: [PATCH] crypto: add asynchronous compression support > > On Wed, Oct 21, 2015 at 04:33:22PM +0900, Sergey Senozhatsky wrote: > > > > the thing is -- I still want to have/use SW compressors; and they [the > > existing S/W algorithms] want virtual addresses. so we need to > > kmap_atomic pages in every SW algorithm? isn't it simpler to keep > > kmap_atomic_to_page around? > > The Crypto API will do it for you. Have a look at how we handle it for aead > and ahash for example. > > Cheers, > -- > Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page: > http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt Adding the "linux-crypto" list back to the latest discussion. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
