[PATCH 0/2] perf record: Fix --overwrite and clarify concepts

[PATCH 0/2] perf record: Fix --overwrite and clarify concepts

[Wang Nan]

Kan reports that 'perf record --overwrite' not working as it should be.

Patch 1/2 fix a bug, map backward events to readonly ring buffer so kernel
can overwrite that ring buffer.

Patch 2/2 clarify concepts of 'overwrite' and 'backward' in the source code
by introducing the concept of 'flightrecorder' and convert many 'overwrite'
to it to clarify that what we really want is a perf record flightrecorder
mode, not only mapping the ring buffer overwritable.

Wang Nan (2):
  perf mmap: Fix perf backward recording
  perf record: Replace 'overwrite' by 'flightrecorder' for better naming

 tools/perf/Documentation/perf-record.txt |  8 ++++----
 tools/perf/builtin-record.c              |  4 ++--
 tools/perf/perf.h                        |  2 +-
 tools/perf/util/evlist.c                 |  8 +++++++-
 tools/perf/util/evsel.c                  |  6 +++---
 tools/perf/util/evsel.h                  |  4 ++--
 tools/perf/util/parse-events.c           | 20 ++++++++++----------
 tools/perf/util/parse-events.h           |  4 ++--
 tools/perf/util/parse-events.l           |  4 ++--
 9 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.10.1

[PATCH 1/2] perf mmap: Fix perf backward recording

[Wang Nan]

perf record backward recording doesn't work as we expected: it never
overwrite when ring buffer full.

Test:

(Run a busy printing python task background like this:

 while True:
     print 123

send SIGUSR2 to perf to capture snapshot.)

 # ./perf record --overwrite -e raw_syscalls:sys_enter -e raw_syscalls:sys_exit --exclude-perf -a --switch-output
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101520743 ]
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101521251 ]
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101521692 ]
 ^C[ perf record: Woken up 1 times to write data ]
 [ perf record: Dump perf.data.2017110101521936 ]
 [ perf record: Captured and wrote 0.826 MB perf.data.<timestamp> ]

 # ./perf script -i ./perf.data.2017110101520743 | head -n3
             perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
             perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
           python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4
 # ./perf script -i ./perf.data.2017110101521251 | head -n3
             perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
             perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
           python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4
 # ./perf script -i ./perf.data.2017110101521692 | head -n3
             perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
             perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
           python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4

Timestamps are never change, but my background task is a dead loop, can
easily overwhelme the ring buffer.

This patch fix it by force unsetting PROT_WRITE for backward ring
buffer, so all backward ring buffer become overwrite ring buffer.

