Fix decompression failure found during the loading of compressed trace
collected on larger scale systems (>48 cores).
The error happened due to lack of decompression space for a mmaped buffer
data chunk split across adjacent PERF_RECORD_COMPRESSED records.
$ perf report -i bt.16384.data --stats
failed to decompress (B): 63869 -> 0 : Destination buffer is too small
user stack dump failure
Can't parse sample, err = -14
0x2637e436 [0x4080]: failed to process type: 9
Error:
failed to process sample
$ perf test 71
71: Zstd perf.data compression/decompression : Ok
Signed-off-by: Alexey Budankov <[email protected]>
---
tools/perf/util/session.c | 22 ++++++++++++++--------
tools/perf/util/session.h | 1 +
tools/perf/util/zstd.c | 4 ++--
3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index d0fd6c614e68..37efa1f43d8b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -36,10 +36,16 @@ static int perf_session__process_compressed_event(struct perf_session *session,
void *src;
size_t decomp_size, src_size;
u64 decomp_last_rem = 0;
- size_t decomp_len = session->header.env.comp_mmap_len;
+ size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
struct decomp *decomp, *decomp_last = session->decomp_last;
- decomp = mmap(NULL, sizeof(struct decomp) + decomp_len, PROT_READ|PROT_WRITE,
+ if (decomp_last) {
+ decomp_last_rem = decomp_last->size - decomp_last->head;
+ decomp_len += decomp_last_rem;
+ }
+
+ mmap_len = sizeof(struct decomp) + decomp_len;
+ decomp = mmap(NULL, mmap_len, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
if (decomp == MAP_FAILED) {
pr_err("Couldn't allocate memory for decompression\n");
@@ -47,10 +53,10 @@ static int perf_session__process_compressed_event(struct perf_session *session,
}
decomp->file_pos = file_offset;
+ decomp->mmap_len = mmap_len;
decomp->head = 0;
- if (decomp_last) {
- decomp_last_rem = decomp_last->size - decomp_last->head;
+ if (decomp_last_rem) {
memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
decomp->size = decomp_last_rem;
}
@@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
if (!decomp_size) {
- munmap(decomp, sizeof(struct decomp) + decomp_len);
+ munmap(decomp, mmap_len);
pr_err("Couldn't decompress data\n");
return -1;
}
@@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
static void perf_session__release_decomp_events(struct perf_session *session)
{
struct decomp *next, *decomp;
- size_t decomp_len;
+ size_t mmap_len;
next = session->decomp;
- decomp_len = session->header.env.comp_mmap_len;
do {
decomp = next;
if (decomp == NULL)
break;
next = decomp->next;
- munmap(decomp, decomp_len + sizeof(struct decomp));
+ mmap_len = decomp->mmap_len;
+ munmap(decomp, mmap_len);
} while (1);
}
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index dd8920b745bc..863dbad87849 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -46,6 +46,7 @@ struct perf_session {
struct decomp {
struct decomp *next;
u64 file_pos;
+ size_t mmap_len;
u64 head;
size_t size;
char data[];
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 23bdb9884576..d2202392ffdb 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -99,8 +99,8 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
while (input.pos < input.size) {
ret = ZSTD_decompressStream(data->dstream, &output, &input);
if (ZSTD_isError(ret)) {
- pr_err("failed to decompress (B): %ld -> %ld : %s\n",
- src_size, output.size, ZSTD_getErrorName(ret));
+ pr_err("failed to decompress (B): %ld -> %ld, dst_size %ld : %s\n",
+ src_size, output.size, dst_size, ZSTD_getErrorName(ret));
break;
}
output.dst = dst + output.pos;
--
2.20.1
On Tue, Jul 09, 2019 at 05:48:14PM +0300, Alexey Budankov wrote:
SNIP
> decomp->file_pos = file_offset;
> + decomp->mmap_len = mmap_len;
> decomp->head = 0;
>
> - if (decomp_last) {
> - decomp_last_rem = decomp_last->size - decomp_last->head;
> + if (decomp_last_rem) {
> memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
> decomp->size = decomp_last_rem;
> }
> @@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
> decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
> &(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
> if (!decomp_size) {
> - munmap(decomp, sizeof(struct decomp) + decomp_len);
> + munmap(decomp, mmap_len);
> pr_err("Couldn't decompress data\n");
> return -1;
> }
> @@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
> static void perf_session__release_decomp_events(struct perf_session *session)
> {
> struct decomp *next, *decomp;
> - size_t decomp_len;
> + size_t mmap_len;
> next = session->decomp;
> - decomp_len = session->header.env.comp_mmap_len;
> do {
> decomp = next;
> if (decomp == NULL)
> break;
> next = decomp->next;
> - munmap(decomp, decomp_len + sizeof(struct decomp));
> + mmap_len = decomp->mmap_len;
> + munmap(decomp, mmap_len);
what's the need for extra mmap_len variable in here?
could you just use decomp->mmap_len directly?
otherwise it lookgs good to me
thanks,
jirka
On 14.07.2019 18:49, Jiri Olsa wrote:
> On Tue, Jul 09, 2019 at 05:48:14PM +0300, Alexey Budankov wrote:
>
> SNIP
>
>> decomp->file_pos = file_offset;
>> + decomp->mmap_len = mmap_len;
>> decomp->head = 0;
>>
>> - if (decomp_last) {
>> - decomp_last_rem = decomp_last->size - decomp_last->head;
>> + if (decomp_last_rem) {
>> memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
>> decomp->size = decomp_last_rem;
>> }
>> @@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>> decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
>> &(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
>> if (!decomp_size) {
>> - munmap(decomp, sizeof(struct decomp) + decomp_len);
>> + munmap(decomp, mmap_len);
>> pr_err("Couldn't decompress data\n");
>> return -1;
>> }
>> @@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
>> static void perf_session__release_decomp_events(struct perf_session *session)
>> {
>> struct decomp *next, *decomp;
>> - size_t decomp_len;
>> + size_t mmap_len;
>> next = session->decomp;
>> - decomp_len = session->header.env.comp_mmap_len;
>> do {
>> decomp = next;
>> if (decomp == NULL)
>> break;
>> next = decomp->next;
>> - munmap(decomp, decomp_len + sizeof(struct decomp));
>> + mmap_len = decomp->mmap_len;
>> + munmap(decomp, mmap_len);
>
> what's the need for extra mmap_len variable in here?
> could you just use decomp->mmap_len directly?
To avoid reference to the object being deallocated.
Plain munmap(), yes - :)
~Alexey
>
> otherwise it lookgs good to me
>
> thanks,
> jirka
>
Em Mon, Jul 15, 2019 at 03:30:24PM +0300, Alexey Budankov escreveu:
> On 14.07.2019 18:49, Jiri Olsa wrote:
> > On Tue, Jul 09, 2019 at 05:48:14PM +0300, Alexey Budankov wrote:
> >
> > SNIP
> >
> >> decomp->file_pos = file_offset;
> >> + decomp->mmap_len = mmap_len;
> >> decomp->head = 0;
> >>
> >> - if (decomp_last) {
> >> - decomp_last_rem = decomp_last->size - decomp_last->head;
> >> + if (decomp_last_rem) {
> >> memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
> >> decomp->size = decomp_last_rem;
> >> }
> >> @@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
> >> decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
> >> &(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
> >> if (!decomp_size) {
> >> - munmap(decomp, sizeof(struct decomp) + decomp_len);
> >> + munmap(decomp, mmap_len);
> >> pr_err("Couldn't decompress data\n");
> >> return -1;
> >> }
> >> @@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
> >> static void perf_session__release_decomp_events(struct perf_session *session)
> >> {
> >> struct decomp *next, *decomp;
> >> - size_t decomp_len;
> >> + size_t mmap_len;
> >> next = session->decomp;
> >> - decomp_len = session->header.env.comp_mmap_len;
> >> do {
> >> decomp = next;
> >> if (decomp == NULL)
> >> break;
> >> next = decomp->next;
> >> - munmap(decomp, decomp_len + sizeof(struct decomp));
> >> + mmap_len = decomp->mmap_len;
> >> + munmap(decomp, mmap_len);
> >
> > what's the need for extra mmap_len variable in here?
> > could you just use decomp->mmap_len directly?
>
> To avoid reference to the object being deallocated.
> Plain munmap(), yes - :)
So, Jiri, Acked-by?
- Arnaldo
On Tue, Jul 16, 2019 at 03:49:59PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 15, 2019 at 03:30:24PM +0300, Alexey Budankov escreveu:
> > On 14.07.2019 18:49, Jiri Olsa wrote:
> > > On Tue, Jul 09, 2019 at 05:48:14PM +0300, Alexey Budankov wrote:
> > >
> > > SNIP
> > >
> > >> decomp->file_pos = file_offset;
> > >> + decomp->mmap_len = mmap_len;
> > >> decomp->head = 0;
> > >>
> > >> - if (decomp_last) {
> > >> - decomp_last_rem = decomp_last->size - decomp_last->head;
> > >> + if (decomp_last_rem) {
> > >> memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
> > >> decomp->size = decomp_last_rem;
> > >> }
> > >> @@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
> > >> decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
> > >> &(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
> > >> if (!decomp_size) {
> > >> - munmap(decomp, sizeof(struct decomp) + decomp_len);
> > >> + munmap(decomp, mmap_len);
> > >> pr_err("Couldn't decompress data\n");
> > >> return -1;
> > >> }
> > >> @@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
> > >> static void perf_session__release_decomp_events(struct perf_session *session)
> > >> {
> > >> struct decomp *next, *decomp;
> > >> - size_t decomp_len;
> > >> + size_t mmap_len;
> > >> next = session->decomp;
> > >> - decomp_len = session->header.env.comp_mmap_len;
> > >> do {
> > >> decomp = next;
> > >> if (decomp == NULL)
> > >> break;
> > >> next = decomp->next;
> > >> - munmap(decomp, decomp_len + sizeof(struct decomp));
> > >> + mmap_len = decomp->mmap_len;
> > >> + munmap(decomp, mmap_len);
> > >
> > > what's the need for extra mmap_len variable in here?
> > > could you just use decomp->mmap_len directly?
> >
> > To avoid reference to the object being deallocated.
> > Plain munmap(), yes - :)
well it's passed as value, so should be ok,
anyway I was just curious, if I'm not missing
something else.. it's ok ;-)
>
> So, Jiri, Acked-by?
yep ;-)
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
>
> - Arnaldo
Em Tue, Jul 16, 2019 at 10:59:30PM +0200, Jiri Olsa escreveu:
> On Tue, Jul 16, 2019 at 03:49:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 15, 2019 at 03:30:24PM +0300, Alexey Budankov escreveu:
> > > On 14.07.2019 18:49, Jiri Olsa wrote:
> > > > On Tue, Jul 09, 2019 at 05:48:14PM +0300, Alexey Budankov wrote:
> > > >
> > > > SNIP
> > > >
> > > >> decomp->file_pos = file_offset;
> > > >> + decomp->mmap_len = mmap_len;
> > > >> decomp->head = 0;
> > > >>
> > > >> - if (decomp_last) {
> > > >> - decomp_last_rem = decomp_last->size - decomp_last->head;
> > > >> + if (decomp_last_rem) {
> > > >> memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
> > > >> decomp->size = decomp_last_rem;
> > > >> }
> > > >> @@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
> > > >> decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
> > > >> &(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
> > > >> if (!decomp_size) {
> > > >> - munmap(decomp, sizeof(struct decomp) + decomp_len);
> > > >> + munmap(decomp, mmap_len);
> > > >> pr_err("Couldn't decompress data\n");
> > > >> return -1;
> > > >> }
> > > >> @@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
> > > >> static void perf_session__release_decomp_events(struct perf_session *session)
> > > >> {
> > > >> struct decomp *next, *decomp;
> > > >> - size_t decomp_len;
> > > >> + size_t mmap_len;
> > > >> next = session->decomp;
> > > >> - decomp_len = session->header.env.comp_mmap_len;
> > > >> do {
> > > >> decomp = next;
> > > >> if (decomp == NULL)
> > > >> break;
> > > >> next = decomp->next;
> > > >> - munmap(decomp, decomp_len + sizeof(struct decomp));
> > > >> + mmap_len = decomp->mmap_len;
> > > >> + munmap(decomp, mmap_len);
> > > >
> > > > what's the need for extra mmap_len variable in here?
> > > > could you just use decomp->mmap_len directly?
> > >
> > > To avoid reference to the object being deallocated.
> > > Plain munmap(), yes - :)
>
> well it's passed as value, so should be ok,
> anyway I was just curious, if I'm not missing
> something else.. it's ok ;-)
>
> >
> > So, Jiri, Acked-by?
>
> yep ;-)
>
> Acked-by: Jiri Olsa <[email protected]>
Thanks, applied.
- Arnaldo
Commit-ID: 872c8ee8f0f47222f7b10da96eea84d0486540a3
Gitweb: https://git.kernel.org/tip/872c8ee8f0f47222f7b10da96eea84d0486540a3
Author: Alexey Budankov <[email protected]>
AuthorDate: Tue, 9 Jul 2019 17:48:14 +0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 23 Jul 2019 09:04:03 -0300
perf session: Fix loading of compressed data split across adjacent records
Fix decompression failure found during the loading of compressed trace
collected on larger scale systems (>48 cores).
The error happened due to lack of decompression space for a mmaped
buffer data chunk split across adjacent PERF_RECORD_COMPRESSED records.
$ perf report -i bt.16384.data --stats
failed to decompress (B): 63869 -> 0 : Destination buffer is too small
user stack dump failure
Can't parse sample, err = -14
0x2637e436 [0x4080]: failed to process type: 9
Error:
failed to process sample
$ perf test 71
71: Zstd perf.data compression/decompression : Ok
Signed-off-by: Alexey Budankov <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/session.c | 22 ++++++++++++++--------
tools/perf/util/session.h | 1 +
tools/perf/util/zstd.c | 4 ++--
3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index d0fd6c614e68..37efa1f43d8b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -36,10 +36,16 @@ static int perf_session__process_compressed_event(struct perf_session *session,
void *src;
size_t decomp_size, src_size;
u64 decomp_last_rem = 0;
- size_t decomp_len = session->header.env.comp_mmap_len;
+ size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
struct decomp *decomp, *decomp_last = session->decomp_last;
- decomp = mmap(NULL, sizeof(struct decomp) + decomp_len, PROT_READ|PROT_WRITE,
+ if (decomp_last) {
+ decomp_last_rem = decomp_last->size - decomp_last->head;
+ decomp_len += decomp_last_rem;
+ }
+
+ mmap_len = sizeof(struct decomp) + decomp_len;
+ decomp = mmap(NULL, mmap_len, PROT_READ|PROT_WRITE,
MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
if (decomp == MAP_FAILED) {
pr_err("Couldn't allocate memory for decompression\n");
@@ -47,10 +53,10 @@ static int perf_session__process_compressed_event(struct perf_session *session,
}
decomp->file_pos = file_offset;
+ decomp->mmap_len = mmap_len;
decomp->head = 0;
- if (decomp_last) {
- decomp_last_rem = decomp_last->size - decomp_last->head;
+ if (decomp_last_rem) {
memcpy(decomp->data, &(decomp_last->data[decomp_last->head]), decomp_last_rem);
decomp->size = decomp_last_rem;
}
@@ -61,7 +67,7 @@ static int perf_session__process_compressed_event(struct perf_session *session,
decomp_size = zstd_decompress_stream(&(session->zstd_data), src, src_size,
&(decomp->data[decomp_last_rem]), decomp_len - decomp_last_rem);
if (!decomp_size) {
- munmap(decomp, sizeof(struct decomp) + decomp_len);
+ munmap(decomp, mmap_len);
pr_err("Couldn't decompress data\n");
return -1;
}
@@ -255,15 +261,15 @@ static void perf_session__delete_threads(struct perf_session *session)
static void perf_session__release_decomp_events(struct perf_session *session)
{
struct decomp *next, *decomp;
- size_t decomp_len;
+ size_t mmap_len;
next = session->decomp;
- decomp_len = session->header.env.comp_mmap_len;
do {
decomp = next;
if (decomp == NULL)
break;
next = decomp->next;
- munmap(decomp, decomp_len + sizeof(struct decomp));
+ mmap_len = decomp->mmap_len;
+ munmap(decomp, mmap_len);
} while (1);
}
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index dd8920b745bc..863dbad87849 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -46,6 +46,7 @@ struct perf_session {
struct decomp {
struct decomp *next;
u64 file_pos;
+ size_t mmap_len;
u64 head;
size_t size;
char data[];
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 23bdb9884576..d2202392ffdb 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -99,8 +99,8 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
while (input.pos < input.size) {
ret = ZSTD_decompressStream(data->dstream, &output, &input);
if (ZSTD_isError(ret)) {
- pr_err("failed to decompress (B): %ld -> %ld : %s\n",
- src_size, output.size, ZSTD_getErrorName(ret));
+ pr_err("failed to decompress (B): %ld -> %ld, dst_size %ld : %s\n",
+ src_size, output.size, dst_size, ZSTD_getErrorName(ret));
break;
}
output.dst = dst + output.pos;