Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support

[Herbert Xu]

On Thu, Feb 04, 2016 at 11:50:46AM +0800, Li, Weigang wrote:
>
> 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?

What I mean is that the bottom half of the scomp patches can still
be used.  We just want to hide it away from the users so won't
provide the direct entry points such as crypto_alloc_scomp, etc..

Cheers,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Herbert Xu]

On Thu, Feb 04, 2016 at 04:17:41PM +0900, Joonsoo Kim wrote:
>
> Do you think not to merge scomp? Please let me know your overall
> plan about this.?

I'm fine with a driver-side scomp interface.  But I'd rather
avoid having yet another user-side compression interface in the
form of scomp if we can avoid it.

Cheers,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Herbert Xu]

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,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Herbert Xu]

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?

Thanks!
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Li, Weigang]

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

[Li, Weigang]

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

[Joonsoo Kim]

2016-02-04 12:28 GMT+09:00 Herbert Xu :
> 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?
>

Do you think not to merge scomp? Please let me know your overall
plan about this.?

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

[Joonsoo Kim]

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

Re: [PATCH v2 04/10] crypto/compress: add asynchronous compression support

[Herbert Xu]

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.

Cheers,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Joonsoo Kim]

Hello, Herbert.

On Wed, Jan 27, 2016 at 04:09:26PM +0800, 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?

Hmm... I'm not an expert on this area so below of my analysis would be
wrong.

Some of compression example in kernel do compression with PAGE_SIZE and
compressed size is naturally less than PAGE_SIZE. There are many cases
that compressed size is below than 100 bytes. To keep and handle data,
they somtimes use kmalloced buffer and I guess it isn't suitable for
SG-based interface. Is it okay to use SG-based interface
if kmalloced object covers two pages?

And, even, someone uses vmalloced buffer that's also not suitable for
SG-based interface. For large amount data case, vmalloced buffer is
more suitable and it needs pointer interface.

> 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.

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
to get the best performance so it has two Kconfig options. One of
them cannot be support in the general layer. Not supporting pointer
based APIs unavoidably causes regression to zram in this case.

And, S/W compression algorithms that exists in kernel
are pointer based so it's natural to support it first
in crypto compression. That will help existing users to change
their direct library call to crypto compression without any regression.
They may be happy to change it because they just could get more
algorithm support without any loss.

I think that supporting pointer-based interface has some merits
mentioned above. However, I'm not sure what's the benefit if we only
support SG-based interface and it's bigger than above.

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

[Herbert Xu]

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,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Li, Weigang]

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

[Herbert Xu]

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?

Cheers,
-- 
Email: Herbert Xu 
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 v2 04/10] crypto/compress: add asynchronous compression support

[Li, Weigang]

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 

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 
Signed-off-by: Joonsoo Kim 


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