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  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:	Sun, 15 Jun 2014 19:17:30 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Jiri Olsa <jolsa@...nel.org>, linux-kernel@...r.kernel.org,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
	David Ahern <dsahern@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...nel.org>,
	Jean Pihet <jean.pihet@...aro.org>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH 01/17] perf tools: Always force
 PERF_RECORD_FINISHED_ROUND event

On Fri, Jun 13, 2014 at 08:51:54PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> 2014-06-13 (금), 00:08 +0200, Jiri Olsa:
> > The PERF_RECORD_FINISHED_ROUND governs queue flushing in
> > reporting, so it needs to be stored for any kind of event.
> > 
> > Forcing the PERF_RECORD_FINISHED_ROUND event to be stored any
> > time we finish the round and wrote at least one event.
> > 
> > Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> > Cc: Corey Ashford <cjashfor@...ux.vnet.ibm.com>
> > Cc: David Ahern <dsahern@...il.com>
> > Cc: Frederic Weisbecker <fweisbec@...il.com>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Jean Pihet <jean.pihet@...aro.org>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > Cc: Paul Mackerras <paulus@...ba.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  tools/perf/builtin-record.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 378b85b..4869050 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -238,6 +238,7 @@ static struct perf_event_header finished_round_event = {
> >  
> >  static int record__mmap_read_all(struct record *rec)
> >  {
> > +	u64 bytes_written = rec->bytes_written;
> >  	int i;
> >  	int rc = 0;
> >  
> > @@ -250,7 +251,11 @@ static int record__mmap_read_all(struct record *rec)
> >  		}
> >  	}
> >  
> > -	if (perf_header__has_feat(&rec->session->header, HEADER_TRACING_DATA))
> > +	/*
> > +	 * Mark the round finished in case we wrote
> > +	 * at least one event.
> > +	 */
> > +	if (bytes_written != rec->bytes_written)
> >  		rc = record__write(rec, &finished_round_event, sizeof(finished_round_event));
> 
> Hmm.. what was the rational behind the original code?  Why did it flush
> the events only if session has tracepoint events?  Frederic?

looks like the reason was the reordering in report, as described
in commit 984028075794c00cbf4fb1e94bb6233e8be08875

but there's no info why is this only for tracepoints
(or when tracepoints events are included)

> I guess this change alone can impact the performance in your case.
> Jiri, do you have a test result of it?

the impact is an extra 64 bytes (event header size) in perf.data
each time we read data from all cpus

I can see the impact more on the report size, where this events
governs the flushing of the reordering queue. If there's no
'finish_round' event, the queue is flushed at the end of the
processing (which leads to issues I explained in patch 0).

Now this problem is workarounded by this patchset via using the
HALF flush any time we reach the maximum of the reordering queue
size.

Still I think it's better to have 'finish_round' event for
any kind of event workloads, to keep the standard/expected
reordering processing in the report command.

thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists