Open files located at trace data directory in case read access
mode is requested. File are opened and its fds assigned to
perf_data dir files especially for loading data directories
content in perf report mode.
Signed-off-by: Alexey Budankov <[email protected]>
---
tools/perf/util/data.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index c47aa34fdc0a..6ad61ac6ba67 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
return -1;
ret = open_file(data);
+ if (!ret && perf_data__is_dir(data)) {
+ if (perf_data__is_read(data))
+ ret = perf_data__open_dir(data);
+ }
/* Cleanup whatever we managed to create so far. */
if (ret && perf_data__is_write(data))
--
2.24.1
On Thu, Oct 22, 2020 at 12:58 AM Alexey Budankov
<[email protected]> wrote:
>
>
> Open files located at trace data directory in case read access
> mode is requested. File are opened and its fds assigned to
> perf_data dir files especially for loading data directories
> content in perf report mode.
>
> Signed-off-by: Alexey Budankov <[email protected]>
> ---
> tools/perf/util/data.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index c47aa34fdc0a..6ad61ac6ba67 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
> return -1;
>
> ret = open_file(data);
> + if (!ret && perf_data__is_dir(data)) {
I think this __is_dir() check is unnecessary since it's checked
from the caller side already.
Thanks
Namhyung
> + if (perf_data__is_read(data))
> + ret = perf_data__open_dir(data);
> + }
>
> /* Cleanup whatever we managed to create so far. */
> if (ret && perf_data__is_write(data))
> --
> 2.24.1
>
On 22.10.2020 7:31, Namhyung Kim wrote:
> On Thu, Oct 22, 2020 at 12:58 AM Alexey Budankov
> <[email protected]> wrote:
>>
>>
>> Open files located at trace data directory in case read access
>> mode is requested. File are opened and its fds assigned to
>> perf_data dir files especially for loading data directories
>> content in perf report mode.
>>
>> Signed-off-by: Alexey Budankov <[email protected]>
>> ---
>> tools/perf/util/data.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
>> index c47aa34fdc0a..6ad61ac6ba67 100644
>> --- a/tools/perf/util/data.c
>> +++ b/tools/perf/util/data.c
>> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
>> return -1;
>>
>> ret = open_file(data);
>> + if (!ret && perf_data__is_dir(data)) {
>
> I think this __is_dir() check is unnecessary since it's checked
> from the caller side already.
Corrected in v3. Thanks!
Alexei
On Wed, Oct 21, 2020 at 06:57:53PM +0300, Alexey Budankov wrote:
>
> Open files located at trace data directory in case read access
> mode is requested. File are opened and its fds assigned to
> perf_data dir files especially for loading data directories
> content in perf report mode.
>
> Signed-off-by: Alexey Budankov <[email protected]>
> ---
> tools/perf/util/data.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index c47aa34fdc0a..6ad61ac6ba67 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
> return -1;
>
> ret = open_file(data);
> + if (!ret && perf_data__is_dir(data)) {
> + if (perf_data__is_read(data))
> + ret = perf_data__open_dir(data);
> + }
perf_data__open_dir is also called from perf_session__new
is it called twice?
jirka
>
> /* Cleanup whatever we managed to create so far. */
> if (ret && perf_data__is_write(data))
> --
> 2.24.1
>
On 24.10.2020 18:43, Jiri Olsa wrote:
> On Wed, Oct 21, 2020 at 06:57:53PM +0300, Alexey Budankov wrote:
>>
>> Open files located at trace data directory in case read access
>> mode is requested. File are opened and its fds assigned to
>> perf_data dir files especially for loading data directories
>> content in perf report mode.
>>
>> Signed-off-by: Alexey Budankov <[email protected]>
>> ---
>> tools/perf/util/data.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
>> index c47aa34fdc0a..6ad61ac6ba67 100644
>> --- a/tools/perf/util/data.c
>> +++ b/tools/perf/util/data.c
>> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
>> return -1;
>>
>> ret = open_file(data);
>> + if (!ret && perf_data__is_dir(data)) {
>> + if (perf_data__is_read(data))
>> + ret = perf_data__open_dir(data);
>> + }
>
> perf_data__open_dir is also called from perf_session__new
> is it called twice?
It is not called twice. It is in different branches.
This one is for write and the other one is for read.
Alexei
>
> jirka
>
>>
>> /* Cleanup whatever we managed to create so far. */
>> if (ret && perf_data__is_write(data))
>> --
>> 2.24.1
>>
>
On Mon, Oct 26, 2020 at 08:47:06PM +0300, Alexey Budankov wrote:
>
> On 24.10.2020 18:43, Jiri Olsa wrote:
> > On Wed, Oct 21, 2020 at 06:57:53PM +0300, Alexey Budankov wrote:
> >>
> >> Open files located at trace data directory in case read access
> >> mode is requested. File are opened and its fds assigned to
> >> perf_data dir files especially for loading data directories
> >> content in perf report mode.
> >>
> >> Signed-off-by: Alexey Budankov <[email protected]>
> >> ---
> >> tools/perf/util/data.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> >> index c47aa34fdc0a..6ad61ac6ba67 100644
> >> --- a/tools/perf/util/data.c
> >> +++ b/tools/perf/util/data.c
> >> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
> >> return -1;
> >>
> >> ret = open_file(data);
> >> + if (!ret && perf_data__is_dir(data)) {
> >> + if (perf_data__is_read(data))
> >> + ret = perf_data__open_dir(data);
> >> + }
> >
> > perf_data__open_dir is also called from perf_session__new
> > is it called twice?
>
> It is not called twice. It is in different branches.
> This one is for write and the other one is for read.
hum, is that right?
# ./perf record --threads
^C[ perf record: Woken up 15 times to write data ]
[ perf record: Captured and wrote 1.421 MB perf.data (515 samples) ]
# gdb ./perf
GNU gdb (GDB) Fedora 9.1-6.fc32
(gdb) b perf_data__open_dir
Breakpoint 1 at 0x5b4753: file util/data.c, line 72.
(gdb) r --no-pager report --stdio
Starting program: /home/jolsa/kernel/linux-perf/tools/perf/perf --no-pager report --stdio
Breakpoint 1, perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
72 {
(gdb) bt
#0 perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
#1 0x00000000005b538d in open_dir (data=0x7fffffffb0e0) at util/data.c:326
#2 0x00000000005b546d in perf_data__open (data=0x7fffffffb0e0) at util/data.c:351
#3 0x00000000005627e8 in perf_session__new (data=0x7fffffffb0e0, repipe=false, tool=0x7fffffffb220) at util/session.c:210
#4 0x000000000045a572 in cmd_report (argc=0, argv=0x7fffffffd7a8) at builtin-report.c:1372
#5 0x00000000004f49ec in run_builtin (p=0xaadab0 <commands+240>, argc=2, argv=0x7fffffffd7a8) at perf.c:312
#6 0x00000000004f4c59 in handle_internal_command (argc=2, argv=0x7fffffffd7a8) at perf.c:364
#7 0x00000000004f4da0 in run_argv (argcp=0x7fffffffd5ec, argv=0x7fffffffd5e0) at perf.c:408
#8 0x00000000004f516c in main (argc=2, argv=0x7fffffffd7a8) at perf.c:538
(gdb) c
Continuing.
Breakpoint 1, perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
72 {
(gdb) bt
#0 perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
#1 0x0000000000562883 in perf_session__new (data=0x7fffffffb0e0, repipe=false, tool=0x7fffffffb220) at util/session.c:234
#2 0x000000000045a572 in cmd_report (argc=0, argv=0x7fffffffd7a8) at builtin-report.c:1372
#3 0x00000000004f49ec in run_builtin (p=0xaadab0 <commands+240>, argc=2, argv=0x7fffffffd7a8) at perf.c:312
#4 0x00000000004f4c59 in handle_internal_command (argc=2, argv=0x7fffffffd7a8) at perf.c:364
#5 0x00000000004f4da0 in run_argv (argcp=0x7fffffffd5ec, argv=0x7fffffffd5e0) at perf.c:408
#6 0x00000000004f516c in main (argc=2, argv=0x7fffffffd7a8) at perf.c:538
AFAICS the second (current) call to perf_data__open_dir will
do the job, because the call you added still does not see
directory with proper version and will bail out on call to
perf_data__is_single_file
perf_session__open call will read headers and update dir version
so the current perf_data__open_dir will open the directory
jirka
On 27.10.2020 14:59, Jiri Olsa wrote:
> On Mon, Oct 26, 2020 at 08:47:06PM +0300, Alexey Budankov wrote:
>>
>> On 24.10.2020 18:43, Jiri Olsa wrote:
>>> On Wed, Oct 21, 2020 at 06:57:53PM +0300, Alexey Budankov wrote:
>>>>
>>>> Open files located at trace data directory in case read access
>>>> mode is requested. File are opened and its fds assigned to
>>>> perf_data dir files especially for loading data directories
>>>> content in perf report mode.
>>>>
>>>> Signed-off-by: Alexey Budankov <[email protected]>
>>>> ---
>>>> tools/perf/util/data.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
>>>> index c47aa34fdc0a..6ad61ac6ba67 100644
>>>> --- a/tools/perf/util/data.c
>>>> +++ b/tools/perf/util/data.c
>>>> @@ -321,6 +321,10 @@ static int open_dir(struct perf_data *data)
>>>> return -1;
>>>>
>>>> ret = open_file(data);
>>>> + if (!ret && perf_data__is_dir(data)) {
>>>> + if (perf_data__is_read(data))
>>>> + ret = perf_data__open_dir(data);
>>>> + }
>>>
>>> perf_data__open_dir is also called from perf_session__new
>>> is it called twice?
>>
>> It is not called twice. It is in different branches.
>> This one is for write and the other one is for read.
>
> hum, is that right?
>
> # ./perf record --threads
> ^C[ perf record: Woken up 15 times to write data ]
> [ perf record: Captured and wrote 1.421 MB perf.data (515 samples) ]
>
> # gdb ./perf
> GNU gdb (GDB) Fedora 9.1-6.fc32
>
> (gdb) b perf_data__open_dir
> Breakpoint 1 at 0x5b4753: file util/data.c, line 72.
>
> (gdb) r --no-pager report --stdio
> Starting program: /home/jolsa/kernel/linux-perf/tools/perf/perf --no-pager report --stdio
>
> Breakpoint 1, perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
> 72 {
> (gdb) bt
> #0 perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
> #1 0x00000000005b538d in open_dir (data=0x7fffffffb0e0) at util/data.c:326
> #2 0x00000000005b546d in perf_data__open (data=0x7fffffffb0e0) at util/data.c:351
> #3 0x00000000005627e8 in perf_session__new (data=0x7fffffffb0e0, repipe=false, tool=0x7fffffffb220) at util/session.c:210
> #4 0x000000000045a572 in cmd_report (argc=0, argv=0x7fffffffd7a8) at builtin-report.c:1372
> #5 0x00000000004f49ec in run_builtin (p=0xaadab0 <commands+240>, argc=2, argv=0x7fffffffd7a8) at perf.c:312
> #6 0x00000000004f4c59 in handle_internal_command (argc=2, argv=0x7fffffffd7a8) at perf.c:364
> #7 0x00000000004f4da0 in run_argv (argcp=0x7fffffffd5ec, argv=0x7fffffffd5e0) at perf.c:408
> #8 0x00000000004f516c in main (argc=2, argv=0x7fffffffd7a8) at perf.c:538
>
> (gdb) c
> Continuing.
>
> Breakpoint 1, perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
> 72 {
> (gdb) bt
> #0 perf_data__open_dir (data=0x7fffffffb0e0) at util/data.c:72
> #1 0x0000000000562883 in perf_session__new (data=0x7fffffffb0e0, repipe=false, tool=0x7fffffffb220) at util/session.c:234
> #2 0x000000000045a572 in cmd_report (argc=0, argv=0x7fffffffd7a8) at builtin-report.c:1372
> #3 0x00000000004f49ec in run_builtin (p=0xaadab0 <commands+240>, argc=2, argv=0x7fffffffd7a8) at perf.c:312
> #4 0x00000000004f4c59 in handle_internal_command (argc=2, argv=0x7fffffffd7a8) at perf.c:364
> #5 0x00000000004f4da0 in run_argv (argcp=0x7fffffffd5ec, argv=0x7fffffffd5e0) at perf.c:408
> #6 0x00000000004f516c in main (argc=2, argv=0x7fffffffd7a8) at perf.c:538
>
>
> AFAICS the second (current) call to perf_data__open_dir will
> do the job, because the call you added still does not see
> directory with proper version and will bail out on call to
> perf_data__is_single_file
>
> perf_session__open call will read headers and update dir version
> so the current perf_data__open_dir will open the directory
Tested once again. Now it looks like this patch is redundant
since dir is already opened by the existing code.
Thanks for pointing this out!
Alexei
>
> jirka
>