Test result:

 # ./perf record --overwrite -e raw_syscalls:sys_enter -e raw_syscalls:sys_exit --exclude-perf -a --switch-output
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101285323 ]
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101290053 ]
 [ perf record: dump data: Woken up 1 times ]
 [ perf record: Dump perf.data.2017110101290446 ]
 ^C[ perf record: Woken up 1 times to write data ]
 [ perf record: Dump perf.data.2017110101290837 ]
 [ perf record: Captured and wrote 0.826 MB perf.data.<timestamp> ]
 # ./perf script -i ./perf.data.2017110101285323 | head -n3
           python  2545 [000] 11064.268083:  raw_syscalls:sys_exit: NR 1 = 4
           python  2545 [000] 11064.268084: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
           python  2545 [000] 11064.268086:  raw_syscalls:sys_exit: NR 1 = 4
 # ./perf script -i ./perf.data.2017110101290 | head -n3
 failed to open ./perf.data.2017110101290: No such file or directory
 # ./perf script -i ./perf.data.2017110101290053 | head -n3
           python  2545 [000] 11071.564062: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
           python  2545 [000] 11071.564064:  raw_syscalls:sys_exit: NR 1 = 4
           python  2545 [000] 11071.564066: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
 # ./perf script -i ./perf.data.2017110101290 | head -n3
 perf.data.2017110101290053  perf.data.2017110101290446  perf.data.2017110101290837
 # ./perf script -i ./perf.data.2017110101290446 | head -n3
             sshd  1321 [000] 11075.499473:  raw_syscalls:sys_exit: NR 14 = 0
             sshd  1321 [000] 11075.499474: raw_syscalls:sys_enter: NR 14 (2, 7ffe98899490, 0, 8, 0, 3000)
             sshd  1321 [000] 11075.499474:  raw_syscalls:sys_exit: NR 14 = 0
 # ./perf script -i ./perf.data.2017110101290837 | head -n3
           python  2545 [000] 11079.280844:  raw_syscalls:sys_exit: NR 1 = 4
           python  2545 [000] 11079.280847: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
           python  2545 [000] 11079.280850:  raw_syscalls:sys_exit: NR 1 = 4

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/util/evlist.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c6c891e..4c5daba 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -799,22 +799,28 @@ 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;
 
 	evlist__for_each_entry(evlist, evsel) {
 		struct perf_mmap *maps = evlist->mmap;
+		struct mmap_params rdonly_mp;
 		int *output = _output;
 		int fd;
 		int cpu;
 
+		mp = _mp;
 		if (evsel->attr.write_backward) {
 			output = _output_backward;
 			maps = evlist->backward_mmap;
+			rdonly_mp = *_mp;
+			rdonly_mp.prot &= ~PROT_WRITE;
+			mp = &rdonly_mp;
 
 			if (!maps) {
 				maps = perf_evlist__alloc_mmap(evlist);
-- 
2.10.1

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Namhyung Kim]

Hi,

On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
> perf record backward recording doesn't work as we expected: it never
> overwrite when ring buffer full.
> 
> Test:
> 
> (Run a busy printing python task background like this:
> 
>  while True:
>      print 123
> 
> send SIGUSR2 to perf to capture snapshot.)
> 
>  # ./perf record --overwrite -e raw_syscalls:sys_enter -e raw_syscalls:sys_exit --exclude-perf -a --switch-output
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101520743 ]
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101521251 ]
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101521692 ]
>  ^C[ perf record: Woken up 1 times to write data ]
>  [ perf record: Dump perf.data.2017110101521936 ]
>  [ perf record: Captured and wrote 0.826 MB perf.data.<timestamp> ]
> 
>  # ./perf script -i ./perf.data.2017110101520743 | head -n3
>              perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
>              perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
>            python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4
>  # ./perf script -i ./perf.data.2017110101521251 | head -n3
>              perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
>              perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
>            python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4
>  # ./perf script -i ./perf.data.2017110101521692 | head -n3
>              perf  2717 [000] 12449.310785: raw_syscalls:sys_enter: NR 16 (5, 2400, 0, 59, 100, 0)
>              perf  2717 [000] 12449.310790: raw_syscalls:sys_enter: NR 7 (4112340, 2, ffffffff, 3df, 100, 0)
>            python  2545 [000] 12449.310800:  raw_syscalls:sys_exit: NR 1 = 4
> 
> Timestamps are never change, but my background task is a dead loop, can
> easily overwhelme the ring buffer.
> 
> This patch fix it by force unsetting PROT_WRITE for backward ring
> buffer, so all backward ring buffer become overwrite ring buffer.
> 
> Test result:
> 
>  # ./perf record --overwrite -e raw_syscalls:sys_enter -e raw_syscalls:sys_exit --exclude-perf -a --switch-output
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101285323 ]
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101290053 ]
>  [ perf record: dump data: Woken up 1 times ]
>  [ perf record: Dump perf.data.2017110101290446 ]
>  ^C[ perf record: Woken up 1 times to write data ]
>  [ perf record: Dump perf.data.2017110101290837 ]
>  [ perf record: Captured and wrote 0.826 MB perf.data.<timestamp> ]
>  # ./perf script -i ./perf.data.2017110101285323 | head -n3
>            python  2545 [000] 11064.268083:  raw_syscalls:sys_exit: NR 1 = 4
>            python  2545 [000] 11064.268084: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
>            python  2545 [000] 11064.268086:  raw_syscalls:sys_exit: NR 1 = 4
>  # ./perf script -i ./perf.data.2017110101290 | head -n3
>  failed to open ./perf.data.2017110101290: No such file or directory
>  # ./perf script -i ./perf.data.2017110101290053 | head -n3
>            python  2545 [000] 11071.564062: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
>            python  2545 [000] 11071.564064:  raw_syscalls:sys_exit: NR 1 = 4
>            python  2545 [000] 11071.564066: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
>  # ./perf script -i ./perf.data.2017110101290 | head -n3
>  perf.data.2017110101290053  perf.data.2017110101290446  perf.data.2017110101290837
>  # ./perf script -i ./perf.data.2017110101290446 | head -n3
>              sshd  1321 [000] 11075.499473:  raw_syscalls:sys_exit: NR 14 = 0
>              sshd  1321 [000] 11075.499474: raw_syscalls:sys_enter: NR 14 (2, 7ffe98899490, 0, 8, 0, 3000)
>              sshd  1321 [000] 11075.499474:  raw_syscalls:sys_exit: NR 14 = 0
>  # ./perf script -i ./perf.data.2017110101290837 | head -n3
>            python  2545 [000] 11079.280844:  raw_syscalls:sys_exit: NR 1 = 4
>            python  2545 [000] 11079.280847: raw_syscalls:sys_enter: NR 1 (1, 12cc330, 4, 7fc237280370, 7fc2373d0700, 2c7b0)
>            python  2545 [000] 11079.280850:  raw_syscalls:sys_exit: NR 1 = 4
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/util/evlist.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c6c891e..4c5daba 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -799,22 +799,28 @@ 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;
>  
>  	evlist__for_each_entry(evlist, evsel) {
>  		struct perf_mmap *maps = evlist->mmap;
> +		struct mmap_params rdonly_mp;
>  		int *output = _output;
>  		int fd;
>  		int cpu;
>  
> +		mp = _mp;
>  		if (evsel->attr.write_backward) {
>  			output = _output_backward;
>  			maps = evlist->backward_mmap;
> +			rdonly_mp = *_mp;
> +			rdonly_mp.prot &= ~PROT_WRITE;
> +			mp = &rdonly_mp;
>  
>  			if (!maps) {
>  				maps = perf_evlist__alloc_mmap(evlist);
> -- 

What about this instead (not tested)?


diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index c6c891e154a6..27ebe355e794 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
                if (*output == -1) {
                        *output = fd;
 
+                       if (evsel->attr.write_backward)
+                               mp->prot = PROT_READ;
+                       else
+                               mp->prot = PROT_READ | PROT_WRITE;
+
                        if (perf_mmap__mmap(&maps[idx], mp, *output)  < 0)
                                return -1;
                } else {
@@ -1058,9 +1063,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
        struct perf_evsel *evsel;
        const struct cpu_map *cpus = evlist->cpus;
        const struct thread_map *threads = evlist->threads;
-       struct mmap_params mp = {
-               .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
-       };
+       struct mmap_params mp;
 
        if (!evlist->mmap)
                evlist->mmap = perf_evlist__alloc_mmap(evlist);

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Wangnan (F)]

On 2017/11/1 17:49, Namhyung Kim wrote:
> Hi,
>
> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
>> perf record backward recording doesn't work as we expected: it never
>> overwrite when ring buffer full.
>>
>> Test:
>>
>> (Run a busy printing python task background like this:
>>
>>   while True:
>>       print 123
>>
>> send SIGUSR2 to perf to capture snapshot.)

[SNIP]

>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> ---
>>   tools/perf/util/evlist.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index c6c891e..4c5daba 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -799,22 +799,28 @@ 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;
>>   
>>   	evlist__for_each_entry(evlist, evsel) {
>>   		struct perf_mmap *maps = evlist->mmap;
>> +		struct mmap_params rdonly_mp;
>>   		int *output = _output;
>>   		int fd;
>>   		int cpu;
>>   
>> +		mp = _mp;
>>   		if (evsel->attr.write_backward) {
>>   			output = _output_backward;
>>   			maps = evlist->backward_mmap;
>> +			rdonly_mp = *_mp;
>> +			rdonly_mp.prot &= ~PROT_WRITE;
>> +			mp = &rdonly_mp;
>>   
>>   			if (!maps) {
>>   				maps = perf_evlist__alloc_mmap(evlist);
>> -- 
> What about this instead (not tested)?
>
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index c6c891e154a6..27ebe355e794 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>                  if (*output == -1) {
>                          *output = fd;
>   
> +                       if (evsel->attr.write_backward)
> +                               mp->prot = PROT_READ;
> +                       else
> +                               mp->prot = PROT_READ | PROT_WRITE;
> +

If evlist->overwrite is true, PROT_WRITE should be unset even if 
write_backward is
not set. If you want to delay the setting of mp->prot, you need to consider
both evlist->overwrite and evsel->attr.write_backward.

Then why not removing mp->prot totally, and add a prot argument to 
perf_mmap__mmap,
since prot setting is always decided independently?

>                          if (perf_mmap__mmap(&maps[idx], mp, *output)  < 0)
>                                  return -1;
>                  } else {
> @@ -1058,9 +1063,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
>          struct perf_evsel *evsel;
>          const struct cpu_map *cpus = evlist->cpus;
>          const struct thread_map *threads = evlist->threads;
> -       struct mmap_params mp = {
> -               .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
> -       };
> +       struct mmap_params mp;
>   
>          if (!evlist->mmap)
>                  evlist->mmap = perf_evlist__alloc_mmap(evlist);

The 'overwrite' argument in perf_evlist__mmap_ex() controls 
evlist->overwrite.

Thank you.

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Namhyung Kim]

On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> 
> 
> On 2017/11/1 17:49, Namhyung Kim wrote:
> > Hi,
> > 
> > On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
> > > perf record backward recording doesn't work as we expected: it never
> > > overwrite when ring buffer full.
> > > 
> > > Test:
> > > 
> > > (Run a busy printing python task background like this:
> > > 
> > >   while True:
> > >       print 123
> > > 
> > > send SIGUSR2 to perf to capture snapshot.)
> 
> [SNIP]
> 
> > > 
> > > Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > > ---
> > >   tools/perf/util/evlist.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index c6c891e..4c5daba 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -799,22 +799,28 @@ 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;
> > >   	evlist__for_each_entry(evlist, evsel) {
> > >   		struct perf_mmap *maps = evlist->mmap;
> > > +		struct mmap_params rdonly_mp;
> > >   		int *output = _output;
> > >   		int fd;
> > >   		int cpu;
> > > +		mp = _mp;
> > >   		if (evsel->attr.write_backward) {
> > >   			output = _output_backward;
> > >   			maps = evlist->backward_mmap;
> > > +			rdonly_mp = *_mp;
> > > +			rdonly_mp.prot &= ~PROT_WRITE;
> > > +			mp = &rdonly_mp;
> > >   			if (!maps) {
> > >   				maps = perf_evlist__alloc_mmap(evlist);
> > > -- 
> > What about this instead (not tested)?
> > 
> > 
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index c6c891e154a6..27ebe355e794 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> >                  if (*output == -1) {
> >                          *output = fd;
> > +                       if (evsel->attr.write_backward)
> > +                               mp->prot = PROT_READ;
> > +                       else
> > +                               mp->prot = PROT_READ | PROT_WRITE;
> > +
> 
> If evlist->overwrite is true, PROT_WRITE should be unset even if
> write_backward is
> not set. If you want to delay the setting of mp->prot, you need to consider
> both evlist->overwrite and evsel->attr.write_backward.

I thought evsel->attr.write_backward should be set when
evlist->overwrite is set.  Do you mean following case?

  perf record --overwrite -e 'cycles/no-overwrite/'


> 
> Then why not removing mp->prot totally, and add a prot argument to
> perf_mmap__mmap,
> since prot setting is always decided independently?

I'm ok with it.


> 
> >                          if (perf_mmap__mmap(&maps[idx], mp, *output)  < 0)
> >                                  return -1;
> >                  } else {
> > @@ -1058,9 +1063,7 @@ int perf_evlist__mmap_ex(struct perf_evlist *evlist, unsigned int pages,
> >          struct perf_evsel *evsel;
> >          const struct cpu_map *cpus = evlist->cpus;
> >          const struct thread_map *threads = evlist->threads;
> > -       struct mmap_params mp = {
> > -               .prot = PROT_READ | (overwrite ? 0 : PROT_WRITE),
> > -       };
> > +       struct mmap_params mp;
> >          if (!evlist->mmap)
> >                  evlist->mmap = perf_evlist__alloc_mmap(evlist);
> 
> The 'overwrite' argument in perf_evlist__mmap_ex() controls
> evlist->overwrite.

Right, and it's always "false" for recording in the current code.

Thanks,
Namhyung

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Wangnan (F)]

On 2017/11/1 20:00, Namhyung Kim wrote:
> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
>>
>> On 2017/11/1 17:49, Namhyung Kim wrote:
>>> Hi,
>>>
>>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
>>>> perf record backward recording doesn't work as we expected: it never
>>>> overwrite when ring buffer full.
>>>>
>>>> Test:
>>>>
>>>> (Run a busy printing python task background like this:
>>>>
>>>>    while True:
>>>>        print 123
>>>>
>>>> send SIGUSR2 to perf to capture snapshot.)
>> [SNIP]
>>
>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>> ---
>>>>    tools/perf/util/evlist.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>> index c6c891e..4c5daba 100644
>>>> --- a/tools/perf/util/evlist.c
>>>> +++ b/tools/perf/util/evlist.c
>>>> @@ -799,22 +799,28 @@ 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;
>>>>    	evlist__for_each_entry(evlist, evsel) {
>>>>    		struct perf_mmap *maps = evlist->mmap;
>>>> +		struct mmap_params rdonly_mp;
>>>>    		int *output = _output;
>>>>    		int fd;
>>>>    		int cpu;
>>>> +		mp = _mp;
>>>>    		if (evsel->attr.write_backward) {
>>>>    			output = _output_backward;
>>>>    			maps = evlist->backward_mmap;
>>>> +			rdonly_mp = *_mp;
>>>> +			rdonly_mp.prot &= ~PROT_WRITE;
>>>> +			mp = &rdonly_mp;
>>>>    			if (!maps) {
>>>>    				maps = perf_evlist__alloc_mmap(evlist);
>>>> -- 
>>> What about this instead (not tested)?
>>>
>>>
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index c6c891e154a6..27ebe355e794 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>>>                   if (*output == -1) {
>>>                           *output = fd;
>>> +                       if (evsel->attr.write_backward)
>>> +                               mp->prot = PROT_READ;
>>> +                       else
>>> +                               mp->prot = PROT_READ | PROT_WRITE;
>>> +
>> If evlist->overwrite is true, PROT_WRITE should be unset even if
>> write_backward is
>> not set. If you want to delay the setting of mp->prot, you need to consider
>> both evlist->overwrite and evsel->attr.write_backward.
> I thought evsel->attr.write_backward should be set when
> evlist->overwrite is set.  Do you mean following case?
>
>    perf record --overwrite -e 'cycles/no-overwrite/'
>

No. evlist->overwrite is unrelated to '--overwrite'. This is why I
said the concept of 'overwrite' and 'backward' is ambiguous.

perf record never sets 'evlist->overwrite'. What '--overwrite' actually
does is setting write_backward. Some testcases needs overwrite evlist.

Thank you.

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Jiri Olsa]

On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote:

SNIP

> > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > > index c6c891e154a6..27ebe355e794 100644
> > > > --- a/tools/perf/util/evlist.c
> > > > +++ b/tools/perf/util/evlist.c
> > > > @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> > > >                   if (*output == -1) {
> > > >                           *output = fd;
> > > > +                       if (evsel->attr.write_backward)
> > > > +                               mp->prot = PROT_READ;
> > > > +                       else
> > > > +                               mp->prot = PROT_READ | PROT_WRITE;
> > > > +
> > > If evlist->overwrite is true, PROT_WRITE should be unset even if
> > > write_backward is
> > > not set. If you want to delay the setting of mp->prot, you need to consider
> > > both evlist->overwrite and evsel->attr.write_backward.
> > I thought evsel->attr.write_backward should be set when
> > evlist->overwrite is set.  Do you mean following case?
> > 
> >    perf record --overwrite -e 'cycles/no-overwrite/'
> > 
> 
> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
> said the concept of 'overwrite' and 'backward' is ambiguous.
> 
> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
> does is setting write_backward. Some testcases needs overwrite evlist.

did not check deeply, but so why can't we do the below?

jirka


---
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f4d9fc54b382..4643c487bd29 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
 	struct record_opts *opts = &rec->opts;
 	char msg[512];
 
-	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
+	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite,
 				 opts->auxtrace_mmap_pages,
 				 opts->auxtrace_snapshot_mode) < 0) {
 		if (errno == EPERM) {

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Wangnan (F)]

On 2017/11/1 20:39, Jiri Olsa wrote:
> On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote:
>
> SNIP
>
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index c6c891e154a6..27ebe355e794 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
>>>>>                    if (*output == -1) {
>>>>>                            *output = fd;
>>>>> +                       if (evsel->attr.write_backward)
>>>>> +                               mp->prot = PROT_READ;
>>>>> +                       else
>>>>> +                               mp->prot = PROT_READ | PROT_WRITE;
>>>>> +
>>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
>>>> write_backward is
>>>> not set. If you want to delay the setting of mp->prot, you need to consider
>>>> both evlist->overwrite and evsel->attr.write_backward.
>>> I thought evsel->attr.write_backward should be set when
>>> evlist->overwrite is set.  Do you mean following case?
>>>
>>>     perf record --overwrite -e 'cycles/no-overwrite/'
>>>
>> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
>> said the concept of 'overwrite' and 'backward' is ambiguous.
>>
>> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
>> does is setting write_backward. Some testcases needs overwrite evlist.
> did not check deeply, but so why can't we do the below?
>
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4d9fc54b382..4643c487bd29 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
>   	struct record_opts *opts = &rec->opts;
>   	char msg[512];
>   
> -	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
> +	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite,
>   				 opts->auxtrace_mmap_pages,
>   				 opts->auxtrace_snapshot_mode) < 0) {

perf_evlist__mmap_ex's overwrite argument is used to control evlist->mmap.

Consider Namhyung's example:

   perf record --overwrite -e 'cycles/no-overwrite/'

In this case both evlist->mmap and evlist->backward_mmap would be set to overwrite.
'cycles' will be put into evlist->mmap, so it will be set to overwrite incorrectly.

Patch 2/2 clarifies these concepts. What we want is the flight recorder mode,
not only a read only ring buffer. Even flight recorder mode is selected, we can
still have a normal ring buffer which keep output data.

Thank you.

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Jiri Olsa]

On Wed, Nov 01, 2017 at 08:56:32PM +0800, Wangnan (F) wrote:
> 
> 
> On 2017/11/1 20:39, Jiri Olsa wrote:
> > On Wed, Nov 01, 2017 at 08:10:49PM +0800, Wangnan (F) wrote:
> > 
> > SNIP
> > 
> > > > > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > > > > index c6c891e154a6..27ebe355e794 100644
> > > > > > --- a/tools/perf/util/evlist.c
> > > > > > +++ b/tools/perf/util/evlist.c
> > > > > > @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct perf_evlist *evlist, int idx,
> > > > > >                    if (*output == -1) {
> > > > > >                            *output = fd;
> > > > > > +                       if (evsel->attr.write_backward)
> > > > > > +                               mp->prot = PROT_READ;
> > > > > > +                       else
> > > > > > +                               mp->prot = PROT_READ | PROT_WRITE;
> > > > > > +
> > > > > If evlist->overwrite is true, PROT_WRITE should be unset even if
> > > > > write_backward is
> > > > > not set. If you want to delay the setting of mp->prot, you need to consider
> > > > > both evlist->overwrite and evsel->attr.write_backward.
> > > > I thought evsel->attr.write_backward should be set when
> > > > evlist->overwrite is set.  Do you mean following case?
> > > > 
> > > >     perf record --overwrite -e 'cycles/no-overwrite/'
> > > > 
> > > No. evlist->overwrite is unrelated to '--overwrite'. This is why I
> > > said the concept of 'overwrite' and 'backward' is ambiguous.
> > > 
> > > perf record never sets 'evlist->overwrite'. What '--overwrite' actually
> > > does is setting write_backward. Some testcases needs overwrite evlist.
> > did not check deeply, but so why can't we do the below?
> > 
> > jirka
> > 
> > 
> > ---
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index f4d9fc54b382..4643c487bd29 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -300,7 +300,7 @@ static int record__mmap_evlist(struct record *rec,
> >   	struct record_opts *opts = &rec->opts;
> >   	char msg[512];
> > -	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
> > +	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, opts->overwrite,
> >   				 opts->auxtrace_mmap_pages,
> >   				 opts->auxtrace_snapshot_mode) < 0) {
> 
> perf_evlist__mmap_ex's overwrite argument is used to control evlist->mmap.
> 
> Consider Namhyung's example:
> 
>   perf record --overwrite -e 'cycles/no-overwrite/'
> 
> In this case both evlist->mmap and evlist->backward_mmap would be set to overwrite.
> 'cycles' will be put into evlist->mmap, so it will be set to overwrite incorrectly.

right, missed the separate mmaps..

so we have some code that uses evlist->overwrite, which is always
set to 'false' in perf record.. but in the crucial checks like
for perf_mmap__consume we use the 'backward' bool to save the day

that might need some consolidation as well.. we could keep the
overwrite flag in the struct perf_mmap.. that could simplify the code

jirka

RE: [PATCH 1/2] perf mmap: Fix perf backward recording

[Liang, Kan]

> On 2017/11/1 20:00, Namhyung Kim wrote:
> > On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> >>
> >> On 2017/11/1 17:49, Namhyung Kim wrote:
> >>> Hi,
> >>>
> >>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
> >>>> perf record backward recording doesn't work as we expected: it never
> >>>> overwrite when ring buffer full.
> >>>>
> >>>> Test:
> >>>>
> >>>> (Run a busy printing python task background like this:
> >>>>
> >>>>    while True:
> >>>>        print 123
> >>>>
> >>>> send SIGUSR2 to perf to capture snapshot.)
> >> [SNIP]
> >>
> >>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>>> ---
> >>>>    tools/perf/util/evlist.c | 8 +++++++-
> >>>>    1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>> index c6c891e..4c5daba 100644
> >>>> --- a/tools/perf/util/evlist.c
> >>>> +++ b/tools/perf/util/evlist.c
> >>>> @@ -799,22 +799,28 @@ 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;
> >>>>    	evlist__for_each_entry(evlist, evsel) {
> >>>>    		struct perf_mmap *maps = evlist->mmap;
> >>>> +		struct mmap_params rdonly_mp;
> >>>>    		int *output = _output;
> >>>>    		int fd;
> >>>>    		int cpu;
> >>>> +		mp = _mp;
> >>>>    		if (evsel->attr.write_backward) {
> >>>>    			output = _output_backward;
> >>>>    			maps = evlist->backward_mmap;
> >>>> +			rdonly_mp = *_mp;
> >>>> +			rdonly_mp.prot &= ~PROT_WRITE;
> >>>> +			mp = &rdonly_mp;
> >>>>    			if (!maps) {
> >>>>    				maps = perf_evlist__alloc_mmap(evlist);
> >>>> --
> >>> What about this instead (not tested)?
> >>>
> >>>
> >>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>> index c6c891e154a6..27ebe355e794 100644
> >>> --- a/tools/perf/util/evlist.c
> >>> +++ b/tools/perf/util/evlist.c
> >>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct
> perf_evlist *evlist, int idx,
> >>>                   if (*output == -1) {
> >>>                           *output = fd;
> >>> +                       if (evsel->attr.write_backward)
> >>> +                               mp->prot = PROT_READ;
> >>> +                       else
> >>> +                               mp->prot = PROT_READ | PROT_WRITE;
> >>> +
> >> If evlist->overwrite is true, PROT_WRITE should be unset even if
> >> write_backward is
> >> not set. If you want to delay the setting of mp->prot, you need to consider
> >> both evlist->overwrite and evsel->attr.write_backward.
> > I thought evsel->attr.write_backward should be set when
> > evlist->overwrite is set.  Do you mean following case?
> >
> >    perf record --overwrite -e 'cycles/no-overwrite/'
> >
> 
> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
> said the concept of 'overwrite' and 'backward' is ambiguous.
>

Yes, I think we should make it clear.

As we discussed previously, there are four possible combinations
to access ring buffer , 'forward non-overwrite', 'forward overwrite',
'backward non-overwrite' and 'backward overwrite'.

Actually, not all of the combinations are necessary.
- 'forward overwrite' mode brings some problems which were mentioned
  in commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute
  to perf event").
- 'backward non-overwrite' mode is very similar as 'forward non-overwrite'.
   There is no extra advantage. Only keep one non-overwrite mode is enough.
So 'forward non-overwrite' and 'backward overwrite' are enough for all perf tools.

Furthermore, 'forward' and 'backward' only indicate the direction of the
ring buffer. They don't impact the result and performance. It is not
important as the concept of overwrite/non-overwrite.

To simplify the concept, only 'non-overwrite' mode and 'overwrite' mode should
be kept. 'non-overwrite' mode indicates the forward ring buffer. 'overwrite' mode
indicates the backward ring buffer.

> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
> does is setting write_backward. Some testcases needs overwrite evlist.
> 

There are only four test cases which set overwrite, sw-clock,task-exit,
mmap-basic, backward-ring-buffer.
Only backward-ring-buffer is 'backward overwrite'.
The rest three are all 'forward overwrite'. We just need to set write_backward
to convert them to 'backward overwrite'.
I think it's not hard to clean up.

Thanks,
Kan

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Wangnan (F)]

On 2017/11/1 21:57, Liang, Kan wrote:
>> On 2017/11/1 20:00, Namhyung Kim wrote:
>>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
>>>> On 2017/11/1 17:49, Namhyung Kim wrote:
>>>>> Hi,
>>>>>
>>>>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
>>>>>> perf record backward recording doesn't work as we expected: it never
>>>>>> overwrite when ring buffer full.
>>>>>>
>>>>>> Test:
>>>>>>
>>>>>> (Run a busy printing python task background like this:
>>>>>>
>>>>>>     while True:
>>>>>>         print 123
>>>>>>
>>>>>> send SIGUSR2 to perf to capture snapshot.)
>>>> [SNIP]
>>>>
>>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>>>> ---
>>>>>>     tools/perf/util/evlist.c | 8 +++++++-
>>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>>> index c6c891e..4c5daba 100644
>>>>>> --- a/tools/perf/util/evlist.c
>>>>>> +++ b/tools/perf/util/evlist.c
>>>>>> @@ -799,22 +799,28 @@ 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;
>>>>>>     	evlist__for_each_entry(evlist, evsel) {
>>>>>>     		struct perf_mmap *maps = evlist->mmap;
>>>>>> +		struct mmap_params rdonly_mp;
>>>>>>     		int *output = _output;
>>>>>>     		int fd;
>>>>>>     		int cpu;
>>>>>> +		mp = _mp;
>>>>>>     		if (evsel->attr.write_backward) {
>>>>>>     			output = _output_backward;
>>>>>>     			maps = evlist->backward_mmap;
>>>>>> +			rdonly_mp = *_mp;
>>>>>> +			rdonly_mp.prot &= ~PROT_WRITE;
>>>>>> +			mp = &rdonly_mp;
>>>>>>     			if (!maps) {
>>>>>>     				maps = perf_evlist__alloc_mmap(evlist);
>>>>>> --
>>>>> What about this instead (not tested)?
>>>>>
>>>>>
>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>>>> index c6c891e154a6..27ebe355e794 100644
>>>>> --- a/tools/perf/util/evlist.c
>>>>> +++ b/tools/perf/util/evlist.c
>>>>> @@ -838,6 +838,11 @@ static int perf_evlist__mmap_per_evsel(struct
>> perf_evlist *evlist, int idx,
>>>>>                    if (*output == -1) {
>>>>>                            *output = fd;
>>>>> +                       if (evsel->attr.write_backward)
>>>>> +                               mp->prot = PROT_READ;
>>>>> +                       else
>>>>> +                               mp->prot = PROT_READ | PROT_WRITE;
>>>>> +
>>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
>>>> write_backward is
>>>> not set. If you want to delay the setting of mp->prot, you need to consider
>>>> both evlist->overwrite and evsel->attr.write_backward.
>>> I thought evsel->attr.write_backward should be set when
>>> evlist->overwrite is set.  Do you mean following case?
>>>
>>>     perf record --overwrite -e 'cycles/no-overwrite/'
>>>
>> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
>> said the concept of 'overwrite' and 'backward' is ambiguous.
>>
> Yes, I think we should make it clear.
>
> As we discussed previously, there are four possible combinations
> to access ring buffer , 'forward non-overwrite', 'forward overwrite',
> 'backward non-overwrite' and 'backward overwrite'.
>
> Actually, not all of the combinations are necessary.
> - 'forward overwrite' mode brings some problems which were mentioned
>    in commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute
>    to perf event").
> - 'backward non-overwrite' mode is very similar as 'forward non-overwrite'.
>     There is no extra advantage. Only keep one non-overwrite mode is enough.
> So 'forward non-overwrite' and 'backward overwrite' are enough for all perf tools.
>
> Furthermore, 'forward' and 'backward' only indicate the direction of the
> ring buffer. They don't impact the result and performance. It is not
> important as the concept of overwrite/non-overwrite.
>
> To simplify the concept, only 'non-overwrite' mode and 'overwrite' mode should
> be kept. 'non-overwrite' mode indicates the forward ring buffer. 'overwrite' mode
> indicates the backward ring buffer.
>
>> perf record never sets 'evlist->overwrite'. What '--overwrite' actually
>> does is setting write_backward. Some testcases needs overwrite evlist.
>>
> There are only four test cases which set overwrite, sw-clock,task-exit,
> mmap-basic, backward-ring-buffer.
> Only backward-ring-buffer is 'backward overwrite'.
> The rest three are all 'forward overwrite'. We just need to set write_backward
> to convert them to 'backward overwrite'.
> I think it's not hard to clean up.

If we add a new rule that overwrite ring buffers are always backward
then it is not hard to cleanup. However, the support of forward
overwrite ring buffer has a long history and the code is not written
by me. I'd like to check if there is some reason to keep support this
configuration?

Thank you.

RE: [PATCH 1/2] perf mmap: Fix perf backward recording

[Liang, Kan]

> On 2017/11/1 21:57, Liang, Kan wrote:
> >> On 2017/11/1 20:00, Namhyung Kim wrote:
> >>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> >>>> On 2017/11/1 17:49, Namhyung Kim wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Wed, Nov 01, 2017 at 05:53:26AM +0000, Wang Nan wrote:
> >>>>>> perf record backward recording doesn't work as we expected: it
> >>>>>> never overwrite when ring buffer full.
> >>>>>>
> >>>>>> Test:
> >>>>>>
> >>>>>> (Run a busy printing python task background like this:
> >>>>>>
> >>>>>>     while True:
> >>>>>>         print 123
> >>>>>>
> >>>>>> send SIGUSR2 to perf to capture snapshot.)
> >>>> [SNIP]
> >>>>
> >>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>>>>> ---
> >>>>>>     tools/perf/util/evlist.c | 8 +++++++-
> >>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>>>> index c6c891e..4c5daba 100644
> >>>>>> --- a/tools/perf/util/evlist.c
> >>>>>> +++ b/tools/perf/util/evlist.c
> >>>>>> @@ -799,22 +799,28 @@ 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;
> >>>>>>     	evlist__for_each_entry(evlist, evsel) {
> >>>>>>     		struct perf_mmap *maps = evlist->mmap;
> >>>>>> +		struct mmap_params rdonly_mp;
> >>>>>>     		int *output = _output;
> >>>>>>     		int fd;
> >>>>>>     		int cpu;
> >>>>>> +		mp = _mp;
> >>>>>>     		if (evsel->attr.write_backward) {
> >>>>>>     			output = _output_backward;
> >>>>>>     			maps = evlist->backward_mmap;
> >>>>>> +			rdonly_mp = *_mp;
> >>>>>> +			rdonly_mp.prot &= ~PROT_WRITE;
> >>>>>> +			mp = &rdonly_mp;
> >>>>>>     			if (!maps) {
> >>>>>>     				maps =
> perf_evlist__alloc_mmap(evlist);
> >>>>>> --
> >>>>> What about this instead (not tested)?
> >>>>>
> >>>>>
> >>>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>>>> index c6c891e154a6..27ebe355e794 100644
> >>>>> --- a/tools/perf/util/evlist.c
> >>>>> +++ b/tools/perf/util/evlist.c
> >>>>> @@ -838,6 +838,11 @@ static int
> perf_evlist__mmap_per_evsel(struct
> >> perf_evlist *evlist, int idx,
> >>>>>                    if (*output == -1) {
> >>>>>                            *output = fd;
> >>>>> +                       if (evsel->attr.write_backward)
> >>>>> +                               mp->prot = PROT_READ;
> >>>>> +                       else
> >>>>> +                               mp->prot = PROT_READ | PROT_WRITE;
> >>>>> +
> >>>> If evlist->overwrite is true, PROT_WRITE should be unset even if
> >>>> write_backward is not set. If you want to delay the setting of
> >>>> mp->prot, you need to consider both evlist->overwrite and
> >>>> evsel->attr.write_backward.
> >>> I thought evsel->attr.write_backward should be set when
> >>> evlist->overwrite is set.  Do you mean following case?
> >>>
> >>>     perf record --overwrite -e 'cycles/no-overwrite/'
> >>>
> >> No. evlist->overwrite is unrelated to '--overwrite'. This is why I
> >> said the concept of 'overwrite' and 'backward' is ambiguous.
> >>
> > Yes, I think we should make it clear.
> >
> > As we discussed previously, there are four possible combinations to
> > access ring buffer , 'forward non-overwrite', 'forward overwrite',
> > 'backward non-overwrite' and 'backward overwrite'.
> >
> > Actually, not all of the combinations are necessary.
> > - 'forward overwrite' mode brings some problems which were mentioned
> >    in commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute
> >    to perf event").
> > - 'backward non-overwrite' mode is very similar as 'forward non-overwrite'.
> >     There is no extra advantage. Only keep one non-overwrite mode is
> enough.
> > So 'forward non-overwrite' and 'backward overwrite' are enough for all
> perf tools.
> >
> > Furthermore, 'forward' and 'backward' only indicate the direction of
> > the ring buffer. They don't impact the result and performance. It is
> > not important as the concept of overwrite/non-overwrite.
> >
> > To simplify the concept, only 'non-overwrite' mode and 'overwrite'
> > mode should be kept. 'non-overwrite' mode indicates the forward ring
> > buffer. 'overwrite' mode indicates the backward ring buffer.
> >
> >> perf record never sets 'evlist->overwrite'. What '--overwrite'
> >> actually does is setting write_backward. Some testcases needs overwrite
> evlist.
> >>
> > There are only four test cases which set overwrite,
> > sw-clock,task-exit, mmap-basic, backward-ring-buffer.
> > Only backward-ring-buffer is 'backward overwrite'.
> > The rest three are all 'forward overwrite'. We just need to set
> > write_backward to convert them to 'backward overwrite'.
> > I think it's not hard to clean up.
> 
> If we add a new rule that overwrite ring buffers are always backward then it
> is not hard to cleanup. However, the support of forward overwrite ring
> buffer has a long history and the code is not written by me. I'd like to check if
> there is some reason to keep support this configuration?
> 

As my observation, currently, there are no perf tools which support
'forward overwrite'.
There are only three test cases (sw-clock, task-exit, mmap-basic) which
is set to 'forward overwrite'. I don’t see any reason it cannot be changed to
'backward overwrite'

Arnaldo? Jirka? Kim?

What do you think?

Thanks,
Kan

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Namhyung Kim]

Hi Kan,

On Wed, Nov 01, 2017 at 04:22:53PM +0000, Liang, Kan wrote:
> > On 2017/11/1 21:57, Liang, Kan wrote:
> > >> On 2017/11/1 20:00, Namhyung Kim wrote:
> > >>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> > > There are only four test cases which set overwrite,
> > > sw-clock,task-exit, mmap-basic, backward-ring-buffer.
> > > Only backward-ring-buffer is 'backward overwrite'.
> > > The rest three are all 'forward overwrite'. We just need to set
> > > write_backward to convert them to 'backward overwrite'.
> > > I think it's not hard to clean up.
> > 
> > If we add a new rule that overwrite ring buffers are always backward then it
> > is not hard to cleanup. However, the support of forward overwrite ring
> > buffer has a long history and the code is not written by me. I'd like to check if
> > there is some reason to keep support this configuration?
> > 
> 
> As my observation, currently, there are no perf tools which support
> 'forward overwrite'.
> There are only three test cases (sw-clock, task-exit, mmap-basic) which
> is set to 'forward overwrite'. I don’t see any reason it cannot be changed to
> 'backward overwrite'
> 
> Arnaldo? Jirka? Kim?
> 
> What do you think?

I think sw-clock, task-exit and mmap-basic test cases can be changed
to use the forward non-overwrite mode.

The forward overwrite might be used by externel applications accessing
the ring buffer directly but not needed for perf tools IMHO.  Let's
keep the code simpler as much as possible.

Thanks,
Namhyung

RE: [PATCH 1/2] perf mmap: Fix perf backward recording

[Liang, Kan]

Hi Namhyung,

> On Wed, Nov 01, 2017 at 04:22:53PM +0000, Liang, Kan wrote:
> > > On 2017/11/1 21:57, Liang, Kan wrote:
> > > >> On 2017/11/1 20:00, Namhyung Kim wrote:
> > > >>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> > > > There are only four test cases which set overwrite,
> > > > sw-clock,task-exit, mmap-basic, backward-ring-buffer.
> > > > Only backward-ring-buffer is 'backward overwrite'.
> > > > The rest three are all 'forward overwrite'. We just need to set
> > > > write_backward to convert them to 'backward overwrite'.
> > > > I think it's not hard to clean up.
> > >
> > > If we add a new rule that overwrite ring buffers are always backward
> > > then it is not hard to cleanup. However, the support of forward
> > > overwrite ring buffer has a long history and the code is not written
> > > by me. I'd like to check if there is some reason to keep support this
> configuration?
> > >
> >
> > As my observation, currently, there are no perf tools which support
> > 'forward overwrite'.
> > There are only three test cases (sw-clock, task-exit, mmap-basic)
> > which is set to 'forward overwrite'. I don’t see any reason it cannot
> > be changed to 'backward overwrite'
> >
> > Arnaldo? Jirka? Kim?
> >
> > What do you think?
> 
> I think sw-clock, task-exit and mmap-basic test cases can be changed to use
> the forward non-overwrite mode.
> 
> The forward overwrite might be used by externel applications accessing the
> ring buffer directly but not needed for perf tools IMHO.  

The proposal is only for perf tool, not kernel. So external applications can still
use forward overwrite to access the ring buffer.

> Let's keep the code simpler as much as possible.

Agree.
Now, there are too many options to access the ring buffer. Not all of them are
supported.
I think we should only keep the crucial options (overwrite/non-overwrite), clearly
define them in the document and cleanup the code.

Also, perf record doesn't use the generic interface (e.g. perf_evlist__mmap*) as other
tools to access ring buffer. Because the generic interface is hardcoded to only support
forward non-overwrite. We should cleanup it as well. But that could be done later
separately.

Thanks,
Kan

Re: [PATCH 1/2] perf mmap: Fix perf backward recording

[Jiri Olsa]

On Thu, Nov 02, 2017 at 01:25:08PM +0000, Liang, Kan wrote:
> Hi Namhyung,
> 
> > On Wed, Nov 01, 2017 at 04:22:53PM +0000, Liang, Kan wrote:
> > > > On 2017/11/1 21:57, Liang, Kan wrote:
> > > > >> On 2017/11/1 20:00, Namhyung Kim wrote:
> > > > >>> On Wed, Nov 01, 2017 at 06:32:50PM +0800, Wangnan (F) wrote:
> > > > > There are only four test cases which set overwrite,
> > > > > sw-clock,task-exit, mmap-basic, backward-ring-buffer.
> > > > > Only backward-ring-buffer is 'backward overwrite'.
> > > > > The rest three are all 'forward overwrite'. We just need to set
> > > > > write_backward to convert them to 'backward overwrite'.
> > > > > I think it's not hard to clean up.
> > > >
> > > > If we add a new rule that overwrite ring buffers are always backward
> > > > then it is not hard to cleanup. However, the support of forward
> > > > overwrite ring buffer has a long history and the code is not written
> > > > by me. I'd like to check if there is some reason to keep support this
> > configuration?
> > > >
> > >
> > > As my observation, currently, there are no perf tools which support
> > > 'forward overwrite'.
> > > There are only three test cases (sw-clock, task-exit, mmap-basic)
> > > which is set to 'forward overwrite'. I don’t see any reason it cannot
> > > be changed to 'backward overwrite'
> > >
> > > Arnaldo? Jirka? Kim?
> > >
> > > What do you think?
> > 
> > I think sw-clock, task-exit and mmap-basic test cases can be changed to use
> > the forward non-overwrite mode.

agreed, we can change them to forward non-overwrite mode

> > The forward overwrite might be used by externel applications accessing the
> > ring buffer directly but not needed for perf tools IMHO.  
> 
> The proposal is only for perf tool, not kernel. So external applications can still
> use forward overwrite to access the ring buffer.
> 
> > Let's keep the code simpler as much as possible.
> 
> Agree.
> Now, there are too many options to access the ring buffer. Not all of them are
> supported.
> I think we should only keep the crucial options (overwrite/non-overwrite), clearly
> define them in the document and cleanup the code.

as you said earlier only 2 modes make sense, so I think perf record should have:

  - forward non-overwrite mode by default
  - backward overwrite mode when '--overwrite' option is set

and make it clear in the docs, maybe in special perf-record man page section

so far I still like the '--overwrite' option more than --flight-recorder' ;-)
also it's been out there for some time now

jirka

[PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Wang Nan]

The meaning of perf record's "overwrite" option and many "overwrite" in
source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
 1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
 2. Set evsel's "backward" attribute (in apply_config_terms).

perf record doesn't use meaning 1 at all, but have a overwrite option, its
real meaning is setting backward.

This patch separates these two concepts, introduce 'flightrecorder' mode
which is what we really want. It combines these 2 concept together, wraps
them into a record mode. In flight recorder mode, perf only dumps data before
something happen.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 tools/perf/Documentation/perf-record.txt |  8 ++++----
 tools/perf/builtin-record.c              |  4 ++--
 tools/perf/perf.h                        |  2 +-
 tools/perf/util/evsel.c                  |  6 +++---
 tools/perf/util/evsel.h                  |  4 ++--
 tools/perf/util/parse-events.c           | 20 ++++++++++----------
 tools/perf/util/parse-events.h           |  4 ++--
 tools/perf/util/parse-events.l           |  4 ++--
 8 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 5a626ef..463c2d3 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -467,19 +467,19 @@ the beginning of record, collect them during finalizing an output file.
 The collected non-sample events reflects the status of the system when
 record is finished.
 
---overwrite::
+--flight-recorder::
 Makes all events use an overwritable ring buffer. An overwritable ring
 buffer works like a flight recorder: when it gets full, the kernel will
 overwrite the oldest records, that thus will never make it to the
 perf.data file.
 
-When '--overwrite' and '--switch-output' are used perf records and drops
+When '--flight-recorder' and '--switch-output' are used perf records and drops
 events until it receives a signal, meaning that something unusual was
 detected that warrants taking a snapshot of the most current events,
 those fitting in the ring buffer at that moment.
 
-'overwrite' attribute can also be set or canceled for an event using
-config terms. For example: 'cycles/overwrite/' and 'instructions/no-overwrite/'.
+'flightrecorder' attribute can also be set or canceled separately for an event using
+config terms. For example: 'cycles/flightrecorder/' and 'instructions/no-flightrecorder/'.
 
 Implies --tail-synthesize.
 
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f4d9fc5..315ea09 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1489,7 +1489,7 @@ static struct option __record_options[] = {
 			"child tasks do not inherit counters"),
 	OPT_BOOLEAN(0, "tail-synthesize", &record.opts.tail_synthesize,
 		    "synthesize non-sample events at the end of output"),
-	OPT_BOOLEAN(0, "overwrite", &record.opts.overwrite, "use overwrite mode"),
+	OPT_BOOLEAN(0, "flight-recoder", &record.opts.flight_recorder, "use flight recoder mode"),
 	OPT_UINTEGER('F', "freq", &record.opts.user_freq, "profile at this frequency"),
 	OPT_CALLBACK('m', "mmap-pages", &record.opts, "pages[,pages]",
 		     "number of mmap data pages and AUX area tracing mmap pages",
@@ -1733,7 +1733,7 @@ int cmd_record(int argc, const char **argv)
 		}
 	}
 
-	if (record.opts.overwrite)
+	if (record.opts.flight_recorder)
 		record.opts.tail_synthesize = true;
 
 	if (rec->evlist->nr_entries == 0 &&
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index fbb0a9c..a7f7618 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -57,7 +57,7 @@ struct record_opts {
 	bool	     all_kernel;
 	bool	     all_user;
 	bool	     tail_synthesize;
-	bool	     overwrite;
+	bool	     flight_recorder;
 	bool	     ignore_missing_thread;
 	unsigned int freq;
 	unsigned int mmap_pages;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index f894893..0e1e8e8 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -772,8 +772,8 @@ static void apply_config_terms(struct perf_evsel *evsel,
 			 */
 			attr->inherit = term->val.inherit ? 1 : 0;
 			break;
-		case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
-			attr->write_backward = term->val.overwrite ? 1 : 0;
+		case PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER:
+			attr->write_backward = term->val.flightrecorder ? 1 : 0;
 			break;
 		default:
 			break;
@@ -856,7 +856,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 
 	attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
 	attr->inherit	    = !opts->no_inherit;
-	attr->write_backward = opts->overwrite ? 1 : 0;
+	attr->write_backward = opts->flight_recorder ? 1 : 0;
 
 	perf_evsel__set_sample_bit(evsel, IP);
 	perf_evsel__set_sample_bit(evsel, TID);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 64782b1..115b637 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -45,7 +45,7 @@ enum {
 	PERF_EVSEL__CONFIG_TERM_STACK_USER,
 	PERF_EVSEL__CONFIG_TERM_INHERIT,
 	PERF_EVSEL__CONFIG_TERM_MAX_STACK,
-	PERF_EVSEL__CONFIG_TERM_OVERWRITE,
+	PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER,
 	PERF_EVSEL__CONFIG_TERM_DRV_CFG,
 	PERF_EVSEL__CONFIG_TERM_BRANCH,
 	PERF_EVSEL__CONFIG_TERM_MAX,
@@ -63,7 +63,7 @@ struct perf_evsel_config_term {
 		u64	stack_user;
 		int	max_stack;
 		bool	inherit;
-		bool	overwrite;
+		bool	flightrecorder;
 		char	*branch;
 	} val;
 };
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 04f35db..61ae3d3 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -914,8 +914,8 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
 	[PARSE_EVENTS__TERM_TYPE_NOINHERIT]		= "no-inherit",
 	[PARSE_EVENTS__TERM_TYPE_INHERIT]		= "inherit",
 	[PARSE_EVENTS__TERM_TYPE_MAX_STACK]		= "max-stack",
-	[PARSE_EVENTS__TERM_TYPE_OVERWRITE]		= "overwrite",
-	[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]		= "no-overwrite",
+	[PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER]	= "flightrecorder",
+	[PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER]	= "no-flightrecorder",
 	[PARSE_EVENTS__TERM_TYPE_DRV_CFG]		= "driver-config",
 };
 
@@ -1013,10 +1013,10 @@ do {									   \
 	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
 		CHECK_TYPE_VAL(NUM);
 		break;
-	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
+	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
 		CHECK_TYPE_VAL(NUM);
 		break;
-	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
+	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
 		CHECK_TYPE_VAL(NUM);
 		break;
 	case PARSE_EVENTS__TERM_TYPE_NAME:
@@ -1072,8 +1072,8 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
 	case PARSE_EVENTS__TERM_TYPE_INHERIT:
 	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
 	case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
-	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
-	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
+	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
+	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
 		return config_term_common(attr, term, err);
 	default:
 		if (err) {
@@ -1149,11 +1149,11 @@ do {								\
 		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
 			ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
 			break;
-		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
-			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+		case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
+			ADD_CONFIG_TERM(FLIGHTRECORDER, flightrecorder, term->val.num ? 1 : 0);
 			break;
-		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
-			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+		case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
+			ADD_CONFIG_TERM(FLIGHTRECORDER, flightrecorder, term->val.num ? 0 : 1);
 			break;
 		case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
 			ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 3909ca0..cd0b4eae 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -70,8 +70,8 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_NOINHERIT,
 	PARSE_EVENTS__TERM_TYPE_INHERIT,
 	PARSE_EVENTS__TERM_TYPE_MAX_STACK,
-	PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
-	PARSE_EVENTS__TERM_TYPE_OVERWRITE,
+	PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER,
+	PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER,
 	PARSE_EVENTS__TERM_TYPE_DRV_CFG,
 	__PARSE_EVENTS__TERM_TYPE_NR,
 };
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 38a42bd..7710d6d 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -251,8 +251,8 @@ stack-size		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_STACKSIZE); }
 max-stack		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_MAX_STACK); }
 inherit			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_INHERIT); }
 no-inherit		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
-no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
+flightrecorder		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER); }
+no-flightrecorder	{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER); }
 ,			{ return ','; }
 "/"			{ BEGIN(INITIAL); return '/'; }
 {name_minus}		{ return str(yyscanner, PE_NAME); }
-- 
2.10.1

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Namhyung Kim]

On Wed, Nov 01, 2017 at 05:53:27AM +0000, Wang Nan wrote:
> The meaning of perf record's "overwrite" option and many "overwrite" in
> source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
>  1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>  2. Set evsel's "backward" attribute (in apply_config_terms).
> 
> perf record doesn't use meaning 1 at all, but have a overwrite option, its
> real meaning is setting backward.
> 
> This patch separates these two concepts, introduce 'flightrecorder' mode
> which is what we really want. It combines these 2 concept together, wraps
> them into a record mode. In flight recorder mode, perf only dumps data before
> something happen.

I'm ok with the it but removing old name looks not good.  How about
keeping them for a while (as deprecated)?.

And 'flightrecorder' seems too long.  Maybe you can use an acronym
like FDR or fdr-mode?

Thanks,
Namhyung


> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  8 ++++----
>  tools/perf/builtin-record.c              |  4 ++--
>  tools/perf/perf.h                        |  2 +-
>  tools/perf/util/evsel.c                  |  6 +++---
>  tools/perf/util/evsel.h                  |  4 ++--
>  tools/perf/util/parse-events.c           | 20 ++++++++++----------
>  tools/perf/util/parse-events.h           |  4 ++--
>  tools/perf/util/parse-events.l           |  4 ++--
>  8 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 5a626ef..463c2d3 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -467,19 +467,19 @@ the beginning of record, collect them during finalizing an output file.
>  The collected non-sample events reflects the status of the system when
>  record is finished.
>  
> ---overwrite::
> +--flight-recorder::
>  Makes all events use an overwritable ring buffer. An overwritable ring
>  buffer works like a flight recorder: when it gets full, the kernel will
>  overwrite the oldest records, that thus will never make it to the
>  perf.data file.
>  
> -When '--overwrite' and '--switch-output' are used perf records and drops
> +When '--flight-recorder' and '--switch-output' are used perf records and drops
>  events until it receives a signal, meaning that something unusual was
>  detected that warrants taking a snapshot of the most current events,
>  those fitting in the ring buffer at that moment.
>  
> -'overwrite' attribute can also be set or canceled for an event using
> -config terms. For example: 'cycles/overwrite/' and 'instructions/no-overwrite/'.
> +'flightrecorder' attribute can also be set or canceled separately for an event using
> +config terms. For example: 'cycles/flightrecorder/' and 'instructions/no-flightrecorder/'.
>  
>  Implies --tail-synthesize.
>  
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4d9fc5..315ea09 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1489,7 +1489,7 @@ static struct option __record_options[] = {
>  			"child tasks do not inherit counters"),
>  	OPT_BOOLEAN(0, "tail-synthesize", &record.opts.tail_synthesize,
>  		    "synthesize non-sample events at the end of output"),
> -	OPT_BOOLEAN(0, "overwrite", &record.opts.overwrite, "use overwrite mode"),
> +	OPT_BOOLEAN(0, "flight-recoder", &record.opts.flight_recorder, "use flight recoder mode"),
>  	OPT_UINTEGER('F', "freq", &record.opts.user_freq, "profile at this frequency"),
>  	OPT_CALLBACK('m', "mmap-pages", &record.opts, "pages[,pages]",
>  		     "number of mmap data pages and AUX area tracing mmap pages",
> @@ -1733,7 +1733,7 @@ int cmd_record(int argc, const char **argv)
>  		}
>  	}
>  
> -	if (record.opts.overwrite)
> +	if (record.opts.flight_recorder)
>  		record.opts.tail_synthesize = true;
>  
>  	if (rec->evlist->nr_entries == 0 &&
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index fbb0a9c..a7f7618 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -57,7 +57,7 @@ struct record_opts {
>  	bool	     all_kernel;
>  	bool	     all_user;
>  	bool	     tail_synthesize;
> -	bool	     overwrite;
> +	bool	     flight_recorder;
>  	bool	     ignore_missing_thread;
>  	unsigned int freq;
>  	unsigned int mmap_pages;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index f894893..0e1e8e8 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -772,8 +772,8 @@ static void apply_config_terms(struct perf_evsel *evsel,
>  			 */
>  			attr->inherit = term->val.inherit ? 1 : 0;
>  			break;
> -		case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
> -			attr->write_backward = term->val.overwrite ? 1 : 0;
> +		case PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER:
> +			attr->write_backward = term->val.flightrecorder ? 1 : 0;
>  			break;
>  		default:
>  			break;
> @@ -856,7 +856,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
>  
>  	attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
>  	attr->inherit	    = !opts->no_inherit;
> -	attr->write_backward = opts->overwrite ? 1 : 0;
> +	attr->write_backward = opts->flight_recorder ? 1 : 0;
>  
>  	perf_evsel__set_sample_bit(evsel, IP);
>  	perf_evsel__set_sample_bit(evsel, TID);
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 64782b1..115b637 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -45,7 +45,7 @@ enum {
>  	PERF_EVSEL__CONFIG_TERM_STACK_USER,
>  	PERF_EVSEL__CONFIG_TERM_INHERIT,
>  	PERF_EVSEL__CONFIG_TERM_MAX_STACK,
> -	PERF_EVSEL__CONFIG_TERM_OVERWRITE,
> +	PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER,
>  	PERF_EVSEL__CONFIG_TERM_DRV_CFG,
>  	PERF_EVSEL__CONFIG_TERM_BRANCH,
>  	PERF_EVSEL__CONFIG_TERM_MAX,
> @@ -63,7 +63,7 @@ struct perf_evsel_config_term {
>  		u64	stack_user;
>  		int	max_stack;
>  		bool	inherit;
> -		bool	overwrite;
> +		bool	flightrecorder;
>  		char	*branch;
>  	} val;
>  };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 04f35db..61ae3d3 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -914,8 +914,8 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
>  	[PARSE_EVENTS__TERM_TYPE_NOINHERIT]		= "no-inherit",
>  	[PARSE_EVENTS__TERM_TYPE_INHERIT]		= "inherit",
>  	[PARSE_EVENTS__TERM_TYPE_MAX_STACK]		= "max-stack",
> -	[PARSE_EVENTS__TERM_TYPE_OVERWRITE]		= "overwrite",
> -	[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]		= "no-overwrite",
> +	[PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER]	= "flightrecorder",
> +	[PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER]	= "no-flightrecorder",
>  	[PARSE_EVENTS__TERM_TYPE_DRV_CFG]		= "driver-config",
>  };
>  
> @@ -1013,10 +1013,10 @@ do {									   \
>  	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
> -	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
> -	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
>  	case PARSE_EVENTS__TERM_TYPE_NAME:
> @@ -1072,8 +1072,8 @@ static int config_term_tracepoint(struct perf_event_attr *attr,
>  	case PARSE_EVENTS__TERM_TYPE_INHERIT:
>  	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
>  	case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> -	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> -	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
> +	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
>  		return config_term_common(attr, term, err);
>  	default:
>  		if (err) {
> @@ -1149,11 +1149,11 @@ do {								\
>  		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
>  			ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
>  			break;
> -		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> +		case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
> +			ADD_CONFIG_TERM(FLIGHTRECORDER, flightrecorder, term->val.num ? 1 : 0);
>  			break;
> -		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> +		case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
> +			ADD_CONFIG_TERM(FLIGHTRECORDER, flightrecorder, term->val.num ? 0 : 1);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
>  			ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 3909ca0..cd0b4eae 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -70,8 +70,8 @@ enum {
>  	PARSE_EVENTS__TERM_TYPE_NOINHERIT,
>  	PARSE_EVENTS__TERM_TYPE_INHERIT,
>  	PARSE_EVENTS__TERM_TYPE_MAX_STACK,
> -	PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
> -	PARSE_EVENTS__TERM_TYPE_OVERWRITE,
> +	PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER,
> +	PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER,
>  	PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>  	__PARSE_EVENTS__TERM_TYPE_NR,
>  };
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 38a42bd..7710d6d 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -251,8 +251,8 @@ stack-size		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_STACKSIZE); }
>  max-stack		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_MAX_STACK); }
>  inherit			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_INHERIT); }
>  no-inherit		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
> -overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
> -no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
> +flightrecorder		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER); }
> +no-flightrecorder	{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER); }
>  ,			{ return ','; }
>  "/"			{ BEGIN(INITIAL); return '/'; }
>  {name_minus}		{ return str(yyscanner, PE_NAME); }
> -- 
> 2.10.1
> 

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Wangnan (F)]

On 2017/11/1 18:03, Namhyung Kim wrote:
> On Wed, Nov 01, 2017 at 05:53:27AM +0000, Wang Nan wrote:
>> The meaning of perf record's "overwrite" option and many "overwrite" in
>> source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
>>   1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>>   2. Set evsel's "backward" attribute (in apply_config_terms).
>>
>> perf record doesn't use meaning 1 at all, but have a overwrite option, its
>> real meaning is setting backward.
>>
>> This patch separates these two concepts, introduce 'flightrecorder' mode
>> which is what we really want. It combines these 2 concept together, wraps
>> them into a record mode. In flight recorder mode, perf only dumps data before
>> something happen.
> I'm ok with the it but removing old name looks not good.  How about
> keeping them for a while (as deprecated)?.

Is there a way to hide '--overwrite' from 'perf record --help' and print 
something
when user really use it?

> And 'flightrecorder' seems too long.  Maybe you can use an acronym
> like FDR or fdr-mode?

fdr-mode is a good name.

Thank you.

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Namhyung Kim]

On Wed, Nov 01, 2017 at 06:17:26PM +0800, Wangnan (F) wrote:
> 
> 
> On 2017/11/1 18:03, Namhyung Kim wrote:
> > On Wed, Nov 01, 2017 at 05:53:27AM +0000, Wang Nan wrote:
> > > The meaning of perf record's "overwrite" option and many "overwrite" in
> > > source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
> > >   1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
> > >   2. Set evsel's "backward" attribute (in apply_config_terms).
> > > 
> > > perf record doesn't use meaning 1 at all, but have a overwrite option, its
> > > real meaning is setting backward.
> > > 
> > > This patch separates these two concepts, introduce 'flightrecorder' mode
> > > which is what we really want. It combines these 2 concept together, wraps
> > > them into a record mode. In flight recorder mode, perf only dumps data before
> > > something happen.
> > I'm ok with the it but removing old name looks not good.  How about
> > keeping them for a while (as deprecated)?.
> 
> Is there a way to hide '--overwrite' from 'perf record --help' and print
> something
> when user really use it?

You can set PARSE_OPT_HIDDEN flag to it.

Thanks,
Namhyung


> 
> > And 'flightrecorder' seems too long.  Maybe you can use an acronym
> > like FDR or fdr-mode?
> 
> fdr-mode is a good name.
> 
> Thank you.
> 

RE: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Liang, Kan]

> The meaning of perf record's "overwrite" option and many "overwrite" in
> source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
>  1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>  2. Set evsel's "backward" attribute (in apply_config_terms).
> 
> perf record doesn't use meaning 1 at all, but have a overwrite option, its
> real meaning is setting backward.
>

I don't understand here.
'overwrite' has 2 meanings. perf record only support 1.
It should be a bug, and need to be fixed.
Why do we need a new mode? 

I think a new mode just make the codes more complex.

Thanks,
Kan

> This patch separates these two concepts, introduce 'flightrecorder' mode
> which is what we really want. It combines these 2 concept together, wraps
> them into a record mode. In flight recorder mode, perf only dumps data
> before
> something happen.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  8 ++++----
>  tools/perf/builtin-record.c              |  4 ++--
>  tools/perf/perf.h                        |  2 +-
>  tools/perf/util/evsel.c                  |  6 +++---
>  tools/perf/util/evsel.h                  |  4 ++--
>  tools/perf/util/parse-events.c           | 20 ++++++++++----------
>  tools/perf/util/parse-events.h           |  4 ++--
>  tools/perf/util/parse-events.l           |  4 ++--
>  8 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt
> b/tools/perf/Documentation/perf-record.txt
> index 5a626ef..463c2d3 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -467,19 +467,19 @@ the beginning of record, collect them during
> finalizing an output file.
>  The collected non-sample events reflects the status of the system when
>  record is finished.
> 
> ---overwrite::
> +--flight-recorder::
>  Makes all events use an overwritable ring buffer. An overwritable ring
>  buffer works like a flight recorder: when it gets full, the kernel will
>  overwrite the oldest records, that thus will never make it to the
>  perf.data file.
> 
> -When '--overwrite' and '--switch-output' are used perf records and drops
> +When '--flight-recorder' and '--switch-output' are used perf records and
> drops
>  events until it receives a signal, meaning that something unusual was
>  detected that warrants taking a snapshot of the most current events,
>  those fitting in the ring buffer at that moment.
> 
> -'overwrite' attribute can also be set or canceled for an event using
> -config terms. For example: 'cycles/overwrite/' and 'instructions/no-
> overwrite/'.
> +'flightrecorder' attribute can also be set or canceled separately for an event
> using
> +config terms. For example: 'cycles/flightrecorder/' and 'instructions/no-
> flightrecorder/'.
> 
>  Implies --tail-synthesize.
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f4d9fc5..315ea09 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1489,7 +1489,7 @@ static struct option __record_options[] = {
>  			"child tasks do not inherit counters"),
>  	OPT_BOOLEAN(0, "tail-synthesize", &record.opts.tail_synthesize,
>  		    "synthesize non-sample events at the end of output"),
> -	OPT_BOOLEAN(0, "overwrite", &record.opts.overwrite, "use
> overwrite mode"),
> +	OPT_BOOLEAN(0, "flight-recoder", &record.opts.flight_recorder,
> "use flight recoder mode"),
>  	OPT_UINTEGER('F', "freq", &record.opts.user_freq, "profile at this
> frequency"),
>  	OPT_CALLBACK('m', "mmap-pages", &record.opts, "pages[,pages]",
>  		     "number of mmap data pages and AUX area tracing mmap
> pages",
> @@ -1733,7 +1733,7 @@ int cmd_record(int argc, const char **argv)
>  		}
>  	}
> 
> -	if (record.opts.overwrite)
> +	if (record.opts.flight_recorder)
>  		record.opts.tail_synthesize = true;
> 
>  	if (rec->evlist->nr_entries == 0 &&
> diff --git a/tools/perf/perf.h b/tools/perf/perf.h
> index fbb0a9c..a7f7618 100644
> --- a/tools/perf/perf.h
> +++ b/tools/perf/perf.h
> @@ -57,7 +57,7 @@ struct record_opts {
>  	bool	     all_kernel;
>  	bool	     all_user;
>  	bool	     tail_synthesize;
> -	bool	     overwrite;
> +	bool	     flight_recorder;
>  	bool	     ignore_missing_thread;
>  	unsigned int freq;
>  	unsigned int mmap_pages;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index f894893..0e1e8e8 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -772,8 +772,8 @@ static void apply_config_terms(struct perf_evsel
> *evsel,
>  			 */
>  			attr->inherit = term->val.inherit ? 1 : 0;
>  			break;
> -		case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
> -			attr->write_backward = term->val.overwrite ? 1 : 0;
> +		case PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER:
> +			attr->write_backward = term->val.flightrecorder ? 1 :
> 0;
>  			break;
>  		default:
>  			break;
> @@ -856,7 +856,7 @@ void perf_evsel__config(struct perf_evsel *evsel,
> struct record_opts *opts,
> 
>  	attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
>  	attr->inherit	    = !opts->no_inherit;
> -	attr->write_backward = opts->overwrite ? 1 : 0;
> +	attr->write_backward = opts->flight_recorder ? 1 : 0;
> 
>  	perf_evsel__set_sample_bit(evsel, IP);
>  	perf_evsel__set_sample_bit(evsel, TID);
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 64782b1..115b637 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -45,7 +45,7 @@ enum {
>  	PERF_EVSEL__CONFIG_TERM_STACK_USER,
>  	PERF_EVSEL__CONFIG_TERM_INHERIT,
>  	PERF_EVSEL__CONFIG_TERM_MAX_STACK,
> -	PERF_EVSEL__CONFIG_TERM_OVERWRITE,
> +	PERF_EVSEL__CONFIG_TERM_FLIGHTRECORDER,
>  	PERF_EVSEL__CONFIG_TERM_DRV_CFG,
>  	PERF_EVSEL__CONFIG_TERM_BRANCH,
>  	PERF_EVSEL__CONFIG_TERM_MAX,
> @@ -63,7 +63,7 @@ struct perf_evsel_config_term {
>  		u64	stack_user;
>  		int	max_stack;
>  		bool	inherit;
> -		bool	overwrite;
> +		bool	flightrecorder;
>  		char	*branch;
>  	} val;
>  };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 04f35db..61ae3d3 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -914,8 +914,8 @@ static const char
> *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
>  	[PARSE_EVENTS__TERM_TYPE_NOINHERIT]		= "no-inherit",
>  	[PARSE_EVENTS__TERM_TYPE_INHERIT]		= "inherit",
>  	[PARSE_EVENTS__TERM_TYPE_MAX_STACK]		= "max-stack",
> -	[PARSE_EVENTS__TERM_TYPE_OVERWRITE]		= "overwrite",
> -	[PARSE_EVENTS__TERM_TYPE_NOOVERWRITE]		= "no-
> overwrite",
> +	[PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER]	=
> "flightrecorder",
> +	[PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER]	= "no-
> flightrecorder",
>  	[PARSE_EVENTS__TERM_TYPE_DRV_CFG]		= "driver-
> config",
>  };
> 
> @@ -1013,10 +1013,10 @@ do {
> 				   \
>  	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
> -	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
> -	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
>  		CHECK_TYPE_VAL(NUM);
>  		break;
>  	case PARSE_EVENTS__TERM_TYPE_NAME:
> @@ -1072,8 +1072,8 @@ static int config_term_tracepoint(struct
> perf_event_attr *attr,
>  	case PARSE_EVENTS__TERM_TYPE_INHERIT:
>  	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
>  	case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> -	case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> -	case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> +	case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
> +	case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
>  		return config_term_common(attr, term, err);
>  	default:
>  		if (err) {
> @@ -1149,11 +1149,11 @@ do {
> 			\
>  		case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
>  			ADD_CONFIG_TERM(MAX_STACK, max_stack, term-
> >val.num);
>  			break;
> -		case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term-
> >val.num ? 1 : 0);
> +		case PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER:
> +			ADD_CONFIG_TERM(FLIGHTRECORDER,
> flightrecorder, term->val.num ? 1 : 0);
>  			break;
> -		case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> -			ADD_CONFIG_TERM(OVERWRITE, overwrite, term-
> >val.num ? 0 : 1);
> +		case PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER:
> +			ADD_CONFIG_TERM(FLIGHTRECORDER,
> flightrecorder, term->val.num ? 0 : 1);
>  			break;
>  		case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
>  			ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term-
> >val.str);
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 3909ca0..cd0b4eae 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -70,8 +70,8 @@ enum {
>  	PARSE_EVENTS__TERM_TYPE_NOINHERIT,
>  	PARSE_EVENTS__TERM_TYPE_INHERIT,
>  	PARSE_EVENTS__TERM_TYPE_MAX_STACK,
> -	PARSE_EVENTS__TERM_TYPE_NOOVERWRITE,
> -	PARSE_EVENTS__TERM_TYPE_OVERWRITE,
> +	PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER,
> +	PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER,
>  	PARSE_EVENTS__TERM_TYPE_DRV_CFG,
>  	__PARSE_EVENTS__TERM_TYPE_NR,
>  };
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 38a42bd..7710d6d 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -251,8 +251,8 @@ stack-size		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_STACKSIZE); }
>  max-stack		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_MAX_STACK); }
>  inherit			{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_INHERIT); }
>  no-inherit		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_NOINHERIT); }
> -overwrite		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_OVERWRITE); }
> -no-overwrite		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
> +flightrecorder		{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_FLIGHTRECORDER); }
> +no-flightrecorder	{ return term(yyscanner,
> PARSE_EVENTS__TERM_TYPE_NOFLIGHTRECORDER); }
>  ,			{ return ','; }
>  "/"			{ BEGIN(INITIAL); return '/'; }
>  {name_minus}		{ return str(yyscanner, PE_NAME); }
> --
> 2.10.1

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Wangnan (F)]

On 2017/11/1 21:26, Liang, Kan wrote:
>> The meaning of perf record's "overwrite" option and many "overwrite" in
>> source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
>>   1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>>   2. Set evsel's "backward" attribute (in apply_config_terms).
>>
>> perf record doesn't use meaning 1 at all, but have a overwrite option, its
>> real meaning is setting backward.
>>
> I don't understand here.
> 'overwrite' has 2 meanings. perf record only support 1.
> It should be a bug, and need to be fixed.

Not a bug, but ambiguous.

Perf record doesn't need overwrite main channel (we have two channels:
evlist->mmap is main channel and evlist->backward_mmap is backward channel),
but some testcases require it, and your new patchset may require it.
'perf record --overwrite' doesn't set main channel overwrite. What it does
is moving all evsels to backward channel, and we can move some evsels back
to the main channel by /no-overwrite/ setting. This behavior is hard to
understand.

Thank you.

RE: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Liang, Kan]

> On 2017/11/1 21:26, Liang, Kan wrote:
> >> The meaning of perf record's "overwrite" option and many "overwrite"
> >> in source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
> >>   1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
> >>   2. Set evsel's "backward" attribute (in apply_config_terms).
> >>
> >> perf record doesn't use meaning 1 at all, but have a overwrite
> >> option, its real meaning is setting backward.
> >>
> > I don't understand here.
> > 'overwrite' has 2 meanings. perf record only support 1.
> > It should be a bug, and need to be fixed.
> 
> Not a bug, but ambiguous.
> 
> Perf record doesn't need overwrite main channel (we have two channels:
> evlist->mmap is main channel and evlist->backward_mmap is backward
> evlist->channel),
> but some testcases require it, and your new patchset may require it.
> 'perf record --overwrite' doesn't set main channel overwrite. What it does is
> moving all evsels to backward channel, and we can move some evsels back to
> the main channel by /no-overwrite/ setting. This behavior is hard to
> understand.
> 

As my understanding, the 'main channel' should depends on what user sets.
If --overwrite is applied, then evlist->backward_mmap should be the
'main channel'. evlist->overwrite should be set to true as well.

/no-overwrite/ setting is per-event setting.
Only when we finish the global setting, then the per-event setting will be
considered. You may refer to apply_config_terms.

Thanks,
Kan

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Wangnan (F)]

On 2017/11/1 22:22, Liang, Kan wrote:
>> On 2017/11/1 21:26, Liang, Kan wrote:
>>>> The meaning of perf record's "overwrite" option and many "overwrite"
>>>> in source code are not clear. In perf's code, the 'overwrite' has 2 meanings:
>>>>    1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>>>>    2. Set evsel's "backward" attribute (in apply_config_terms).
>>>>
>>>> perf record doesn't use meaning 1 at all, but have a overwrite
>>>> option, its real meaning is setting backward.
>>>>
>>> I don't understand here.
>>> 'overwrite' has 2 meanings. perf record only support 1.
>>> It should be a bug, and need to be fixed.
>> Not a bug, but ambiguous.
>>
>> Perf record doesn't need overwrite main channel (we have two channels:
>> evlist->mmap is main channel and evlist->backward_mmap is backward
>> evlist->channel),
>> but some testcases require it, and your new patchset may require it.
>> 'perf record --overwrite' doesn't set main channel overwrite. What it does is
>> moving all evsels to backward channel, and we can move some evsels back to
>> the main channel by /no-overwrite/ setting. This behavior is hard to
>> understand.
>>
> As my understanding, the 'main channel' should depends on what user sets.
> If --overwrite is applied, then evlist->backward_mmap should be the
> 'main channel'. evlist->overwrite should be set to true as well.

Then it introduces a main channel switching mechanism, and we need
checking evlist->overwrite and another factor to determine which
one is the main channel. Make things more complex.

> /no-overwrite/ setting is per-event setting.
> Only when we finish the global setting, then the per-event setting will be
> considered. You may refer to apply_config_terms.

Yes.

> Thanks,
> Kan
>
>
>
>

RE: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Liang, Kan]

> On 2017/11/1 22:22, Liang, Kan wrote:
> >> On 2017/11/1 21:26, Liang, Kan wrote:
> >>>> The meaning of perf record's "overwrite" option and many "overwrite"
> >>>> in source code are not clear. In perf's code, the 'overwrite' has 2
> meanings:
> >>>>    1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
> >>>>    2. Set evsel's "backward" attribute (in apply_config_terms).
> >>>>
> >>>> perf record doesn't use meaning 1 at all, but have a overwrite
> >>>> option, its real meaning is setting backward.
> >>>>
> >>> I don't understand here.
> >>> 'overwrite' has 2 meanings. perf record only support 1.
> >>> It should be a bug, and need to be fixed.
> >> Not a bug, but ambiguous.
> >>
> >> Perf record doesn't need overwrite main channel (we have two channels:
> >> evlist->mmap is main channel and evlist->backward_mmap is backward
> >> evlist->channel),
> >> but some testcases require it, and your new patchset may require it.
> >> 'perf record --overwrite' doesn't set main channel overwrite. What it does
> is
> >> moving all evsels to backward channel, and we can move some evsels
> back to
> >> the main channel by /no-overwrite/ setting. This behavior is hard to
> >> understand.
> >>
> > As my understanding, the 'main channel' should depends on what user sets.
> > If --overwrite is applied, then evlist->backward_mmap should be the
> > 'main channel'. evlist->overwrite should be set to true as well.
> 
> Then it introduces a main channel switching mechanism, and we need
> checking evlist->overwrite and another factor to determine which
> one is the main channel. Make things more complex.

