NetBSD Problem Report #36865

From Wolfgang.Stukenbrock@nagler-company.com  Thu Aug 30 10:50:10 2007
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 1259163B8B5
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 30 Aug 2007 10:50:10 +0000 (UTC)
Message-Id: <200708301050.l7UAo8vQ029755@test-s0.nagler-company.com>
Date: Thu, 30 Aug 2007 12:50:08 +0200 (CEST)
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@NetBSD.org
Subject: opencrypto - deflate wastes time with useless data copying
X-Send-Pr-Version: 3.95

>Number:         36865
>Category:       kern
>Synopsis:       opencrypto - deflate wastes time with useless data copying
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Thu Aug 30 10:55:00 +0000 2007
>Closed-Date:    Mon Feb 21 21:53:51 +0000 2011
>Last-Modified:  Mon Feb 21 21:53:51 +0000 2011
>Originator:     Wolfgang Stukenbrock
>Release:        NetBSD 3.1
>Organization:
Dr. Nagler & company GmbH

>Environment:


System: NetBSD test-s0 3.1 NetBSD 3.1 (test-s0) #0: Tue Apr 3 11:33:43 CEST 2007 root@test-s0:/usr/src/sys/arch/i386/compile/test-s0 i386
Architecture: i386
Machine: i386
>Description:
	In /usr/src/sys/opencrypto/deflate.c there will be some buffers be
	allocated during operation.
	After operation is done, a new buffer is allocated and all resulting
	data is copyed into that one, even if there is only one result buffer.
	It would be faster to return the result-buffer directly if there
	is only on present.
	This routine is only used in crptosoft.c via xform.c from function
	swcr_compdec() and the returned buffer is freed there after moving
	the data out of it.
	A second subject to be enhanced is the structure component "flag" of
	struct deflate_buf.
	It is used to detect, if there is valid information in thsi structure
	in order to call FREE() only for valid information.
	The same checking can be done by using the variable i, that is used
	to determine the next insertion point in case of buffer expansion.
	The struct deflate_buf is only used inside of this file.
>How-To-Repeat:
	not relevant
>Fix:
	The follwoing patch will remove the structure component "flag" and
	avoid useless copy operations.
	remark: this patch assunes that patch from 36864 has not been applied yet!

*** deflate.h   2007/08/30 09:51:30     1.1
--- deflate.h   2007/08/30 09:52:06
***************
*** 51,57 ****
  struct deflate_buf {
  	u_int8_t *out;
  	u_int32_t size;
- 	int flag;
  };

  #endif /* _CRYPTO_DEFLATE_H_ */
--- 51,56 ----


*** deflate.c	2007/08/30 09:51:30	1.1
--- deflate.c	2007/08/30 10:47:16
***************
*** 81,92 ****
  	z_stream zbuf;
  	u_int8_t *output;
  	u_int32_t count, result;
! 	int error, i = 0, j;
  	struct deflate_buf buf[ZBUF];

  	bzero(&zbuf, sizeof(z_stream));
- 	for (j = 0; j < ZBUF; j++)
- 		buf[j].flag = 0;

  	zbuf.next_in = data;	/* data that is going to be processed */
  	zbuf.zalloc = ocf_zalloc;
--- 81,90 ----
  	z_stream zbuf;
  	u_int8_t *output;
  	u_int32_t count, result;
! 	int error, i, j;
  	struct deflate_buf buf[ZBUF];

  	bzero(&zbuf, sizeof(z_stream));

  	zbuf.next_in = data;	/* data that is going to be processed */
  	zbuf.zalloc = ocf_zalloc;
***************
*** 95,107 ****
  	zbuf.avail_in = size;	/* Total length of data to be processed */

  	if (!decomp) {
! 		MALLOC(buf[i].out, u_int8_t *, (u_long) size, M_CRYPTO_DATA,
! 		    M_NOWAIT);
! 		if (buf[i].out == NULL)
! 			goto bad;
! 		buf[i].size = size;
! 		buf[i].flag = 1;
! 		i++;
  	} else {
  		/*
  	 	 * Choose a buffer with 4x the size of the input buffer
--- 93,99 ----
  	zbuf.avail_in = size;	/* Total length of data to be processed */

  	if (!decomp) {
! 		buf[0].size = size;
  	} else {
  		/*
  	 	 * Choose a buffer with 4x the size of the input buffer
***************
*** 110,123 ****
  	 	 * updated while the decompression is going on
  	 	 */

! 		MALLOC(buf[i].out, u_int8_t *, (u_long) (size * 4),
! 		    M_CRYPTO_DATA, M_NOWAIT);
! 		if (buf[i].out == NULL)
! 			goto bad;
! 		buf[i].size = size * 4;
! 		buf[i].flag = 1;
! 		i++;
  	}

  	zbuf.next_out = buf[0].out;
  	zbuf.avail_out = buf[0].size;
--- 102,113 ----
  	 	 * updated while the decompression is going on
  	 	 */

! 		buf[0].size = size * 4;
  	}
