linux-kernel - RE: [PATCH V3 04/12] perf mmap: introduce perf_mmap__read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Jan 2018 14:15:38 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     "acme@...nel.org" <acme@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "wangnan0@...wei.com" <wangnan0@...wei.com>,
        "jolsa@...nel.org" <jolsa@...nel.org>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "yao.jin@...ux.intel.com" <yao.jin@...ux.intel.com>,
        "kernel-team@....com" <kernel-team@....com>
Subject: RE: [PATCH V3 04/12] perf mmap: introduce perf_mmap__read_done

> Hello,
> 
> On Thu, Dec 21, 2017 at 10:08:46AM -0800, kan.liang@...el.com wrote:
> > From: Kan Liang <kan.liang@...el.com>
> >
> > The direction of overwrite mode is backward. The last mmap__read_event
> > will set tail to map->prev. Need to correct the map->prev to head
> > which is the end of next read.
> 
> Why do you update the map->prev needlessly then?  I think we don't need it
> for overwrite/backward mode, right?

The map->prev is needless only when the overwrite does really happen in ringbuffer.
In a light load system or with big ringbuffer, the unprocessed data will not be
overwritten. So it's necessary to keep an pointer to indicate the last position.

Overwrite mode is backward, but the event processing is always forward.
So map->prev has to be updated in __read_done().

> 
> Also I guess the current code might miss some events since the head can be
> different between _read_init() and _read_done(), no?
> 

The overwrite mode requires the ring buffer to be paused during processing.
The head is unchanged between __read_init() and __read_done().

The event during the pause should be missed. But I think it has little impact for
the accuracy of the snapshot and can be tolerant for perf top.
I mentioned it in the change log of patch 11/12.
I also removed the lost events checking for perf top.

Thanks,
Kan

> Thanks,
> Namhyung
> 
> 
> >
> > It will be used later.
> >
> > Signed-off-by: Kan Liang <kan.liang@...el.com>
> > ---
> >  tools/perf/util/mmap.c | 11 +++++++++++  tools/perf/util/mmap.h |  1
> > +
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c index
> > a844a2f..4aaeb64 100644
> > --- a/tools/perf/util/mmap.c
> > +++ b/tools/perf/util/mmap.c
> > @@ -343,3 +343,14 @@ int perf_mmap__push(struct perf_mmap *md,
> bool
> > overwrite,
> >  out:
> >  	return rc;
> >  }
> > +
> > +/*
> > + * Mandatory for overwrite mode
> > + * The direction of overwrite mode is backward.
> > + * The last mmap__read_event will set tail to map->prev.
> > + * Need to correct the map->prev to head which is the end of next read.
> > + */
> > +void perf_mmap__read_done(struct perf_mmap *map) {
> > +	map->prev = perf_mmap__read_head(map); }
> > diff --git a/tools/perf/util/mmap.h b/tools/perf/util/mmap.h index
> > abe9b9f..2df27c1 100644
> > --- a/tools/perf/util/mmap.h
> > +++ b/tools/perf/util/mmap.h
> > @@ -96,4 +96,5 @@ size_t perf_mmap__mmap_len(struct perf_mmap
> *map);
> >
> >  int perf_mmap__read_init(struct perf_mmap *map, bool overwrite,
> >  			 u64 *start, u64 *end);
> > +void perf_mmap__read_done(struct perf_mmap *map);
> >  #endif /*__PERF_MMAP_H */
> > --
> > 2.5.5
> >