We should check the evlist->overwrite.
Now, all perf tools force evlist->overwrite = false. I think it doesn’t make sense.

What is another factor?

I don't think it will be too complex.

In perf_evlist__mmap_ex, we just need to add a check.
If (!overwrite)
	evlist->mmap = perf_evlist__alloc_mmap(evlist);
else
	evlist->backward_mmap = perf_evlist__alloc_mmap(evlist);

In perf_evlist__mmap_per_evsel, we already handle per-event overwrite.
It just need to add some similar codes to handler per-event nonoverwrite.  

For other codes, they should already check evlist->mmap and evlist->backward_mmap.
So they probably don't need to change.


Thanks,
Kan

Re: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Wangnan (F)]

On 2017/11/1 23:04, Liang, Kan wrote:
>> On 2017/11/1 22:22, Liang, Kan wrote:
>>>> On 2017/11/1 21:26, Liang, Kan wrote:
>>>>>> The meaning of perf record's "overwrite" option and many "overwrite"
>>>>>> in source code are not clear. In perf's code, the 'overwrite' has 2
>> meanings:
>>>>>>     1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
>>>>>>     2. Set evsel's "backward" attribute (in apply_config_terms).
>>>>>>
>>>>>> perf record doesn't use meaning 1 at all, but have a overwrite
>>>>>> option, its real meaning is setting backward.
>>>>>>
>>>>> I don't understand here.
>>>>> 'overwrite' has 2 meanings. perf record only support 1.
>>>>> It should be a bug, and need to be fixed.
>>>> Not a bug, but ambiguous.
>>>>
>>>> Perf record doesn't need overwrite main channel (we have two channels:
>>>> evlist->mmap is main channel and evlist->backward_mmap is backward
>>>> evlist->channel),
>>>> but some testcases require it, and your new patchset may require it.
>>>> 'perf record --overwrite' doesn't set main channel overwrite. What it does
>> is
>>>> moving all evsels to backward channel, and we can move some evsels
>> back to
>>>> the main channel by /no-overwrite/ setting. This behavior is hard to
>>>> understand.
>>>>
>>> As my understanding, the 'main channel' should depends on what user sets.
>>> If --overwrite is applied, then evlist->backward_mmap should be the
>>> 'main channel'. evlist->overwrite should be set to true as well.
>> Then it introduces a main channel switching mechanism, and we need
>> checking evlist->overwrite and another factor to determine which
>> one is the main channel. Make things more complex.
> We should check the evlist->overwrite.
> Now, all perf tools force evlist->overwrite = false. I think it doesn’t make sense.
>
> What is another factor?

