[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: PR/36864 CVS commit: src/sys/opencrypto
due to the fact that this reports is still in feedback-mode, I got a
reminder mail again.
The last look into the actual implementation shows that there is now a
hint for the size is passed from the caller to the decompression stuff
(the o_len field is used for that).
This makes an additional sysctl-variable - as mentioned from my side
before - obsolete. The use of MCLBYTES is a good idea and avoids any
additional allocation and/or copy operation for normal MTU sizes. For
Jumbo frames only additional allocations are required on very good
(better than factor 4) compression.
So from the view of IPSEC I think this PR can be closed.
I had no look the /dev/crypto part again. So I cannot decide if this PR
can be closed from the view of /dev/crypto too.
But in any case, it should be changed from feedback to something else.
Wolfgang Stukenbrock wrote:
The following reply was made to PR kern/36864; it has been noted by GNATS.
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock%nagler-company.com@localhost>
Cc: kern-bug-people%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost,
Subject: Re: PR/36864 CVS commit: src/sys/opencrypto
Date: Mon, 21 Feb 2011 15:49:31 +0100
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:
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:
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
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
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
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
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.
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
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
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,
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
For testing and development of something /dev/crypto might be a very
nice thing, of cause.
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
> 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.
Dr. Nagler & Company GmbH
Tel. +49 9622/71 97-42
Fax +49 9622/71 97-50
Handelregister: Amberg HRB
USt.-ID-Nummer: DE 273143997
Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze
Main Index |
Thread Index |