On 01/23/2013 10:48 AM, Ondrej Oprala wrote: > + /* Delay buffer allocation if possible. */ > + if ((skip_records > OFF_T_MAX / input_blocksize) > + || 0 > skip_via_lseek (input_file, STDIN_FILENO, 0, SEEK_CUR)) > + alloc_ibuf (); > > - /* Write a sentinel to the slop after the buffer, > - to allow efficient checking for NUL blocks. */ > - assert (sizeof (uintptr_t) <= OUTPUT_BLOCK_SLOP); > - memset (obuf + output_blocksize, 1, sizeof (uintptr_t)); > + if ((seek_records > OFF_T_MAX / output_blocksize) > + || 0 > skip_via_lseek (output_file, STDOUT_FILENO, 0, SEEK_CUR)) > + alloc_obuf ();
This doesn't look right. skip_via_lseek() doesn't support lseek(..., 0, SEEK_CUR) and may always warn for tape devices? Also does skip bytes need to be considered? Also you don't always need to allocate both buffers. How about avoiding new conditions altogether and just alloc_[io]buf() before they're needed (i.e. closer to the read() as I suggested originally). Also I adjusted the memory limits in the test, to reduce the chances of false positives, and bolstered the test cases to cater for the new allocation calls within skip(). I'll push the attached soon. thanks, Pádraig.
>From 87c883e00f043faf669040a96a24c59bdeeda662 Mon Sep 17 00:00:00 2001 From: Ondrej Oprala <[email protected]> Date: Tue, 22 Jan 2013 14:21:23 +0100 Subject: [PATCH] dd: avoid buffer allocations unless needed * src/dd.c: Add new static global variable ibuf. (alloc_ibuf, alloc_obuf): New functions factored from dd_copy(). (dd_copy): Call the new functions to allocate memory for ibuf and obuf when necessary. (skip): Likewise. * tests/dd/no-allocate.sh: New test. * tests/local.mk: Reference the test. --- src/dd.c | 135 ++++++++++++++++++++++++++++------------------ tests/dd/no-allocate.sh | 53 ++++++++++++++++++ tests/local.mk | 1 + 3 files changed, 136 insertions(+), 53 deletions(-) create mode 100755 tests/dd/no-allocate.sh diff --git a/src/dd.c b/src/dd.c index c98e578..f727a5e 100644 --- a/src/dd.c +++ b/src/dd.c @@ -236,6 +236,9 @@ static uintmax_t r_truncate = 0; static char newline_character = '\n'; static char space_character = ' '; +/* Input buffer. */ +static char *ibuf; + /* Output buffer. */ static char *obuf; @@ -646,6 +649,65 @@ Options are:\n\ exit (status); } +static char * +human_size (size_t n) +{ + static char hbuf[LONGEST_HUMAN_READABLE + 1]; + int human_opts = + (human_autoscale | human_round_to_nearest | human_base_1024 + | human_space_before_unit | human_SI | human_B); + return human_readable (n, hbuf, human_opts, 1, 1); +} + +/* Ensure input buffer IBUF is allocated. */ + +static void +alloc_ibuf (void) +{ + if (ibuf) + return; + + char *real_buf = malloc (input_blocksize + INPUT_BLOCK_SLOP); + if (!real_buf) + error (EXIT_FAILURE, 0, + _("memory exhausted by input buffer of size %zu bytes (%s)"), + input_blocksize, human_size (input_blocksize)); + + real_buf += SWAB_ALIGN_OFFSET; /* allow space for swab */ + + ibuf = ptr_align (real_buf, page_size); +} + +/* Ensure output buffer OBUF is allocated/initialized. */ + +static void +alloc_obuf (void) +{ + if (obuf) + return; + + if (conversions_mask & C_TWOBUFS) + { + /* Page-align the output buffer, too. */ + char *real_obuf = malloc (output_blocksize + OUTPUT_BLOCK_SLOP); + if (!real_obuf) + error (EXIT_FAILURE, 0, + _("memory exhausted by output buffer of size %zu bytes (%s)"), + output_blocksize, human_size (output_blocksize)); + obuf = ptr_align (real_obuf, page_size); + } + else + { + alloc_ibuf (); + obuf = ibuf; + } + + /* Write a sentinel to the slop after the buffer, + to allow efficient checking for NUL blocks. */ + assert (sizeof (uintptr_t) <= OUTPUT_BLOCK_SLOP); + memset (obuf + output_blocksize, 1, sizeof (uintptr_t)); +} + static void translate_charset (char const *new_trans) { @@ -1526,7 +1588,7 @@ skip_via_lseek (char const *filename, int fdesc, off_t offset, int whence) /* Throw away RECORDS blocks of BLOCKSIZE bytes plus BYTES bytes on file descriptor FDESC, which is open with read permission for FILE. - Store up to BLOCKSIZE bytes of the data at a time in BUF, if + Store up to BLOCKSIZE bytes of the data at a time in IBUF or OBUF, if necessary. RECORDS or BYTES must be nonzero. If FDESC is STDIN_FILENO, advance the input offset. Return the number of records remaining, i.e., that were not skipped because EOF was @@ -1535,7 +1597,7 @@ skip_via_lseek (char const *filename, int fdesc, off_t offset, int whence) static uintmax_t skip (int fdesc, char const *file, uintmax_t records, size_t blocksize, - size_t *bytes, char *buf) + size_t *bytes) { uintmax_t offset = records * blocksize + *bytes; @@ -1607,6 +1669,18 @@ skip (int fdesc, char const *file, uintmax_t records, size_t blocksize, } /* else file_size && offset > OFF_T_MAX or file ! seekable */ + char *buf; + if (fdesc == STDIN_FILENO) + { + alloc_ibuf (); + buf = ibuf; + } + else + { + alloc_obuf (); + buf = obuf; + } + do { ssize_t nread = iread_fnc (fdesc, buf, records ? blocksize : *bytes); @@ -1823,26 +1897,12 @@ set_fd_flags (int fd, int add_flags, char const *name) } } -static char * -human_size (size_t n) -{ - static char hbuf[LONGEST_HUMAN_READABLE + 1]; - int human_opts = - (human_autoscale | human_round_to_nearest | human_base_1024 - | human_space_before_unit | human_SI | human_B); - return human_readable (n, hbuf, human_opts, 1, 1); -} - /* The main loop. */ static int dd_copy (void) { - char *ibuf, *bufstart; /* Input buffer. */ - /* These are declared static so that even though we don't free the - buffers, valgrind will recognize that there is no "real" leak. */ - static char *real_buf; /* real buffer address before alignment */ - static char *real_obuf; + char *bufstart; /* Input buffer. */ ssize_t nread; /* Bytes read in the current block. */ /* If nonzero, then the previously read block was partial and @@ -1869,45 +1929,12 @@ dd_copy (void) It is necessary when accessing raw (i.e. character special) disk devices on Unixware or other SVR4-derived system. */ - real_buf = malloc (input_blocksize + INPUT_BLOCK_SLOP); - if (!real_buf) - error (EXIT_FAILURE, 0, - _("memory exhausted by input buffer of size %zu bytes (%s)"), - input_blocksize, human_size (input_blocksize)); - - ibuf = real_buf; - ibuf += SWAB_ALIGN_OFFSET; /* allow space for swab */ - - ibuf = ptr_align (ibuf, page_size); - - if (conversions_mask & C_TWOBUFS) - { - /* Page-align the output buffer, too. */ - real_obuf = malloc (output_blocksize + OUTPUT_BLOCK_SLOP); - if (!real_obuf) - error (EXIT_FAILURE, 0, - _("memory exhausted by output buffer of size %zu bytes (%s)"), - output_blocksize, human_size (output_blocksize)); - obuf = ptr_align (real_obuf, page_size); - } - else - { - real_obuf = NULL; - obuf = ibuf; - } - - /* Write a sentinel to the slop after the buffer, - to allow efficient checking for NUL blocks. */ - assert (sizeof (uintptr_t) <= OUTPUT_BLOCK_SLOP); - memset (obuf + output_blocksize, 1, sizeof (uintptr_t)); - if (skip_records != 0 || skip_bytes != 0) { uintmax_t us_bytes = input_offset + (skip_records * input_blocksize) + skip_bytes; uintmax_t us_blocks = skip (STDIN_FILENO, input_file, - skip_records, input_blocksize, &skip_bytes, - ibuf); + skip_records, input_blocksize, &skip_bytes); us_bytes -= input_offset; /* POSIX doesn't say what to do when dd detects it has been @@ -1927,8 +1954,7 @@ dd_copy (void) { size_t bytes = seek_bytes; uintmax_t write_records = skip (STDOUT_FILENO, output_file, - seek_records, output_blocksize, &bytes, - obuf); + seek_records, output_blocksize, &bytes); if (write_records != 0 || bytes != 0) { @@ -1955,6 +1981,9 @@ dd_copy (void) if (max_records == 0 && max_bytes == 0) return exit_status; + alloc_ibuf (); + alloc_obuf (); + while (1) { if (r_partial + r_full >= max_records + !!max_bytes) diff --git a/tests/dd/no-allocate.sh b/tests/dd/no-allocate.sh new file mode 100755 index 0000000..e45dd5c --- /dev/null +++ b/tests/dd/no-allocate.sh @@ -0,0 +1,53 @@ +#!/bin/sh +# make sure that dd doesn't allocate memory unnecessarily + +# Copyright (C) 2013 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src +print_ver_ dd +require_ulimit_ + +# count and skip is zero, we don't need to allocate memory +(ulimit -v 20000; dd bs=30M count=0) || fail=1 +(ulimit -v 20000; dd ibs=30M count=0) || fail=1 +(ulimit -v 20000; dd obs=30M count=0) || fail=1 + + +# Use a fifo for which seek fails, but read does not +if mkfifo tape; then + # for non seekable output we need to allocate buffer when needed + echo 1 > tape& + (ulimit -v 20000; dd bs=30M skip=1 count=0 if=tape) && fail=1 + + echo 1 > tape& + (ulimit -v 20000; dd ibs=30M skip=1 count=0 if=tape) && fail=1 + + echo 1 > tape& + (ulimit -v 20000; dd obs=30M skip=1 count=0 if=tape) || fail=1 + + + # for non seekable output we need to allocate buffer when needed + echo 1 > tape& + (ulimit -v 20000; dd bs=30M seek=1 count=0 of=tape) && fail=1 + + echo 1 > tape& + (ulimit -v 20000; dd obs=30M seek=1 count=0 of=tape) && fail=1 + + echo 1 > tape& + (ulimit -v 20000; dd ibs=30M seek=1 count=0 of=tape) || fail=1 +fi + +Exit $fail diff --git a/tests/local.mk b/tests/local.mk index dc87ef4..5cd10ad 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -464,6 +464,7 @@ all_tests = \ tests/df/skip-rootfs.sh \ tests/dd/direct.sh \ tests/dd/misc.sh \ + tests/dd/no-allocate.sh \ tests/dd/nocache.sh \ tests/dd/not-rewound.sh \ tests/dd/reblock.sh \ -- 1.7.7.6