If we support mixed channel as well as forward overwrite ring buffer,
evlist->overwrite is not enough.

> I don't think it will be too complex.
>
> In perf_evlist__mmap_ex, we just need to add a check.
> If (!overwrite)
> 	evlist->mmap = perf_evlist__alloc_mmap(evlist);
> else
> 	evlist->backward_mmap = perf_evlist__alloc_mmap(evlist);
>
> In perf_evlist__mmap_per_evsel, we already handle per-event overwrite.
> It just need to add some similar codes to handler per-event nonoverwrite.

Then the logic becomes:

  if (check write_backward) {
     maps = evlist->backward_mmap;
     if (!maps) {
       maps = perf_evlist__alloc_mmap(...);
       if (!maps) {
           /* error processing */
       }
       evlist->backward_mmap = maps;
       if (evlist->bkw_mmap_state == BKW_MMAP_NOTREADY)
         perf_evlist__toggle_bkw_mmap(evlist, BKW_MMAP_RUNNING);
     }
  } else {
     maps = evlist->mmap;
     if (!maps) {
       maps = perf_evlist__alloc_mmap(...);
       if (!maps) {
           /* error processing */
       }
       evlist->mmap = maps;
       ....
     }
  }

> For other codes, they should already check evlist->mmap and evlist->backward_mmap.
> So they probably don't need to change.
>
>
> Thanks,
> Kan
>
>

