[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: PR/36864 CVS commit: src/sys/opencrypto
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, 07 Mar 2011 10:27:11 +0100
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
> To: gnats-bugs%NetBSD.org@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:
> 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.
> 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
> 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
> 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.
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 |