| Message ID | 20201126023814.1322133-1-rashmica.g@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Revert "htm: Use splice() to copy dump" | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (bdec2f8c512ff4a70eaadcb01a7494b4cb5e13e5) |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
On Thu, 2020-11-26 at 13:38 +1100, Rashmica Gupta wrote: > This reverts commit 436eb8c74fb4a762b61837ee27ddbd6b5fe21334. > Unable to use splice on newer kernels due to 36e2c7421f02 ("fs: don't > allow splice read/write without explicit ops"). So revert back to plain > old read/write. Is there a performance penalty with this and if so how much? Mikey > > Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> > --- > libpdbg/htm.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/libpdbg/htm.c b/libpdbg/htm.c > index 0d755dd..43cef84 100644 > --- a/libpdbg/htm.c > +++ b/libpdbg/htm.c > @@ -966,39 +966,41 @@ static int do_htm_status(struct htm *htm) > return 1; > } > > +#define COPY_BUF_SIZE getpagesize() > static int copy_file(int output, int input, uint64_t size) > { > + char *buf; > size_t r; > - int pipefd[2]; > - int rc = -1; > > - if (pipe(pipefd)) { > - perror("pipe"); > - exit(1); > + buf = malloc(COPY_BUF_SIZE); > + if (!buf) { > + PR_ERROR("Can't malloc buffer\n"); > + return -1; > } > > while (size) { > - r = splice(input, 0, pipefd[1], 0, size, 0); > + r = read(input, buf, MIN(COPY_BUF_SIZE, size)); > if (r == -1) { > PR_ERROR("Failed to read\n"); > goto out; > } > if (r == 0) { > - PR_ERROR("Unexpect EOF\n"); > + PR_ERROR("EOF\n"); > goto out; > } > > - if (splice(pipefd[0], 0, output, 0, r, 0) != r) { > + if (write(output, buf, r) != r) { > PR_ERROR("Short write!\n"); > goto out; > } > size -= r; > } > - rc = 0; > + > + return 0; > + > out: > - close(pipefd[1]); > - close(pipefd[0]); > - return rc; > + free(buf); > + return -1; > > } >
On Tue, 2020-12-01 at 11:52 +1100, Rashmica Gupta wrote: > On Tue, 2020-12-01 at 11:37 +1100, Michael Neuling wrote: > > On Thu, 2020-11-26 at 13:38 +1100, Rashmica Gupta wrote: > > > This reverts commit 436eb8c74fb4a762b61837ee27ddbd6b5fe21334. > > > Unable to use splice on newer kernels due to 36e2c7421f02 ("fs: > > > don't > > > allow splice read/write without explicit ops"). So revert back to > > > plain > > > old read/write. > > > > Is there a performance penalty with this and if so how much? > > On older kernels most probably, but does it matter when splice just > fails on newer kernels? Do you want to dectect kernel version and > choose whether to use splice or not? (adding people back on CC) If the performance hit is particularly bad, then we might want the splice() optimisation, but if it's only a small hit then I wouldn't bother. Mikey
On Tue, 2020-12-01 at 12:25 +1100, Michael Neuling wrote: > On Tue, 2020-12-01 at 11:52 +1100, Rashmica Gupta wrote: > > On Tue, 2020-12-01 at 11:37 +1100, Michael Neuling wrote: > > > On Thu, 2020-11-26 at 13:38 +1100, Rashmica Gupta wrote: > > > > This reverts commit 436eb8c74fb4a762b61837ee27ddbd6b5fe21334. > > > > Unable to use splice on newer kernels due to 36e2c7421f02 ("fs: > > > > don't > > > > allow splice read/write without explicit ops"). So revert back > > > > to > > > > plain > > > > old read/write. > > > > > > Is there a performance penalty with this and if so how much? > > > > On older kernels most probably, but does it matter when splice just > > fails on newer kernels? Do you want to dectect kernel version and > > choose whether to use splice or not? > > (adding people back on CC) > > If the performance hit is particularly bad, then we might want the > splice() > optimisation, but if it's only a small hit then I wouldn't bother. With my eyeball statistics, there is no noticable difference with dumping a 1.7GB file. > > Mikey >
On Fri, 2020-12-04 at 08:42 +1100, Rashmica Gupta wrote: > On Tue, 2020-12-01 at 12:25 +1100, Michael Neuling wrote: > > On Tue, 2020-12-01 at 11:52 +1100, Rashmica Gupta wrote: > > > On Tue, 2020-12-01 at 11:37 +1100, Michael Neuling wrote: > > > > On Thu, 2020-11-26 at 13:38 +1100, Rashmica Gupta wrote: > > > > > This reverts commit 436eb8c74fb4a762b61837ee27ddbd6b5fe21334. > > > > > Unable to use splice on newer kernels due to 36e2c7421f02 ("fs: > > > > > don't > > > > > allow splice read/write without explicit ops"). So revert back > > > > > to > > > > > plain > > > > > old read/write. > > > > > > > > Is there a performance penalty with this and if so how much? > > > > > > On older kernels most probably, but does it matter when splice just > > > fails on newer kernels? Do you want to dectect kernel version and > > > choose whether to use splice or not? > > > > (adding people back on CC) > > > > If the performance hit is particularly bad, then we might want the > > splice() > > optimisation, but if it's only a small hit then I wouldn't bother. > > With my eyeball statistics, there is no noticable difference with > dumping a 1.7GB file. OK, great, let's not bother with any optimisation the. Thanks, Mikey
diff --git a/libpdbg/htm.c b/libpdbg/htm.c index 0d755dd..43cef84 100644 --- a/libpdbg/htm.c +++ b/libpdbg/htm.c @@ -966,39 +966,41 @@ static int do_htm_status(struct htm *htm) return 1; } +#define COPY_BUF_SIZE getpagesize() static int copy_file(int output, int input, uint64_t size) { + char *buf; size_t r; - int pipefd[2]; - int rc = -1; - if (pipe(pipefd)) { - perror("pipe"); - exit(1); + buf = malloc(COPY_BUF_SIZE); + if (!buf) { + PR_ERROR("Can't malloc buffer\n"); + return -1; } while (size) { - r = splice(input, 0, pipefd[1], 0, size, 0); + r = read(input, buf, MIN(COPY_BUF_SIZE, size)); if (r == -1) { PR_ERROR("Failed to read\n"); goto out; } if (r == 0) { - PR_ERROR("Unexpect EOF\n"); + PR_ERROR("EOF\n"); goto out; } - if (splice(pipefd[0], 0, output, 0, r, 0) != r) { + if (write(output, buf, r) != r) { PR_ERROR("Short write!\n"); goto out; } size -= r; } - rc = 0; + + return 0; + out: - close(pipefd[1]); - close(pipefd[0]); - return rc; + free(buf); + return -1; }
This reverts commit 436eb8c74fb4a762b61837ee27ddbd6b5fe21334. Unable to use splice on newer kernels due to 36e2c7421f02 ("fs: don't allow splice read/write without explicit ops"). So revert back to plain old read/write. Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com> --- libpdbg/htm.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)