RE: [PATCH 2/2] perf record: Replace 'overwrite' by 'flightrecorder' for better naming

[Liang, Kan]

 > On 2017/11/1 23:04, Liang, Kan wrote:
> >> On 2017/11/1 22:22, Liang, Kan wrote:
> >>>> On 2017/11/1 21:26, Liang, Kan wrote:
> >>>>>> The meaning of perf record's "overwrite" option and many
> "overwrite"
> >>>>>> in source code are not clear. In perf's code, the 'overwrite' has
> >>>>>> 2
> >> meanings:
> >>>>>>     1. Make ringbuffer readonly (perf_evlist__mmap_ex's argument).
> >>>>>>     2. Set evsel's "backward" attribute (in apply_config_terms).
> >>>>>>
> >>>>>> perf record doesn't use meaning 1 at all, but have a overwrite
> >>>>>> option, its real meaning is setting backward.
> >>>>>>
> >>>>> I don't understand here.
> >>>>> 'overwrite' has 2 meanings. perf record only support 1.
> >>>>> It should be a bug, and need to be fixed.
> >>>> Not a bug, but ambiguous.
> >>>>
> >>>> Perf record doesn't need overwrite main channel (we have two
> channels:
> >>>> evlist->mmap is main channel and evlist->backward_mmap is backward
> >>>> evlist->channel),
> >>>> but some testcases require it, and your new patchset may require it.
> >>>> 'perf record --overwrite' doesn't set main channel overwrite. What
> >>>> it does
> >> is
> >>>> moving all evsels to backward channel, and we can move some evsels
> >> back to
> >>>> the main channel by /no-overwrite/ setting. This behavior is hard
> >>>> to understand.
> >>>>
> >>> As my understanding, the 'main channel' should depends on what user
> sets.
> >>> If --overwrite is applied, then evlist->backward_mmap should be the
> >>> 'main channel'. evlist->overwrite should be set to true as well.
> >> Then it introduces a main channel switching mechanism, and we need
> >> checking evlist->overwrite and another factor to determine which one
> >> is the main channel. Make things more complex.
> > We should check the evlist->overwrite.
> > Now, all perf tools force evlist->overwrite = false. I think it doesn’t make
> sense.
> >
> > What is another factor?
> 
> If we support mixed channel as well as forward overwrite ring buffer,
> evlist->overwrite is not enough.