+ 	MALLOC(buf[0].out, u_int8_t *, (u_long) buf[0].size, M_CRYPTO_DATA, M_NOWAIT);
+ 	if (buf[0].out == NULL)
+ 		goto bad;
+ 	i = 1;

  	zbuf.next_out = buf[0].out;
  	zbuf.avail_out = buf[0].size;
***************
*** 135,141 ****
  			goto bad;
  		else if (zbuf.avail_in == 0 && zbuf.avail_out != 0)
  			goto end;
! 		else if (zbuf.avail_out == 0 && i < (ZBUF - 1)) {
  			/* we need more output space, allocate size */
  			MALLOC(buf[i].out, u_int8_t *, (u_long) size,
  			    M_CRYPTO_DATA, M_NOWAIT);
--- 125,131 ----
  			goto bad;
  		else if (zbuf.avail_in == 0 && zbuf.avail_out != 0)
  			goto end;
! 		else if (zbuf.avail_out == 0 && i < ZBUF) {
  			/* we need more output space, allocate size */
  			MALLOC(buf[i].out, u_int8_t *, (u_long) size,
  			    M_CRYPTO_DATA, M_NOWAIT);
***************
*** 143,149 ****
  				goto bad;
  			zbuf.next_out = buf[i].out;
  			buf[i].size = size;
- 			buf[i].flag = 1;
  			zbuf.avail_out = buf[i].size;
  			i++;
  		} else
--- 133,138 ----
***************
*** 153,186 ****
  end:
  	result = count = zbuf.total_out;

- 	MALLOC(*out, u_int8_t *, (u_long) result, M_CRYPTO_DATA, M_NOWAIT);
- 	if (*out == NULL)
- 		goto bad;
  	if (decomp)
  		inflateEnd(&zbuf);
  	else
  		deflateEnd(&zbuf);
! 	output = *out;
! 	for (j = 0; buf[j].flag != 0; j++) {
! 		if (count > buf[j].size) {
! 			bcopy(buf[j].out, *out, buf[j].size);
! 			*out += buf[j].size;
! 			FREE(buf[j].out, M_CRYPTO_DATA);
! 			count -= buf[j].size;
! 		} else {
! 			/* it should be the last buffer */
! 			bcopy(buf[j].out, *out, count);
! 			*out += count;
! 			FREE(buf[j].out, M_CRYPTO_DATA);
! 			count = 0;
  		}
  	}
- 	*out = output;
  	return result;

  bad:
  	*out = NULL;
! 	for (j = 0; buf[j].flag != 0; j++)
  		FREE(buf[j].out, M_CRYPTO_DATA);
  	if (decomp)
  		inflateEnd(&zbuf);
--- 142,181 ----
  end:
  	result = count = zbuf.total_out;

  	if (decomp)
  		inflateEnd(&zbuf);
  	else
  		deflateEnd(&zbuf);
! 
! 	if (i != 1) { /* copy everything into one buffer */
! 		MALLOC(*out, u_int8_t *, (u_long) result, M_CRYPTO_DATA, M_NOWAIT);
! 		if (*out == NULL)
! 			goto bad;
! 		output = *out;
! 		for (j = 0; j < i; j++) {
! 			if (count > buf[j].size) {
! 				bcopy(buf[j].out, output, buf[j].size);
! 				output += buf[j].size;
! 				FREE(buf[j].out, M_CRYPTO_DATA);
! 				count -= buf[j].size;
! 			} else {
! 				/* it should be the last buffer */
! 				bcopy(buf[j].out, output, count);
! 				output += count;
! 				FREE(buf[j].out, M_CRYPTO_DATA);
! 				count = 0;
! 			}
  		}
+ 	} else { /* only one result buffer present - just return that one
+ 		  * do not wast time in with malloc/free and copying things around
+ 		  */
+ 		*out = buf[0].out;
  	}
  	return result;

  bad:
  	*out = NULL;
! 	for (j = 0; j < i; j++)
  		FREE(buf[j].out, M_CRYPTO_DATA);
  	if (decomp)
  		inflateEnd(&zbuf);

>Release-Note:

>Audit-Trail:
From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/36865: opencrypto - deflate wastes time with useless data copying
Date: Thu, 30 Aug 2007 13:04:35 +0200

 Hi,

 my patch is missing the initialisation of variable i = 0 in line 84.

 sorry - my fault.
 The last cleanup while generating the patch info was buggy ..

 Without this initialisation the patch will not compile.

 W. Stukenbrock

 gnats-admin@NetBSD.org wrote:

 > Thank you very much for your problem report.
 > It has the internal identification `kern/36865'.
 > The individual assigned to look at your
 > report is: kern-bug-people. 
 > 
 > 
 >>Category:       kern
 >>Responsible:    kern-bug-people
 >>Synopsis:       opencrypto - deflate wastes time with useless data copying
 >>Arrival-Date:   Thu Aug 30 10:55:00 +0000 2007
 >>
 > 


State-Changed-From-To: open->closed
State-Changed-By: drochner@NetBSD.org
State-Changed-When: Wed, 16 Feb 2011 19:10:24 +0000
State-Changed-Why:
is the code in -current OK?


From: "Matthias Drochner" <drochner@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/36865 CVS commit: src/sys/opencrypto
Date: Wed, 16 Feb 2011 19:08:59 +0000

 Module Name:	src
 Committed By:	drochner
 Date:		Wed Feb 16 19:08:58 UTC 2011

 Modified Files:
 	src/sys/opencrypto: deflate.c deflate.h

 Log Message:
 -avoid allocation of an extra result buffer and data copy in case
  the DEFLATE complssion/decompression result is within a single
  buffer already
 -simplify bookkeeping of allocated buffers (and don't waste the
  last member of the metadata array)
 from Wolfgang Stukenbrock per PR kern/36865 (with some cleanup
 of error handling by me)
 The Gzip compression case can be improved too, but for now I've applied
 the buffer bookkeeping changes.

 tested with IP4 IPCOMP


 To generate a diff of this commit:
 cvs rdiff -u -r1.14 -r1.15 src/sys/opencrypto/deflate.c
 cvs rdiff -u -r1.6 -r1.7 src/sys/opencrypto/deflate.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: closed->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 20 Feb 2011 00:38:24 +0000
State-Changed-Why:
Since you asked a question I think you meant feedback, not closed :-)


From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org,
        dholland@NetBSD.org, Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: kern/36865 (opencrypto - deflate wastes time with useless data copying)
Date: Mon, 21 Feb 2011 12:26:54 +0100

 Hi,

 I've a look at the patch.

 It looks OK. (The deflate part is running here fine, gzip only validated 
 by a look at the source, because I've disabled the gzip part here 
 completely - see PR44210 and I don't need gzip here.)

 But perhaps it would be a good idea to keep the defalte and gzip method 
 as common as possible.
 Therefore I would have used a "bad3" label in the gzip part too and 
 avoid the "i=0" setup in the declaration.

 Then I'm no friend of calling memcpy() for 0 bytes - as done in both 
 version (around line 165 - also done in the gzip part if the last 
 allocated buffer is empty).
 Line 165 I would have done it with something like
 "if (tocopy == 0) { KASSERT(j == (i - 1)); } else { memcpy(output, 
 buf[j].out, tocopy); }"
 but this is of cause a question of the current religion ...

 I think this report can be closed.

 Best regards

 W. Stukenbrock

 dholland@NetBSD.org wrote:

 > Synopsis: opencrypto - deflate wastes time with useless data copying
 > 
 > State-Changed-From-To: closed->feedback
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Sun, 20 Feb 2011 00:38:24 +0000
 > State-Changed-Why:
 > Since you asked a question I think you meant feedback, not closed :-)
 > 
 > 
 > 
 > 


 -- 


 Dr. Nagler & Company GmbH
 Hauptstraße 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze


State-Changed-From-To: feedback->closed
State-Changed-By: drochner@NetBSD.org
State-Changed-When: Mon, 21 Feb 2011 21:53:51 +0000
State-Changed-Why:
Submitter is satisfied. I'll care about the GZIP sync later.
About the memcpy(,,0): While it doesn't look nice, it is
OK for memcpy() itself. As I understand it, this case can
only occur if either a compressed buffer without proper
termination is handled (as produced by former versions of
this code), or the zlib bug described in PR kern/44594 is
hit. Both problems should be fixed eventually, and then the
extra check for 0 length shouls never hit.


>Unformatted:

Home	PR Database Search