Re: PR/36864 CVS commit: src/sys/opencrypto

[Wolfgang Stukenbrock <Wolfgang.Stukenbrock%nagler-company.com@localhost>]

Hi,

I've had a look at it.

In general it is a good idea - see conclusion below.

Let's do some calculations ...

I've compressed /dev/zero with gzip -9 - result:
   size     output
    1024 ->   29  -> factor 35
   10240 ->   45  -> factor 227
  102400 ->  133  -> factor 769
 1024000 -> 1026  -> factor 998
10240000 -> 9981  -> factor 1025

remark: 1024000000 -> 995308 -> factor 1028, it looks like we won't get 
much better factors at all.

ZBUF is 10 at the moment, so we get the following size factors,
aggregated size factors and buffer sizes (assumed last is 1000000)

   4 -    4 -    1950
   8 -   12 -    3901
  16 -   28 -    7812
  32 -   60 -   15625
  64 -  124 -   31250
 128 -  292 -   62500
 256 -  548 -  125000
 512 - 1060 -  230000
1024 - 2084 -  500000
2048 - 4132 - 1000000 -> max buffer 1978038 (near 2 MB)

remark: If we will never get much better factor than 1030 at all, ZBUF = 
8 would be large enougth. My test above seems to point to that.

There are two use-cases for the opencrypto stuff - as far as I know:
1. IPSEC
2. /dev/crypto

Let's look at 1 first:
The reasonable max. size for network packets for common network types is 
1500 or 9k for jumbo frames.
So lets assume 10k. This will lead to a max factor of 227 in compressing 
only 0 bytes. This mean, you have 45 bytes of input size, resulting in a 
180 byte start buffer. In order to do the decompression you need 6 
allocations (180+360+720+1440+2880+5760 = 11340).
This is much better than 10240/180 = 57 buffer allocations.

In my PR, I've mentioned a minimum start size for the algorithm. I've 
fixed it to 2k with the idea that a power of two is a good idea and all 
normal network packets will fit into one buffer when decompressed.
Till now I think that it would be a good idea to create a sysctl 
variable that defines a minimum start buffer size that should be used if 
4*size is smaller. The variable should have a default value of 0 - no 
minimum size.
This allows the admin to set it to e.g. 1500 (or 9k if jumbo frames are 
used for compressed traffic) so that "normal" network packets will fit 
into one buffer regardless of the compression factor.
For 10k pakets 1500 will lead to only 3 allocations (1500, 3000, 6000)
I think this would be a even better performance boost for IPSEC, because 
if we can get everything into one buffer, we avoid copying everything 
again at end of decompression ...


Let's look at 2:
In general we cannot assume anything about the size that gets compressed 
or decompressed.
The limit of 1000000 Bytes max. chunk size may be a very hard thing 
here. (Sorry, I do not use /dev/crypto, so I don't know anything about 
realy reasonable sizes.)
In general, I think for /dev/crypto it would be better to allow any size 
for compression/decompression and do the copyin/copyout in parts. (There 
must be a reason why all compression lib interfaces are designed like 
this ...)
Again, I do not know if this would break some semanticts of the 
interface for /dev/crypto with overlapping in-/out-buffers.
And I've no Idea if all hardware devices would be able to handle this.
Perhaps there is already an interface mode for /dev/crypto that allows 
partial work and continuations - I've never looked for this up to now.
remark: That may conflict with the change done some PR's before by 
setting Z_FINISH in compression - that needs to get "tunable" to support 
this ...

The current Patch will limit decompression to something a little bit 
less 2MB. This may be OK, but I would be surprised if it is, because 2MB 
is not very much for user-level stuff.


Conclusion:
For IPSEC this change is a great step into the right direction.
But I recommend adding a sysctl variable with a minimum start size too.
Due to the fact that I do not use /dev/crypto, I cannot decide if the 
limit to ca. 2MB in decompression is OK or not.


Last, some ideas from my side for /dev/crypto:

For /dev/crypto usage I think that the internal interfaces of opencrypto 
are not optimal.
The aproach to get everything into some "objects" and operate on them, 
creating new objects, sounds like a commonly used aproach in object 
oriented programming.
This aproach is not optimal for data-stream processing, because it 
assumes that we have endless memory if we are going to process larger 
amounts of data - it will not scale. It is no good idea to do this on 
user level and inside the kernel such an aproach has lost nothing - my 
oppion.
The current patch will speed up /dev/crypto work, so it is a step into 
the right direction for /dev/crypto usage too.
In the interface of /dev/crypto the user needs to reserve enougth 
user-level memory to copy back everything.
So the user-process knows something about the expected size of the 
thing. (Or makes a dummy call just in order to get the required size 
information without any data transfer.)
Perhaps this can be used to do some even better heuristics when 
allocating buffer space. If there are some limit-checks at /dev/crypto 
syscall level, the size used there should be used here too. (At least a 
comment that references theese values is missing here.)
Perhaps a max result size parameter makes sence in the calling interface 
of deflate/gzip-routines, to avoid useless decompression that cannot be 
copied back to user level due to size reasons. Or the "rest" gets 
decompressed into a static garbage buffer in order get the result-size, 
if required.
The "best sollution" would be (from my point of view), if the whole 
internal interface get changed, so that partial data transfer may 
directly happen from/to the user level and the result-size gets 
allocated (and expanded if needed) by the caller.
Of cause this would break interchangeability with other implementations 
and I'm not shure it this realy make sense.
I also simply do not know what kind of "productive" user-level programs 
are "designed" to use compresion via /dev/crypto.
All network data stream related things should be inside of the kernel - 
my oppinion - for performance reasons. Sorry, I'm no friend of placeing 
kernel functionality (mainly data stream or network packet processing) 
in user-level programs.
(racoon (IKE) and IPSEC is a good example here: (fast) data-stream 
processing inside the kernel - key-exchange, access validation and other 
complex things in a user-level program that configures the kernel IPSEC 
parts.)
For testing and development of something /dev/crypto might be a very 
nice thing, of cause.

Best regards

W. Stukenbrock


Matthias Drochner wrote:

> The following reply was made to PR kern/36864; it has been noted by GNATS.
>
> From: "Matthias Drochner" <drochner%netbsd.org@localhost>
> To: gnats-bugs%gnats.NetBSD.org@localhost
> Cc: 
> Subject: PR/36864 CVS commit: src/sys/opencrypto
> Date: Fri, 18 Feb 2011 22:02:10 +0000
>
>  Module Name:   src
>  Committed By:  drochner
>  Date:          Fri Feb 18 22:02:09 UTC 2011
>  
>  Modified Files:
>         src/sys/opencrypto: deflate.c
>  
>  Log Message:
>  redo result buffer allocation, to avoid dynamic allocations:
>  -use exponentially growing buffer sizes instead of just linear extension
>  -drop the dynamic allocation of buffer metadata introduced in rev.1.8 --
>   if the initial array is not sufficient something is wrong
>  -apply some (arbitrary, heuristic) limit so that compressed data
>   which extract into insane amounts of constant data don't kill the system
>  This addresses PR kern/36864 by Wolfgang Stukenbrock. Some tuning
>  might be useful, but hopefully this is an improvement already.
>  
>  
>  To generate a diff of this commit:
>  cvs rdiff -u -r1.17 -r1.18 src/sys/opencrypto/deflate.c
>  
>  Please note that diffs are not public domain; they are subject to the
>  copyright notices on the relevant files.
>