I think you have a detailed analysis regarding to the weakness of 'forward overwrite'.
commit ID 9ecda41acb97 ("perf/core: Add ::write_backward attribute to perf event").

There is no perf tools use 'forward overwrite' mode.

The only users are three perf test cases. We can change them to 'backward overwrite'

I think it's OK to discard the 'forward overwrite' mode

> 
> > I don't think it will be too complex.
> >
> > In perf_evlist__mmap_ex, we just need to add a check.
> > If (!overwrite)
> > 	evlist->mmap = perf_evlist__alloc_mmap(evlist); else
> > 	evlist->backward_mmap = perf_evlist__alloc_mmap(evlist);
> >
> > In perf_evlist__mmap_per_evsel, we already handle per-event overwrite.
> > It just need to add some similar codes to handler per-event nonoverwrite.
> 
> Then the logic becomes:
> 
>   if (check write_backward) {
>      maps = evlist->backward_mmap;
>      if (!maps) {
>        maps = perf_evlist__alloc_mmap(...);
>        if (!maps) {
>            /* error processing */
>        }
>        evlist->backward_mmap = maps;
>        if (evlist->bkw_mmap_state == BKW_MMAP_NOTREADY)
>          perf_evlist__toggle_bkw_mmap(evlist, BKW_MMAP_RUNNING);
>      }
>   } else {
>      maps = evlist->mmap;
>      if (!maps) {
>        maps = perf_evlist__alloc_mmap(...);
>        if (!maps) {
>            /* error processing */
>        }
>        evlist->mmap = maps;
>        ....
>      }
>   }
> 

Thanks.
It looks good to me.

Kan

> > For other codes, they should already check evlist->mmap and evlist-
> >backward_mmap.
> > So they probably don't need to change.
> >
> >
> > Thanks,
> > Kan
> >
> >
>