All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 14:00 [PATCH] perf tools: fix: Force backward ring buffer mapped readonly Wang Nan
@ 2017-10-11 13:16 ` Liang, Kan
  2017-10-12  1:03   ` Wangnan (F)
  0 siblings, 1 reply; 9+ messages in thread
From: Liang, Kan @ 2017-10-11 13:16 UTC (permalink / raw)
  To: Wang Nan, linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan


> perf record's --overwrite option doesn't work as we expect.
> For example:
> 
>     $ ~/linux/tools/perf$ sudo rm ./perf.data*
>     rm: cannot remove './perf.data*': No such file or directory
>     : ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:* -g --
> overwrite \
> 	    				--switch-output=1s --tail-synthesize
> dd if=/dev/zero of=/dev/null
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221460002 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221460102 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221460202 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221460302 ]
>     ^C[ perf record: Woken up 1 times to write data ]
>     2066346+0 records in
>     2066346+0 records out
>     1057969152 bytes (1.1 GB, 1009 MiB) copied, 4.25983 s, 248 MB/s
>     [ perf record: Dump perf.data.2017101221460332 ]
>     [ perf record: Captured and wrote 0.034 MB perf.data.<timestamp> ]
> 
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460002 |
> head -n 1
>     dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460102 |
> head -n 1
>     dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460202 |
> head -n 1
>     dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460302 |
> head -n 1
>     dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
> 
> In the above example we get same records from the backward ring buffer
> all the time. Overwriting is not triggered.
> 
> This commit maps backward ring buffers readonly, make it overwritable.
> It is safe because we assume backward ring buffer always overwritable
> in other part of code.
> 
> After applying this patch:
> 
>     $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:* -g --
> overwrite \
> 	    			   --switch-output=1s --tail-synthesize   dd
> if=/dev/zero of=/dev/null
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221540350 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221540451 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221540551 ]
>     [ perf record: dump data: Woken up 1 times ]
>     [ perf record: Dump perf.data.2017101221540651 ]
>     ^C[ perf record: Woken up 1 times to write data ]
>     1604606+0 records in
>     1604605+0 records out
>     821557760 bytes (822 MB, 783 MiB) copied, 4.42736 s, 186 MB/s
>     [ perf record: Dump perf.data.2017101221540700 ]
>     [ perf record: Captured and wrote 0.034 MB perf.data.<timestamp> ]
> 
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540350 |
> head -n 1
>     dd  5126 [003] 183074.104581:  raw_syscalls:sys_exit: NR 0 = 512
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540451 |
> head -n 1
>     dd  5126 [003] 183075.106496:  raw_syscalls:sys_exit: NR 1 = 512
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540551 |
> head -n 1
>     dd  5126 [003] 183076.108093: raw_syscalls:sys_enter: NR 1 (1, af8000, 200,
> 871, 0, af8060)
>     $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540651 |
> head -n 1
>     dd  5126 [003] 183077.109676:  raw_syscalls:sys_exit: NR 1 = 512
> 
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Liang Kan <kan.liang@intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Li Zefan <lizefan@huawei.com>
> ---
>  tools/perf/util/evlist.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c6c891e..a86b0d2 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist *evlist
> __maybe_unused,
>  }
> 
>  static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> -				       struct mmap_params *mp, int cpu_idx,
> +				       struct mmap_params *_mp, int cpu_idx,
>  				       int thread, int *_output, int
> *_output_backward)
>  {
>  	struct perf_evsel *evsel;
>  	int revent;
>  	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
> +	struct mmap_params *mp = _mp;
> +	struct mmap_params backward_mp;
> 
>  	evlist__for_each_entry(evlist, evsel) {
>  		struct perf_mmap *maps = evlist->mmap;
> @@ -815,6 +817,9 @@ static int perf_evlist__mmap_per_evsel(struct
> perf_evlist *evlist, int idx,
>  		if (evsel->attr.write_backward) {
>  			output = _output_backward;
>  			maps = evlist->backward_mmap;
> +			backward_mp = *mp;
> +			backward_mp.prot &= ~PROT_WRITE;
> +			mp = &backward_mp;
> 
>  			if (!maps) {
>  				maps = perf_evlist__alloc_mmap(evlist);

So it's trying to support per-event overwrite.
How about the global --overwrite option?
I think we should use opts->overwrite to replace the hard code 'false' for
perf_evlist__mmap_ex as well.

Thanks,
Kan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-11 13:16 ` Liang, Kan
@ 2017-10-12  1:03   ` Wangnan (F)
  2017-10-12 12:56     ` Liang, Kan
  0 siblings, 1 reply; 9+ messages in thread
From: Wangnan (F) @ 2017-10-12  1:03 UTC (permalink / raw)
  To: Liang, Kan, linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan



On 2017/10/11 21:16, Liang, Kan wrote:
>> perf record's --overwrite option doesn't work as we expect.
>> For example:

[SNIP]

>>
>> In the above example we get same records from the backward ring buffer
>> all the time. Overwriting is not triggered.
>>
>> This commit maps backward ring buffers readonly, make it overwritable.
>> It is safe because we assume backward ring buffer always overwritable
>> in other part of code.
>>
>> After applying this patch:
>>
>>      $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:* -g --
>> overwrite \
>> 	    			   --switch-output=1s --tail-synthesize   dd
>> if=/dev/zero of=/dev/null

[SNIP]

>>
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Liang Kan <kan.liang@intel.com>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Li Zefan <lizefan@huawei.com>
>> ---
>>   tools/perf/util/evlist.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index c6c891e..a86b0d2 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist *evlist
>> __maybe_unused,
>>   }
>>
>>   static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>> -				       struct mmap_params *mp, int cpu_idx,
>> +				       struct mmap_params *_mp, int cpu_idx,
>>   				       int thread, int *_output, int
>> *_output_backward)
>>   {
>>   	struct perf_evsel *evsel;
>>   	int revent;
>>   	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>> +	struct mmap_params *mp = _mp;
>> +	struct mmap_params backward_mp;
>>
>>   	evlist__for_each_entry(evlist, evsel) {
>>   		struct perf_mmap *maps = evlist->mmap;
>> @@ -815,6 +817,9 @@ static int perf_evlist__mmap_per_evsel(struct
>> perf_evlist *evlist, int idx,
>>   		if (evsel->attr.write_backward) {
>>   			output = _output_backward;
>>   			maps = evlist->backward_mmap;
>> +			backward_mp = *mp;
>> +			backward_mp.prot &= ~PROT_WRITE;
>> +			mp = &backward_mp;
>>
>>   			if (!maps) {
>>   				maps = perf_evlist__alloc_mmap(evlist);
> So it's trying to support per-event overwrite.
> How about the global --overwrite option?

Not only the per-event overwrite. See the example above. The overall 
--overwrite
option is also respected. In perf_evsel__config, per-event evsel 
'backward' setting
is set based on overall '--overwrite' and per-event '/overwrite/' setting.

> I think we should use opts->overwrite to replace the hard code 'false' for
> perf_evlist__mmap_ex as well.
>
> Thanks,
> Kan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12  1:03   ` Wangnan (F)
@ 2017-10-12 12:56     ` Liang, Kan
  2017-10-12 14:35       ` Wangnan (F)
  0 siblings, 1 reply; 9+ messages in thread
From: Liang, Kan @ 2017-10-12 12:56 UTC (permalink / raw)
  To: Wangnan (F), linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan

> 
> On 2017/10/11 21:16, Liang, Kan wrote:
> >> perf record's --overwrite option doesn't work as we expect.
> >> For example:
> 
> [SNIP]
> 
> >>
> >> In the above example we get same records from the backward ring
> >> buffer all the time. Overwriting is not triggered.
> >>
> >> This commit maps backward ring buffers readonly, make it overwritable.
> >> It is safe because we assume backward ring buffer always overwritable
> >> in other part of code.
> >>
> >> After applying this patch:
> >>
> >>      $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:*
> >> -g -- overwrite \
> >> 	    			   --switch-output=1s --tail-synthesize   dd
> >> if=/dev/zero of=/dev/null
> 
> [SNIP]
> 
> >>
> >>
> >> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >> Cc: Liang Kan <kan.liang@intel.com>
> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Ingo Molnar <mingo@kernel.org>
> >> Cc: Jiri Olsa <jolsa@kernel.org>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Cc: Adrian Hunter <adrian.hunter@intel.com>
> >> Cc: Andi Kleen <ak@linux.intel.com>
> >> Cc: Li Zefan <lizefan@huawei.com>
> >> ---
> >>   tools/perf/util/evlist.c | 7 ++++++-
> >>   1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >> index c6c891e..a86b0d2 100644
> >> --- a/tools/perf/util/evlist.c
> >> +++ b/tools/perf/util/evlist.c
> >> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
> >> *evlist __maybe_unused,
> >>   }
> >>
> >>   static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> >> -				       struct mmap_params *mp, int cpu_idx,
> >> +				       struct mmap_params *_mp, int cpu_idx,
> >>   				       int thread, int *_output, int
> >> *_output_backward)
> >>   {
> >>   	struct perf_evsel *evsel;
> >>   	int revent;
> >>   	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
> >> +	struct mmap_params *mp = _mp;
> >> +	struct mmap_params backward_mp;
> >>
> >>   	evlist__for_each_entry(evlist, evsel) {
> >>   		struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
> @@ static
> >> int perf_evlist__mmap_per_evsel(struct
> >> perf_evlist *evlist, int idx,
> >>   		if (evsel->attr.write_backward) {
> >>   			output = _output_backward;
> >>   			maps = evlist->backward_mmap;
> >> +			backward_mp = *mp;
> >> +			backward_mp.prot &= ~PROT_WRITE;
> >> +			mp = &backward_mp;
> >>
> >>   			if (!maps) {
> >>   				maps = perf_evlist__alloc_mmap(evlist);
> > So it's trying to support per-event overwrite.
> > How about the global --overwrite option?
> 
> Not only the per-event overwrite. See the example above. The overall --
> overwrite option is also respected. In perf_evsel__config, per-event evsel
> 'backward' setting is set based on overall '--overwrite' and per-event
> '/overwrite/' setting.

But how about evlist->overwrite? I think it still keeps the wrong setting.
The overwrite is implicitly applied. Some settings are inconsistent.

Is there any drawback if you use opts->overwrite for perf_evlist__mmap_ex?

Thanks,
Kan
> 
> > I think we should use opts->overwrite to replace the hard code 'false'
> > for perf_evlist__mmap_ex as well.
> >
> > Thanks,
> > Kan
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
@ 2017-10-12 14:00 Wang Nan
  2017-10-11 13:16 ` Liang, Kan
  0 siblings, 1 reply; 9+ messages in thread
From: Wang Nan @ 2017-10-12 14:00 UTC (permalink / raw)
  To: linux-kernel, kan.liang, acme
  Cc: Wang Nan, Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Adrian Hunter, Andi Kleen, Li Zefan

perf record's --overwrite option doesn't work as we expect.
For example:

    $ ~/linux/tools/perf$ sudo rm ./perf.data*
    rm: cannot remove './perf.data*': No such file or directory
    : ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:* -g --overwrite \
	    				--switch-output=1s --tail-synthesize   dd if=/dev/zero of=/dev/null
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221460002 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221460102 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221460202 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221460302 ]
    ^C[ perf record: Woken up 1 times to write data ]
    2066346+0 records in
    2066346+0 records out
    1057969152 bytes (1.1 GB, 1009 MiB) copied, 4.25983 s, 248 MB/s
    [ perf record: Dump perf.data.2017101221460332 ]
    [ perf record: Captured and wrote 0.034 MB perf.data.<timestamp> ]

    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460002 | head -n 1
    dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460102 | head -n 1
    dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460202 | head -n 1
    dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221460302 | head -n 1
    dd  3918 [006] 182589.668954:  raw_syscalls:sys_exit: NR 59 = 0

In the above example we get same records from the backward ring buffer
all the time. Overwriting is not triggered.

This commit maps backward ring buffers readonly, make it overwritable.
It is safe because we assume backward ring buffer always overwritable
in other part of code.

After applying this patch:

    $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:* -g --overwrite \
	    			   --switch-output=1s --tail-synthesize   dd if=/dev/zero of=/dev/null
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221540350 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221540451 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221540551 ]
    [ perf record: dump data: Woken up 1 times ]
    [ perf record: Dump perf.data.2017101221540651 ]
    ^C[ perf record: Woken up 1 times to write data ]
    1604606+0 records in
    1604605+0 records out
    821557760 bytes (822 MB, 783 MiB) copied, 4.42736 s, 186 MB/s
    [ perf record: Dump perf.data.2017101221540700 ]
    [ perf record: Captured and wrote 0.034 MB perf.data.<timestamp> ]

    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540350 | head -n 1
    dd  5126 [003] 183074.104581:  raw_syscalls:sys_exit: NR 0 = 512
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540451 | head -n 1
    dd  5126 [003] 183075.106496:  raw_syscalls:sys_exit: NR 1 = 512
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540551 | head -n 1
    dd  5126 [003] 183076.108093: raw_syscalls:sys_enter: NR 1 (1, af8000, 200, 871, 0, af8060)
    $ ~/linux/tools/perf$ sudo ./perf script -i ./perf.data.2017101221540651 | head -n 1
    dd  5126 [003] 183077.109676:  raw_syscalls:sys_exit: NR 1 = 512


Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Liang Kan <kan.liang@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Li Zefan <lizefan@huawei.com>
---
 tools/perf/util/evlist.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c6c891e..a86b0d2 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist *evlist __maybe_unused,
 }
 
 static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
-				       struct mmap_params *mp, int cpu_idx,
+				       struct mmap_params *_mp, int cpu_idx,
 				       int thread, int *_output, int *_output_backward)
 {
 	struct perf_evsel *evsel;
 	int revent;
 	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
+	struct mmap_params *mp = _mp;
+	struct mmap_params backward_mp;
 
 	evlist__for_each_entry(evlist, evsel) {
 		struct perf_mmap *maps = evlist->mmap;
@@ -815,6 +817,9 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
 		if (evsel->attr.write_backward) {
 			output = _output_backward;
 			maps = evlist->backward_mmap;
+			backward_mp = *mp;
+			backward_mp.prot &= ~PROT_WRITE;
+			mp = &backward_mp;
 
 			if (!maps) {
 				maps = perf_evlist__alloc_mmap(evlist);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 12:56     ` Liang, Kan
@ 2017-10-12 14:35       ` Wangnan (F)
  2017-10-12 14:45         ` Liang, Kan
  0 siblings, 1 reply; 9+ messages in thread
From: Wangnan (F) @ 2017-10-12 14:35 UTC (permalink / raw)
  To: Liang, Kan, linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan



On 2017/10/12 20:56, Liang, Kan wrote:
>> On 2017/10/11 21:16, Liang, Kan wrote:
>>>> perf record's --overwrite option doesn't work as we expect.
>>>> For example:
>> [SNIP]
>>
>>>> In the above example we get same records from the backward ring
>>>> buffer all the time. Overwriting is not triggered.
>>>>
>>>> This commit maps backward ring buffers readonly, make it overwritable.
>>>> It is safe because we assume backward ring buffer always overwritable
>>>> in other part of code.
>>>>
>>>> After applying this patch:
>>>>
>>>>       $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:*
>>>> -g -- overwrite \
>>>> 	    			   --switch-output=1s --tail-synthesize   dd
>>>> if=/dev/zero of=/dev/null
>> [SNIP]
>>
>>>>
>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>> Cc: Liang Kan <kan.liang@intel.com>
>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>> Cc: Li Zefan <lizefan@huawei.com>
>>>> ---
>>>>    tools/perf/util/evlist.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index c6c891e..a86b0d2 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
>>>> *evlist __maybe_unused,
>>>>    }
>>>>
>>>>    static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>>>> -				       struct mmap_params *mp, int cpu_idx,
>>>> +				       struct mmap_params *_mp, int cpu_idx,
>>>>    				       int thread, int *_output, int
>>>> *_output_backward)
>>>>    {
>>>>    	struct perf_evsel *evsel;
>>>>    	int revent;
>>>>    	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>>>> +	struct mmap_params *mp = _mp;
>>>> +	struct mmap_params backward_mp;
>>>>
>>>>    	evlist__for_each_entry(evlist, evsel) {
>>>>    		struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
>> @@ static
>>>> int perf_evlist__mmap_per_evsel(struct
>>>> perf_evlist *evlist, int idx,
>>>>    		if (evsel->attr.write_backward) {
>>>>    			output = _output_backward;
>>>>    			maps = evlist->backward_mmap;
>>>> +			backward_mp = *mp;
>>>> +			backward_mp.prot &= ~PROT_WRITE;
>>>> +			mp = &backward_mp;
>>>>
>>>>    			if (!maps) {
>>>>    				maps = perf_evlist__alloc_mmap(evlist);
>>> So it's trying to support per-event overwrite.
>>> How about the global --overwrite option?
>> Not only the per-event overwrite. See the example above. The overall --
>> overwrite option is also respected. In perf_evsel__config, per-event evsel
>> 'backward' setting is set based on overall '--overwrite' and per-event
>> '/overwrite/' setting.
> But how about evlist->overwrite? I think it still keeps the wrong setting.
> The overwrite is implicitly applied. Some settings are inconsistent.
>
> Is there any drawback if you use opts->overwrite for perf_evlist__mmap_ex?

We will always face such inconsistency, because we have
an /no-overwrite/ option which can be set per-evsel.
Setting evlist->overwrite won't make things more consistent,
because in a evlist, different evsel can have different
overwrite setting. A simple solution is making evlist
non-overwrite by default, and watch all overwrite evsels
a special cases. Then we have only 2 cases to consider:

1. overwrite evsel in a non-overwrite evlist.
2. non-overwrite evsel in a non-overwrite evlist.

If we reset evlist->overwrite according to --overwrite, we
will have 4 cases to consider:

1. overwrite evsel in a overwrite evlist.
2. non-overwrite evsel in a overwrite evlist.
3. overwrite evsel in a non-overwrite evlist.
4. non-overwrite evsel in a non-overwrite evlist.

The real problem is: there's 'overwrite' and 'backward'
concepts in our code, but these two concepts are neither
independent nor identical.

Thank you.


> Thanks,
> Kan
>>> I think we should use opts->overwrite to replace the hard code 'false'
>>> for perf_evlist__mmap_ex as well.
>>>
>>> Thanks,
>>> Kan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 14:35       ` Wangnan (F)
@ 2017-10-12 14:45         ` Liang, Kan
  2017-10-12 14:46           ` Wangnan (F)
  0 siblings, 1 reply; 9+ messages in thread
From: Liang, Kan @ 2017-10-12 14:45 UTC (permalink / raw)
  To: Wangnan (F), linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan


> On 2017/10/12 20:56, Liang, Kan wrote:
> >> On 2017/10/11 21:16, Liang, Kan wrote:
> >>>> perf record's --overwrite option doesn't work as we expect.
> >>>> For example:
> >> [SNIP]
> >>
> >>>> In the above example we get same records from the backward ring
> >>>> buffer all the time. Overwriting is not triggered.
> >>>>
> >>>> This commit maps backward ring buffers readonly, make it overwritable.
> >>>> It is safe because we assume backward ring buffer always overwritable
> >>>> in other part of code.
> >>>>
> >>>> After applying this patch:
> >>>>
> >>>>       $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:*
> >>>> -g -- overwrite \
> >>>> 	    			   --switch-output=1s --tail-synthesize   dd
> >>>> if=/dev/zero of=/dev/null
> >> [SNIP]
> >>
> >>>>
> >>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>>> Cc: Liang Kan <kan.liang@intel.com>
> >>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>> Cc: Ingo Molnar <mingo@kernel.org>
> >>>> Cc: Jiri Olsa <jolsa@kernel.org>
> >>>> Cc: Namhyung Kim <namhyung@kernel.org>
> >>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> >>>> Cc: Andi Kleen <ak@linux.intel.com>
> >>>> Cc: Li Zefan <lizefan@huawei.com>
> >>>> ---
> >>>>    tools/perf/util/evlist.c | 7 ++++++-
> >>>>    1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>> index c6c891e..a86b0d2 100644
> >>>> --- a/tools/perf/util/evlist.c
> >>>> +++ b/tools/perf/util/evlist.c
> >>>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
> >>>> *evlist __maybe_unused,
> >>>>    }
> >>>>
> >>>>    static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int
> idx,
> >>>> -				       struct mmap_params *mp, int cpu_idx,
> >>>> +				       struct mmap_params *_mp, int cpu_idx,
> >>>>    				       int thread, int *_output, int
> >>>> *_output_backward)
> >>>>    {
> >>>>    	struct perf_evsel *evsel;
> >>>>    	int revent;
> >>>>    	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
> >>>> +	struct mmap_params *mp = _mp;
> >>>> +	struct mmap_params backward_mp;
> >>>>
> >>>>    	evlist__for_each_entry(evlist, evsel) {
> >>>>    		struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
> >> @@ static
> >>>> int perf_evlist__mmap_per_evsel(struct
> >>>> perf_evlist *evlist, int idx,
> >>>>    		if (evsel->attr.write_backward) {
> >>>>    			output = _output_backward;
> >>>>    			maps = evlist->backward_mmap;
> >>>> +			backward_mp = *mp;
> >>>> +			backward_mp.prot &= ~PROT_WRITE;
> >>>> +			mp = &backward_mp;
> >>>>
> >>>>    			if (!maps) {
> >>>>    				maps = perf_evlist__alloc_mmap(evlist);
> >>> So it's trying to support per-event overwrite.
> >>> How about the global --overwrite option?
> >> Not only the per-event overwrite. See the example above. The overall --
> >> overwrite option is also respected. In perf_evsel__config, per-event evsel
> >> 'backward' setting is set based on overall '--overwrite' and per-event
> >> '/overwrite/' setting.
> > But how about evlist->overwrite? I think it still keeps the wrong setting.
> > The overwrite is implicitly applied. Some settings are inconsistent.
> >
> > Is there any drawback if you use opts->overwrite for
> perf_evlist__mmap_ex?
> 
> We will always face such inconsistency, because we have
> an /no-overwrite/ option which can be set per-evsel.
> Setting evlist->overwrite won't make things more consistent,
> because in a evlist, different evsel can have different
> overwrite setting. A simple solution is making evlist
> non-overwrite by default, and watch all overwrite evsels
> a special cases. Then we have only 2 cases to consider:
> 
> 1. overwrite evsel in a non-overwrite evlist.
> 2. non-overwrite evsel in a non-overwrite evlist.
>

If evlist->overwrite is always non-overwrite, why not remove it?
 
> If we reset evlist->overwrite according to --overwrite, we
> will have 4 cases to consider:
> 
> 1. overwrite evsel in a overwrite evlist.
> 2. non-overwrite evsel in a overwrite evlist.
> 3. overwrite evsel in a non-overwrite evlist.
> 4. non-overwrite evsel in a non-overwrite evlist.
> 
> The real problem is: there's 'overwrite' and 'backward'
> concepts in our code, but these two concepts are neither
> independent nor identical.
> 
> Thank you.
> 
> 
> > Thanks,
> > Kan
> >>> I think we should use opts->overwrite to replace the hard code 'false'
> >>> for perf_evlist__mmap_ex as well.
> >>>
> >>> Thanks,
> >>> Kan
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 14:45         ` Liang, Kan
@ 2017-10-12 14:46           ` Wangnan (F)
  2017-10-12 14:52             ` Wangnan (F)
  0 siblings, 1 reply; 9+ messages in thread
From: Wangnan (F) @ 2017-10-12 14:46 UTC (permalink / raw)
  To: Liang, Kan, linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan



On 2017/10/12 22:45, Liang, Kan wrote:
>> On 2017/10/12 20:56, Liang, Kan wrote:
>>>> On 2017/10/11 21:16, Liang, Kan wrote:
>>>>>> perf record's --overwrite option doesn't work as we expect.
>>>>>> For example:
>>>> [SNIP]
>>>>
>>>>>> In the above example we get same records from the backward ring
>>>>>> buffer all the time. Overwriting is not triggered.
>>>>>>
>>>>>> This commit maps backward ring buffers readonly, make it overwritable.
>>>>>> It is safe because we assume backward ring buffer always overwritable
>>>>>> in other part of code.
>>>>>>
>>>>>> After applying this patch:
>>>>>>
>>>>>>        $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e raw_syscalls:*
>>>>>> -g -- overwrite \
>>>>>> 	    			   --switch-output=1s --tail-synthesize   dd
>>>>>> if=/dev/zero of=/dev/null
>>>> [SNIP]
>>>>
>>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>>>> Cc: Liang Kan <kan.liang@intel.com>
>>>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>>> Cc: Li Zefan <lizefan@huawei.com>
>>>>>> ---
>>>>>>     tools/perf/util/evlist.c | 7 ++++++-
>>>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>>> index c6c891e..a86b0d2 100644
>>>>>> --- a/tools/perf/util/evlist.c
>>>>>> +++ b/tools/perf/util/evlist.c
>>>>>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
>>>>>> *evlist __maybe_unused,
>>>>>>     }
>>>>>>
>>>>>>     static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int
>> idx,
>>>>>> -				       struct mmap_params *mp, int cpu_idx,
>>>>>> +				       struct mmap_params *_mp, int cpu_idx,
>>>>>>     				       int thread, int *_output, int
>>>>>> *_output_backward)
>>>>>>     {
>>>>>>     	struct perf_evsel *evsel;
>>>>>>     	int revent;
>>>>>>     	int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>>>>>> +	struct mmap_params *mp = _mp;
>>>>>> +	struct mmap_params backward_mp;
>>>>>>
>>>>>>     	evlist__for_each_entry(evlist, evsel) {
>>>>>>     		struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
>>>> @@ static
>>>>>> int perf_evlist__mmap_per_evsel(struct
>>>>>> perf_evlist *evlist, int idx,
>>>>>>     		if (evsel->attr.write_backward) {
>>>>>>     			output = _output_backward;
>>>>>>     			maps = evlist->backward_mmap;
>>>>>> +			backward_mp = *mp;
>>>>>> +			backward_mp.prot &= ~PROT_WRITE;
>>>>>> +			mp = &backward_mp;
>>>>>>
>>>>>>     			if (!maps) {
>>>>>>     				maps = perf_evlist__alloc_mmap(evlist);
>>>>> So it's trying to support per-event overwrite.
>>>>> How about the global --overwrite option?
>>>> Not only the per-event overwrite. See the example above. The overall --
>>>> overwrite option is also respected. In perf_evsel__config, per-event evsel
>>>> 'backward' setting is set based on overall '--overwrite' and per-event
>>>> '/overwrite/' setting.
>>> But how about evlist->overwrite? I think it still keeps the wrong setting.
>>> The overwrite is implicitly applied. Some settings are inconsistent.
>>>
>>> Is there any drawback if you use opts->overwrite for
>> perf_evlist__mmap_ex?
>>
>> We will always face such inconsistency, because we have
>> an /no-overwrite/ option which can be set per-evsel.
>> Setting evlist->overwrite won't make things more consistent,
>> because in a evlist, different evsel can have different
>> overwrite setting. A simple solution is making evlist
>> non-overwrite by default, and watch all overwrite evsels
>> a special cases. Then we have only 2 cases to consider:
>>
>> 1. overwrite evsel in a non-overwrite evlist.
>> 2. non-overwrite evsel in a non-overwrite evlist.
>>
> If evlist->overwrite is always non-overwrite, why not remove it?
>   

Some testcases require it.

Thank you.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 14:46           ` Wangnan (F)
@ 2017-10-12 14:52             ` Wangnan (F)
  2017-10-12 15:11               ` Liang, Kan
  0 siblings, 1 reply; 9+ messages in thread
From: Wangnan (F) @ 2017-10-12 14:52 UTC (permalink / raw)
  To: Liang, Kan, linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan



On 2017/10/12 22:46, Wangnan (F) wrote:
>
>
> On 2017/10/12 22:45, Liang, Kan wrote:
>>> On 2017/10/12 20:56, Liang, Kan wrote:
>>>>> On 2017/10/11 21:16, Liang, Kan wrote:
>>>>>>> perf record's --overwrite option doesn't work as we expect.
>>>>>>> For example:
>>>>> [SNIP]
>>>>>
>>>>>>> In the above example we get same records from the backward ring
>>>>>>> buffer all the time. Overwriting is not triggered.
>>>>>>>
>>>>>>> This commit maps backward ring buffers readonly, make it 
>>>>>>> overwritable.
>>>>>>> It is safe because we assume backward ring buffer always 
>>>>>>> overwritable
>>>>>>> in other part of code.
>>>>>>>
>>>>>>> After applying this patch:
>>>>>>>
>>>>>>>        $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e 
>>>>>>> raw_syscalls:*
>>>>>>> -g -- overwrite \
>>>>>>>                        --switch-output=1s --tail-synthesize   dd
>>>>>>> if=/dev/zero of=/dev/null
>>>>> [SNIP]
>>>>>
>>>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>>>>> Cc: Liang Kan <kan.liang@intel.com>
>>>>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>>>> Cc: Li Zefan <lizefan@huawei.com>
>>>>>>> ---
>>>>>>>     tools/perf/util/evlist.c | 7 ++++++-
>>>>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>>>> index c6c891e..a86b0d2 100644
>>>>>>> --- a/tools/perf/util/evlist.c
>>>>>>> +++ b/tools/perf/util/evlist.c
>>>>>>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct perf_evlist
>>>>>>> *evlist __maybe_unused,
>>>>>>>     }
>>>>>>>
>>>>>>>     static int perf_evlist__mmap_per_evsel(struct perf_evlist 
>>>>>>> *evlist, int
>>> idx,
>>>>>>> -                       struct mmap_params *mp, int cpu_idx,
>>>>>>> +                       struct mmap_params *_mp, int cpu_idx,
>>>>>>>                            int thread, int *_output, int
>>>>>>> *_output_backward)
>>>>>>>     {
>>>>>>>         struct perf_evsel *evsel;
>>>>>>>         int revent;
>>>>>>>         int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
>>>>>>> +    struct mmap_params *mp = _mp;
>>>>>>> +    struct mmap_params backward_mp;
>>>>>>>
>>>>>>>         evlist__for_each_entry(evlist, evsel) {
>>>>>>>             struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
>>>>> @@ static
>>>>>>> int perf_evlist__mmap_per_evsel(struct
>>>>>>> perf_evlist *evlist, int idx,
>>>>>>>             if (evsel->attr.write_backward) {
>>>>>>>                 output = _output_backward;
>>>>>>>                 maps = evlist->backward_mmap;
>>>>>>> +            backward_mp = *mp;
>>>>>>> +            backward_mp.prot &= ~PROT_WRITE;
>>>>>>> +            mp = &backward_mp;
>>>>>>>
>>>>>>>                 if (!maps) {
>>>>>>>                     maps = perf_evlist__alloc_mmap(evlist);
>>>>>> So it's trying to support per-event overwrite.
>>>>>> How about the global --overwrite option?
>>>>> Not only the per-event overwrite. See the example above. The 
>>>>> overall --
>>>>> overwrite option is also respected. In perf_evsel__config, 
>>>>> per-event evsel
>>>>> 'backward' setting is set based on overall '--overwrite' and 
>>>>> per-event
>>>>> '/overwrite/' setting.
>>>> But how about evlist->overwrite? I think it still keeps the wrong 
>>>> setting.
>>>> The overwrite is implicitly applied. Some settings are inconsistent.
>>>>
>>>> Is there any drawback if you use opts->overwrite for
>>> perf_evlist__mmap_ex?
>>>
>>> We will always face such inconsistency, because we have
>>> an /no-overwrite/ option which can be set per-evsel.
>>> Setting evlist->overwrite won't make things more consistent,
>>> because in a evlist, different evsel can have different
>>> overwrite setting. A simple solution is making evlist
>>> non-overwrite by default, and watch all overwrite evsels
>>> a special cases. Then we have only 2 cases to consider:
>>>
>>> 1. overwrite evsel in a non-overwrite evlist.
>>> 2. non-overwrite evsel in a non-overwrite evlist.
>>>
>> If evlist->overwrite is always non-overwrite, why not remove it?
>
> Some testcases require it.
>

Sorry. I think removing it is reasonable now, but we need to solve
the relationship between overwrite and backward first. I suggest remove
the whole 'backward' concept, and makes evsels backward if it is
overwrite. Is there any usecases that:
1. overwrite but not backward ring buffer: it will be unparsable after 
ring buffer full.
2. backward but not overwrite ring buffer: I don't see any advantage.

Thank you.

> Thank you.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] perf tools: fix: Force backward ring buffer mapped readonly
  2017-10-12 14:52             ` Wangnan (F)
@ 2017-10-12 15:11               ` Liang, Kan
  0 siblings, 0 replies; 9+ messages in thread
From: Liang, Kan @ 2017-10-12 15:11 UTC (permalink / raw)
  To: Wangnan (F), linux-kernel, acme
  Cc: Peter Zijlstra, Ingo Molnar, Jiri Olsa, Namhyung Kim,
	Alexander Shishkin, Hunter, Adrian, Andi Kleen, Li Zefan

> > On 2017/10/12 22:45, Liang, Kan wrote:
> >>> On 2017/10/12 20:56, Liang, Kan wrote:
> >>>>> On 2017/10/11 21:16, Liang, Kan wrote:
> >>>>>>> perf record's --overwrite option doesn't work as we expect.
> >>>>>>> For example:
> >>>>> [SNIP]
> >>>>>
> >>>>>>> In the above example we get same records from the backward ring
> >>>>>>> buffer all the time. Overwriting is not triggered.
> >>>>>>>
> >>>>>>> This commit maps backward ring buffers readonly, make it
> >>>>>>> overwritable.
> >>>>>>> It is safe because we assume backward ring buffer always
> >>>>>>> overwritable
> >>>>>>> in other part of code.
> >>>>>>>
> >>>>>>> After applying this patch:
> >>>>>>>
> >>>>>>>        $ ~/linux/tools/perf$ sudo ./perf record -m 4 -e
> >>>>>>> raw_syscalls:*
> >>>>>>> -g -- overwrite \
> >>>>>>>                        --switch-output=1s --tail-synthesize   dd
> >>>>>>> if=/dev/zero of=/dev/null
> >>>>> [SNIP]
> >>>>>
> >>>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>>>>>> Cc: Liang Kan <kan.liang@intel.com>
> >>>>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >>>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>>>>> Cc: Ingo Molnar <mingo@kernel.org>
> >>>>>>> Cc: Jiri Olsa <jolsa@kernel.org>
> >>>>>>> Cc: Namhyung Kim <namhyung@kernel.org>
> >>>>>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >>>>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
> >>>>>>> Cc: Andi Kleen <ak@linux.intel.com>
> >>>>>>> Cc: Li Zefan <lizefan@huawei.com>
> >>>>>>> ---
> >>>>>>>     tools/perf/util/evlist.c | 7 ++++++-
> >>>>>>>     1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>>>>> index c6c891e..a86b0d2 100644
> >>>>>>> --- a/tools/perf/util/evlist.c
> >>>>>>> +++ b/tools/perf/util/evlist.c
> >>>>>>> @@ -799,12 +799,14 @@ perf_evlist__should_poll(struct
> perf_evlist
> >>>>>>> *evlist __maybe_unused,
> >>>>>>>     }
> >>>>>>>
> >>>>>>>     static int perf_evlist__mmap_per_evsel(struct perf_evlist
> >>>>>>> *evlist, int
> >>> idx,
> >>>>>>> -                       struct mmap_params *mp, int cpu_idx,
> >>>>>>> +                       struct mmap_params *_mp, int cpu_idx,
> >>>>>>>                            int thread, int *_output, int
> >>>>>>> *_output_backward)
> >>>>>>>     {
> >>>>>>>         struct perf_evsel *evsel;
> >>>>>>>         int revent;
> >>>>>>>         int evlist_cpu = cpu_map__cpu(evlist->cpus, cpu_idx);
> >>>>>>> +    struct mmap_params *mp = _mp;
> >>>>>>> +    struct mmap_params backward_mp;
> >>>>>>>
> >>>>>>>         evlist__for_each_entry(evlist, evsel) {
> >>>>>>>             struct perf_mmap *maps = evlist->mmap; @@ -815,6 +817,9
> >>>>> @@ static
> >>>>>>> int perf_evlist__mmap_per_evsel(struct
> >>>>>>> perf_evlist *evlist, int idx,
> >>>>>>>             if (evsel->attr.write_backward) {
> >>>>>>>                 output = _output_backward;
> >>>>>>>                 maps = evlist->backward_mmap;
> >>>>>>> +            backward_mp = *mp;
> >>>>>>> +            backward_mp.prot &= ~PROT_WRITE;
> >>>>>>> +            mp = &backward_mp;
> >>>>>>>
> >>>>>>>                 if (!maps) {
> >>>>>>>                     maps = perf_evlist__alloc_mmap(evlist);
> >>>>>> So it's trying to support per-event overwrite.
> >>>>>> How about the global --overwrite option?
> >>>>> Not only the per-event overwrite. See the example above. The
> >>>>> overall --
> >>>>> overwrite option is also respected. In perf_evsel__config,
> >>>>> per-event evsel
> >>>>> 'backward' setting is set based on overall '--overwrite' and
> >>>>> per-event
> >>>>> '/overwrite/' setting.
> >>>> But how about evlist->overwrite? I think it still keeps the wrong
> >>>> setting.
> >>>> The overwrite is implicitly applied. Some settings are inconsistent.
> >>>>
> >>>> Is there any drawback if you use opts->overwrite for
> >>> perf_evlist__mmap_ex?
> >>>
> >>> We will always face such inconsistency, because we have
> >>> an /no-overwrite/ option which can be set per-evsel.
> >>> Setting evlist->overwrite won't make things more consistent,
> >>> because in a evlist, different evsel can have different
> >>> overwrite setting. A simple solution is making evlist
> >>> non-overwrite by default, and watch all overwrite evsels
> >>> a special cases. Then we have only 2 cases to consider:
> >>>
> >>> 1. overwrite evsel in a non-overwrite evlist.
> >>> 2. non-overwrite evsel in a non-overwrite evlist.
> >>>
> >> If evlist->overwrite is always non-overwrite, why not remove it?
> >
> > Some testcases require it.
> >
> 
> Sorry. I think removing it is reasonable now, but we need to solve
> the relationship between overwrite and backward first. I suggest remove
> the whole 'backward' concept, and makes evsels backward if it is
> overwrite. Is there any usecases that:
> 1. overwrite but not backward ring buffer: it will be unparsable after
> ring buffer full.
> 2. backward but not overwrite ring buffer: I don't see any advantage.
>

I agree.
I think we care more about the overwrite or non-overwrite.
It doesn't matter it is backward or forward unless it brings issues.

Thanks,
Kan
 
> Thank you.
> 
> > Thank you.
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-10-12 15:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 14:00 [PATCH] perf tools: fix: Force backward ring buffer mapped readonly Wang Nan
2017-10-11 13:16 ` Liang, Kan
2017-10-12  1:03   ` Wangnan (F)
2017-10-12 12:56     ` Liang, Kan
2017-10-12 14:35       ` Wangnan (F)
2017-10-12 14:45         ` Liang, Kan
2017-10-12 14:46           ` Wangnan (F)
2017-10-12 14:52             ` Wangnan (F)
2017-10-12 15:11               ` Liang, Kan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.