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