LinuxLists.cc - [PATCH 1/4] perf tools: Use perf_evsel__match in perf_evsel__is_bpf

[permalink] [raw]

Subject: [PATCH 4/4] perf tools: Move syscall_64.tbl check into check-headers.sh

Probably leftover from the time we introducd the check-headers.sh script.

Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/arch/x86/Makefile | 3 ---
tools/perf/check-headers.sh | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
index 1a38e78117ce..8cc6642fce7a 100644
--- a/tools/perf/arch/x86/Makefile
+++ b/tools/perf/arch/x86/Makefile
@@ -19,9 +19,6 @@ systbl := $(sys)/syscalltbl.sh
_dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')

$(header): $(sys)/syscall_64.tbl $(systbl)
- @(test -d ../../kernel -a -d ../../tools -a -d ../perf && ( \
- (diff -B arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl >/dev/null) \
- || echo "Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'" >&2 )) || true
$(Q)$(SHELL) '$(systbl)' $(sys)/syscall_64.tbl 'x86_64' > $@

clean::
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 73e723675c5f..2c10e46d98e9 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -93,3 +93,6 @@ check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>"'
check include/uapi/asm-generic/mman.h '-I "^#include <$uapi/$*asm-generic/mman-common.h>"'
check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
+
+# diff non-symmetric files
+check_2 arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl
--
2.17.1

2018-07-20 11:01:54

[permalink] [raw]

Subject: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

The warning message in check_w function uses wrongly
the $file variable instead of $file1 and $file2.

Fixes: 582472973593 ("perf check-headers.sh: Add support to check 2 independent files")
Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/check-headers.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 814aaf269949..73e723675c5f 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,7 +67,7 @@ check_2 () {
cmd="diff $* $file1 $file2 > /dev/null"

test -f $file2 &&
- eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+ eval $cmd || echo "Warning: Kernel ABI header at '$file1' differs from latest version at '$file2'" >&2
}

check () {
--
2.17.1

2018-07-20 11:02:38

[permalink] [raw]

Subject: [PATCH 2/4] perf stat: Get rid of extra clock display function

There's no reason to have separate function to display clock
events. It's only purpose was to convert the nanosecond value
into microseconds. We do that now in generic code, if the unit
and scale values are properly set, which this patch do for
clock events.

The output differs in the unit field being displayed in its
columns rather than having it added as a suffix of the event
name. Plus the value is rounded into 2 decimal numbers as
for any other event.

Before:
# perf stat -e cpu-clock,task-clock -C 0 sleep 3

Performance counter stats for 'CPU(s) 0':

3001.123137 cpu-clock (msec) # 1.000 CPUs utilized
3001.133250 task-clock (msec) # 1.000 CPUs utilized

3.001159813 seconds time elapsed

Now:
# perf stat -e cpu-clock,task-clock -C 0 sleep 3

Performance counter stats for 'CPU(s) 0':

3,001.05 msec cpu-clock # 1.000 CPUs utilized
3,001.05 msec task-clock # 1.000 CPUs utilized

3.001077794 seconds time elapsed

There's small difference in csv output, as we now output the unit
field, which was empty before. It's in the proper spot, so there's
no compatibility issue.

Before:

# perf stat -e cpu-clock,task-clock -C 0 -x, sleep 3
3001.065177,,cpu-clock,3001064187,100.00,1.000,CPUs utilized
3001.077085,,task-clock,3001077085,100.00,1.000,CPUs utilized

# perf stat -e cpu-clock,task-clock -C 0 -x, sleep 3
3000.80,msec,cpu-clock,3000799026,100.00,1.000,CPUs utilized
3000.80,msec,task-clock,3000799550,100.00,1.000,CPUs utilized

Adding perf_evsel__is_clock to replace nsec_counter.

Cc: Andi Kleen <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/builtin-stat.c | 48 ++---------------------------------
tools/perf/util/evsel.c | 11 ++++++++
tools/perf/util/evsel.h | 6 +++++
tools/perf/util/stat-shadow.c | 5 ++--
4 files changed, 21 insertions(+), 49 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index dfd13d6e2931..d097b5b47eb8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -296,18 +296,6 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
return perf_evsel__open_per_thread(evsel, evsel_list->threads);
}

-/*
- * Does the counter have nsecs as a unit?
- */
-static inline int nsec_counter(struct perf_evsel *evsel)
-{
- if (perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK) ||
- perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK))
- return 1;
-
- return 0;
-}
-
static int process_synthesized_event(struct perf_tool *tool __maybe_unused,
union perf_event *event,
struct perf_sample *sample __maybe_unused,
@@ -1058,34 +1046,6 @@ static void print_metric_header(void *ctx, const char *color __maybe_unused,
fprintf(os->fh, "%*s ", metric_only_len, unit);
}

-static void nsec_printout(int id, int nr, struct perf_evsel *evsel, double avg)
-{
- FILE *output = stat_config.output;
- double msecs = avg / NSEC_PER_MSEC;
- const char *fmt_v, *fmt_n;
- char name[25];
-
- fmt_v = csv_output ? "%.6f%s" : "%18.6f%s";
- fmt_n = csv_output ? "%s" : "%-25s";
-
- aggr_printout(evsel, id, nr);
-
- scnprintf(name, sizeof(name), "%s%s",
- perf_evsel__name(evsel), csv_output ? "" : " (msec)");
-
- fprintf(output, fmt_v, msecs, csv_sep);
-
- if (csv_output)
- fprintf(output, "%s%s", evsel->unit, csv_sep);
- else
- fprintf(output, "%-*s%s", unit_width, evsel->unit, csv_sep);
-
- fprintf(output, fmt_n, name);
-
- if (evsel->cgrp)
- fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
-}
-
static int first_shadow_cpu(struct perf_evsel *evsel, int id)
{
int i;
@@ -1241,11 +1201,7 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
return;
}

- if (metric_only)
- /* nothing */;
- else if (nsec_counter(counter))
- nsec_printout(id, nr, counter, uval);
- else
+ if (!metric_only)
abs_printout(id, nr, counter, uval);

out.print_metric = pm;
@@ -1331,7 +1287,7 @@ static void collect_all_aliases(struct perf_evsel *counter,
alias->scale != counter->scale ||
alias->cgrp != counter->cgrp ||
strcmp(alias->unit, counter->unit) ||
- nsec_counter(alias) != nsec_counter(counter))
+ perf_evsel__is_clock(alias) != perf_evsel__is_clock(counter))
break;
alias->merged_stat = true;
cb(alias, data, false);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 94fce4f537e9..5285da0417c5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -260,6 +260,17 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
evsel->attr.sample_period = 1;
}

+ if (perf_evsel__is_clock(evsel)) {
+ /*
+ * The evsel->unit points to static alias->unit
+ * so it's ok to use static string in here.
+ */
+ static const char *unit = "msec";
+
+ evsel->unit = unit;
+ evsel->scale = 1e-6;
+ }
+
return evsel;
}

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 890babf9ce86..973c03167947 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -405,6 +405,12 @@ static inline bool perf_evsel__is_bpf_output(struct perf_evsel *evsel)
return perf_evsel__match(evsel, SOFTWARE, SW_BPF_OUTPUT);
}

+static inline bool perf_evsel__is_clock(struct perf_evsel *evsel)
+{
+ return perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK) ||
+ perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK);
+}
+
struct perf_attr_details {
bool freq;
bool verbose;
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 594d14a02b67..99990f5f2512 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -913,11 +913,10 @@ void perf_stat__print_shadow_stats(struct perf_evsel *evsel,
ratio = total / avg;

print_metric(ctxp, NULL, "%8.0f", "cycles / elision", ratio);
- } else if (perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK) ||
- perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK)) {
+ } else if (perf_evsel__is_clock(evsel)) {
if ((ratio = avg_stats(&walltime_nsecs_stats)) != 0)
print_metric(ctxp, NULL, "%8.3f", "CPUs utilized",
- avg / ratio);
+ avg / (ratio * evsel->scale));
else
print_metric(ctxp, NULL, NULL, "CPUs utilized", 0);
} else if (perf_stat_evsel__is(evsel, TOPDOWN_FETCH_BUBBLES)) {
--
2.17.1

2018-07-20 14:59:07

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> The warning message in check_w function uses wrongly
> the $file variable instead of $file1 and $file2.

Humm,

Before:

Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'

After:

Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'

The previous version is better, I can then just use:

diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h

and get what changed, with your change I have to go to tools/perf before
doing that diff, which is an unnecessary extra step in at least my
workflow.

- Arnaldo

> Fixes: 582472973593 ("perf check-headers.sh: Add support to check 2 independent files")
> Link: http://lkml.kernel.org/n/[email protected]
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/check-headers.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index 814aaf269949..73e723675c5f 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -67,7 +67,7 @@ check_2 () {
> cmd="diff $* $file1 $file2 > /dev/null"
>
> test -f $file2 &&
> - eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
> + eval $cmd || echo "Warning: Kernel ABI header at '$file1' differs from latest version at '$file2'" >&2
> }
>
> check () {
> --
> 2.17.1

2018-07-20 15:04:55

[permalink] [raw]

Subject: Re: [PATCH 4/4] perf tools: Move syscall_64.tbl check into check-headers.sh

Em Fri, Jul 20, 2018 at 01:00:36PM +0200, Jiri Olsa escreveu:
> Probably leftover from the time we introducd the check-headers.sh script.

So, with your patch:

[acme@jouet perf]$ git diff
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f0b1709a5ffb..cfc1b0949bb1 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
332 common statx __x64_sys_statx
333 common io_pgetevents __x64_sys_io_pgetevents
334 common rseq __x64_sys_rseq
+335 common krava __x64_sys_krava

#
# x32-specific system call numbers start at 512 to avoid cache impact
[acme@jouet perf]$

It doesn't warn about that new cool syscall, without your patch:

Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'

Can you check this?

- Arnaldo

> Link: http://lkml.kernel.org/n/[email protected]
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/arch/x86/Makefile | 3 ---
> tools/perf/check-headers.sh | 3 +++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
> index 1a38e78117ce..8cc6642fce7a 100644
> --- a/tools/perf/arch/x86/Makefile
> +++ b/tools/perf/arch/x86/Makefile
> @@ -19,9 +19,6 @@ systbl := $(sys)/syscalltbl.sh
> _dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')
>
> $(header): $(sys)/syscall_64.tbl $(systbl)
> - @(test -d ../../kernel -a -d ../../tools -a -d ../perf && ( \
> - (diff -B arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl >/dev/null) \
> - || echo "Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'" >&2 )) || true
> $(Q)$(SHELL) '$(systbl)' $(sys)/syscall_64.tbl 'x86_64' > $@
>
> clean::
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index 73e723675c5f..2c10e46d98e9 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -93,3 +93,6 @@ check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
> check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>"'
> check include/uapi/asm-generic/mman.h '-I "^#include <$uapi/$*asm-generic/mman-common.h>"'
> check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
> +
> +# diff non-symmetric files
> +check_2 arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl
> --
> 2.17.1

2018-07-20 15:15:17

[permalink] [raw]

Subject: Re: [PATCH 4/4] perf tools: Move syscall_64.tbl check into check-headers.sh

On Fri, Jul 20, 2018 at 12:03:54PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 20, 2018 at 01:00:36PM +0200, Jiri Olsa escreveu:
> > Probably leftover from the time we introducd the check-headers.sh script.
>
> So, with your patch:
>
> [acme@jouet perf]$ git diff
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..cfc1b0949bb1 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common krava __x64_sys_krava
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> [acme@jouet perf]$
>
>
> It doesn't warn about that new cool syscall, without your patch:
>
> Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'
>
> Can you check this?

hum, did you have the previous patch applied?
without it it displays misleading output

will check

jirka

2018-07-20 15:16:57

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > The warning message in check_w function uses wrongly
> > the $file variable instead of $file1 and $file2.
>
> Humm,
>
> Before:
>
> Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'
>
> After:
>
> Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'
>
>
> The previous version is better, I can then just use:
>
> diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h
>
> and get what changed, with your change I have to go to tools/perf before
> doing that diff, which is an unnecessary extra step in at least my
> workflow.

so all paths output based in kernel tree root then, will change

jirka

2018-07-20 15:25:16

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > The warning message in check_w function uses wrongly
> > > the $file variable instead of $file1 and $file2.
> >
> > Humm,
> >
> > Before:
> >
> > Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'
> >
> > After:
> >
> > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'
> >
> >
> > The previous version is better, I can then just use:
> >
> > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h
> >
> > and get what changed, with your change I have to go to tools/perf before
> > doing that diff, which is an unnecessary extra step in at least my
> > workflow.
>
> so all paths output based in kernel tree root then, will change
>
> jirka

I was going to ask about this in a separate email initially, but then
thought I'd use this email exchange instead, as my question is about
the code in question. Hope you don't mind.

If I'm reading this right, the intended behavoir of the block of code
below is to test file2 for existance, and if it exists, to evaluate $cmd.
If file1 and file2 are found to differ, print the warning.

test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
differs from latest version at '$file'" >&2

The '||' path of execution is however also taken if file2 doesn't exist,
which is probably very unlikely to happen. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

Is this something you would rather leave as is, or perhaps use something
along the lines of the code below instead:

test -f $file2 && {
eval $cmd ||
echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
version at '$file'" >&2
}

Thanks.

2018-07-23 07:03:30

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

On Fri, Jul 20, 2018 at 06:22:49PM +0300, Alexander Kapshuk wrote:
> On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa <[email protected]> wrote:
> >
> > On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > > The warning message in check_w function uses wrongly
> > > > the $file variable instead of $file1 and $file2.
> > >
> > > Humm,
> > >
> > > Before:
> > >
> > > Warning: Kernel ABI header at 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at 'arch/powerpc/include/uapi/asm/unistd.h'
> > >
> > > After:
> > >
> > > Warning: Kernel ABI header at '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at '../../arch/powerpc/include/uapi/asm/unistd.h'
> > >
> > >
> > > The previous version is better, I can then just use:
> > >
> > > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h arch/powerpc/include/uapi/asm/unistd.h
> > >
> > > and get what changed, with your change I have to go to tools/perf before
> > > doing that diff, which is an unnecessary extra step in at least my
> > > workflow.
> >
> > so all paths output based in kernel tree root then, will change
> >
> > jirka
>
> I was going to ask about this in a separate email initially, but then
> thought I'd use this email exchange instead, as my question is about
> the code in question. Hope you don't mind.
>
> If I'm reading this right, the intended behavoir of the block of code
> below is to test file2 for existance, and if it exists, to evaluate $cmd.
> If file1 and file2 are found to differ, print the warning.
>
> test -f $file2 &&
> eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
> differs from latest version at '$file'" >&2
>
> The '||' path of execution is however also taken if file2 doesn't exist,
> which is probably very unlikely to happen. See below.
>
> % file1=file1; file2=file2
> % cmd="echo diff $file1 $file2"
> % test -f $file2 &&
> eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> differs from latest version at '$file2'" >&2
> Warning: Kernel ABI header at 'tools/file1' differs from latest
> version at 'file2'
>
> Is this something you would rather leave as is, or perhaps use something
> along the lines of the code below instead:
>
> test -f $file2 && {
> eval $cmd ||
> echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
> version at '$file'" >&2
> }

hi,
yea, probably.. please feel free to post a patch.. just make sure all
the displayed files paths are based on the kernel root

thanks,
jirka

2018-07-24 07:21:31

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

On Mon, Jul 23, 2018, 08:01 Jiri Olsa <[email protected]> wrote:

> On Fri, Jul 20, 2018 at 06:22:49PM +0300, Alexander Kapshuk wrote:
> > On Fri, Jul 20, 2018 at 6:16 PM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Fri, Jul 20, 2018 at 11:57:45AM -0300, Arnaldo Carvalho de Melo
> wrote:
> > > > Em Fri, Jul 20, 2018 at 01:00:35PM +0200, Jiri Olsa escreveu:
> > > > > The warning message in check_w function uses wrongly
> > > > > the $file variable instead of $file1 and $file2.
> > > >
> > > > Humm,
> > > >
> > > > Before:
> > > >
> > > > Warning: Kernel ABI header at
> 'tools/arch/powerpc/include/uapi/asm/unistd.h' differs from latest version
> at 'arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > > After:
> > > >
> > > > Warning: Kernel ABI header at
> '../arch/powerpc/include/uapi/asm/unistd.h' differs from latest version at
> '../../arch/powerpc/include/uapi/asm/unistd.h'
> > > >
> > > >
> > > > The previous version is better, I can then just use:
> > > >
> > > > diff -u tools/arch/powerpc/include/uapi/asm/unistd.h
> arch/powerpc/include/uapi/asm/unistd.h
> > > >
> > > > and get what changed, with your change I have to go to tools/perf
> before
> > > > doing that diff, which is an unnecessary extra step in at least my
> > > > workflow.
> > >
> > > so all paths output based in kernel tree root then, will change
> > >
> > > jirka
> >
> > I was going to ask about this in a separate email initially, but then
> > thought I'd use this email exchange instead, as my question is about
> > the code in question. Hope you don't mind.
> >
> > If I'm reading this right, the intended behavoir of the block of code
> > below is to test file2 for existance, and if it exists, to evaluate $cmd.
> > If file1 and file2 are found to differ, print the warning.
> >
> > test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file'
> > differs from latest version at '$file'" >&2
> >
> > The '||' path of execution is however also taken if file2 doesn't exist,
> > which is probably very unlikely to happen. See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > Is this something you would rather leave as is, or perhaps use something
> > along the lines of the code below instead:
> >
> > test -f $file2 && {
> > eval $cmd ||
> > echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
> > version at '$file'" >&2
> > }
>
> hi,
> yea, probably.. please feel free to post a patch.. just make sure all
> the displayed files paths are based on the kernel root
>
> thanks,
> jirka
>

I'm away traveling till August 10th, and I may not be able to send the
patch in until I get back. Is that OK?
Thanks.

2018-07-25 20:54:38

[permalink] [raw]

Subject: [tip:perf/core] perf tools: Use perf_evsel__match instead of open coded equivalent

Commit-ID: 2d6cae13f10d5d8b370038dbfdf60317c22b9f52
Gitweb: https://git.kernel.org/tip/2d6cae13f10d5d8b370038dbfdf60317c22b9f52
Author: Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jul 2018 13:00:33 +0200
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 24 Jul 2018 14:54:13 -0300

perf tools: Use perf_evsel__match instead of open coded equivalent

Use perf_evsel__match() helper in perf_evsel__is_bpf_output().

Signed-off-by: Jiri Olsa <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/evsel.h | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d277930b19a1..890babf9ce86 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -402,10 +402,7 @@ bool perf_evsel__is_function_event(struct perf_evsel *evsel);

static inline bool perf_evsel__is_bpf_output(struct perf_evsel *evsel)
{
- struct perf_event_attr *attr = &evsel->attr;
-
- return (attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
- (attr->type == PERF_TYPE_SOFTWARE);
+ return perf_evsel__match(evsel, SOFTWARE, SW_BPF_OUTPUT);
}

struct perf_attr_details {

2018-07-25 20:55:27

[permalink] [raw]

Subject: [tip:perf/core] perf stat: Get rid of extra clock display function

Commit-ID: 0aa802a79469a86ebe143019144cd4df8ae852e4
Gitweb: https://git.kernel.org/tip/0aa802a79469a86ebe143019144cd4df8ae852e4
Author: Jiri Olsa <[email protected]>
AuthorDate: Fri, 20 Jul 2018 13:00:34 +0200
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 24 Jul 2018 14:54:58 -0300

perf stat: Get rid of extra clock display function

There's no reason to have separate function to display clock events.
It's only purpose was to convert the nanosecond value into microseconds.
We do that now in generic code, if the unit and scale values are
properly set, which this patch do for clock events.

The output differs in the unit field being displayed in its columns
rather than having it added as a suffix of the event name. Plus the
value is rounded into 2 decimal numbers as for any other event.

Before:

# perf stat -e cpu-clock,task-clock -C 0 sleep 3

Performance counter stats for 'CPU(s) 0':

3001.123137 cpu-clock (msec) # 1.000 CPUs utilized
3001.133250 task-clock (msec) # 1.000 CPUs utilized

3.001159813 seconds time elapsed

Now:

# perf stat -e cpu-clock,task-clock -C 0 sleep 3

Performance counter stats for 'CPU(s) 0':

3,001.05 msec cpu-clock # 1.000 CPUs utilized
3,001.05 msec task-clock # 1.000 CPUs utilized

3.001077794 seconds time elapsed

There's a small difference in csv output, as we now output the unit
field, which was empty before. It's in the proper spot, so there's no
compatibility issue.

Before:

# perf stat -e cpu-clock,task-clock -C 0 -x, sleep 3
3001.065177,,cpu-clock,3001064187,100.00,1.000,CPUs utilized
3001.077085,,task-clock,3001077085,100.00,1.000,CPUs utilized

# perf stat -e cpu-clock,task-clock -C 0 -x, sleep 3
3000.80,msec,cpu-clock,3000799026,100.00,1.000,CPUs utilized
3000.80,msec,task-clock,3000799550,100.00,1.000,CPUs utilized

Add perf_evsel__is_clock to replace nsec_counter.

Signed-off-by: Jiri Olsa <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-stat.c | 48 ++-----------------------------------------
tools/perf/util/evsel.c | 11 ++++++++++
tools/perf/util/evsel.h | 6 ++++++
tools/perf/util/stat-shadow.c | 5 ++---
4 files changed, 21 insertions(+), 49 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index dfd13d6e2931..d097b5b47eb8 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -296,18 +296,6 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
return perf_evsel__open_per_thread(evsel, evsel_list->threads);
}

-/*
- * Does the counter have nsecs as a unit?
- */
-static inline int nsec_counter(struct perf_evsel *evsel)
-{
- if (perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK) ||
- perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK))
- return 1;
-
- return 0;
-}
-
static int process_synthesized_event(struct perf_tool *tool __maybe_unused,
union perf_event *event,
struct perf_sample *sample __maybe_unused,
@@ -1058,34 +1046,6 @@ static void print_metric_header(void *ctx, const char *color __maybe_unused,
fprintf(os->fh, "%*s ", metric_only_len, unit);
}

-static void nsec_printout(int id, int nr, struct perf_evsel *evsel, double avg)
-{
- FILE *output = stat_config.output;
- double msecs = avg / NSEC_PER_MSEC;
- const char *fmt_v, *fmt_n;
- char name[25];
-
- fmt_v = csv_output ? "%.6f%s" : "%18.6f%s";
- fmt_n = csv_output ? "%s" : "%-25s";
-
- aggr_printout(evsel, id, nr);
-
- scnprintf(name, sizeof(name), "%s%s",
- perf_evsel__name(evsel), csv_output ? "" : " (msec)");
-
- fprintf(output, fmt_v, msecs, csv_sep);
-
- if (csv_output)
- fprintf(output, "%s%s", evsel->unit, csv_sep);
- else
- fprintf(output, "%-*s%s", unit_width, evsel->unit, csv_sep);
-
- fprintf(output, fmt_n, name);
-
- if (evsel->cgrp)
- fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
-}
-
static int first_shadow_cpu(struct perf_evsel *evsel, int id)
{
int i;
@@ -1241,11 +1201,7 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
return;
}

- if (metric_only)
- /* nothing */;
- else if (nsec_counter(counter))
- nsec_printout(id, nr, counter, uval);
- else
+ if (!metric_only)
abs_printout(id, nr, counter, uval);

out.print_metric = pm;
@@ -1331,7 +1287,7 @@ static void collect_all_aliases(struct perf_evsel *counter,
alias->scale != counter->scale ||
alias->cgrp != counter->cgrp ||
strcmp(alias->unit, counter->unit) ||
- nsec_counter(alias) != nsec_counter(counter))
+ perf_evsel__is_clock(alias) != perf_evsel__is_clock(counter))
break;
alias->merged_stat = true;
cb(alias, data, false);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 94fce4f537e9..5285da0417c5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -260,6 +260,17 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
evsel->attr.sample_period = 1;
}

+ if (perf_evsel__is_clock(evsel)) {
+ /*
+ * The evsel->unit points to static alias->unit
+ * so it's ok to use static string in here.
+ */
+ static const char *unit = "msec";
+
+ evsel->unit = unit;
+ evsel->scale = 1e-6;
+ }
+
return evsel;
}

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 890babf9ce86..973c03167947 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -405,6 +405,12 @@ static inline bool perf_evsel__is_bpf_output(struct perf_evsel *evsel)
return perf_evsel__match(evsel, SOFTWARE, SW_BPF_OUTPUT);
}

+static inline bool perf_evsel__is_clock(struct perf_evsel *evsel)
+{
+ return perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK) ||
+ perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK);
+}
+
struct perf_attr_details {
bool freq;
bool verbose;
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 594d14a02b67..99990f5f2512 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -913,11 +913,10 @@ void perf_stat__print_shadow_stats(struct perf_evsel *evsel,
ratio = total / avg;

print_metric(ctxp, NULL, "%8.0f", "cycles / elision", ratio);
- } else if (perf_evsel__match(evsel, SOFTWARE, SW_TASK_CLOCK) ||
- perf_evsel__match(evsel, SOFTWARE, SW_CPU_CLOCK)) {
+ } else if (perf_evsel__is_clock(evsel)) {
if ((ratio = avg_stats(&walltime_nsecs_stats)) != 0)
print_metric(ctxp, NULL, "%8.3f", "CPUs utilized",
- avg / ratio);
+ avg / (ratio * evsel->scale));
else
print_metric(ctxp, NULL, NULL, "CPUs utilized", 0);
} else if (perf_stat_evsel__is(evsel, TOPDOWN_FETCH_BUBBLES)) {

2018-07-26 06:23:46

[permalink] [raw]

Subject: Re: [PATCH 3/4] perf tools: Fix check-headers.sh output file variables

On Tue, Jul 24, 2018 at 08:20:07AM +0100, Alexander Kapshuk wrote:

SNIP

> > > % file1=file1; file2=file2
> > > % cmd="echo diff $file1 $file2"
> > > % test -f $file2 &&
> > > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > > differs from latest version at '$file2'" >&2
> > > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > > version at 'file2'
> > >
> > > Is this something you would rather leave as is, or perhaps use something
> > > along the lines of the code below instead:
> > >
> > > test -f $file2 && {
> > > eval $cmd ||
> > > echo "Warning: Kernel ABI header at 'tools/$file' differs from latest
> > > version at '$file'" >&2
> > > }
> >
> > hi,
> > yea, probably.. please feel free to post a patch.. just make sure all
> > the displayed files paths are based on the kernel root
> >
> > thanks,
> > jirka
> >
>
> I'm away traveling till August 10th, and I may not be able to send the
> patch in until I get back. Is that OK?

sure, thanks

jirka

2018-08-11 08:42:24

[permalink] [raw]

Subject: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 &&
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if
file2 does exist. Should the files being compared differ, a diff command
to compare the files concerned is printed on standard output. E.g.

diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Signed-off-by: Alexander Kapshuk <[email protected]>
---
tools/perf/check-headers.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {

cmd="diff $* $file1 $file2 > /dev/null"

- test -f $file2 &&
- eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+ test -f $file2 && {
+ eval $cmd || {
+ echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+ echo diff -u tools/$file $file
+ }
+ }
}

check () {
--
2.18.0

2018-08-13 11:23:55

[permalink] [raw]

Subject: Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

On Sat, Aug 11, 2018 at 11:39:15AM +0300, Alexander Kapshuk wrote:
> The '||' path of execution in the 'test' block of the check_2() function
> may also be taken if file2 does not exist, in which case the warning
> message about the ABI headers being different would still be printed
> where it should not be. See below.
>
> % file1=file1; file2=file2
> % cmd="echo diff $file1 $file2"
> % test -f $file2 &&
> eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> differs from latest version at '$file2'" >&2
> Warning: Kernel ABI header at 'tools/file1' differs from latest
> version at 'file2'
>
> The proposed patch converts the code following the '&&' operator into a
> compound list to be executed in the current process environment only if
> file2 does exist. Should the files being compared differ, a diff command
> to compare the files concerned is printed on standard output. E.g.
>
> diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
>
> Signed-off-by: Alexander Kapshuk <[email protected]>

I posted follow up patches based on this one
as a reply on this patch

for this one:

Acked-by: Jiri Olsa <[email protected]>

thanks,
jirka

> ---
> tools/perf/check-headers.sh | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index de28466c0186..ea48aa6f8d19 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -67,8 +67,12 @@ check_2 () {
>
> cmd="diff $* $file1 $file2 > /dev/null"
>
> - test -f $file2 &&
> - eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
> + test -f $file2 && {
> + eval $cmd || {
> + echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
> + echo diff -u tools/$file $file
> + }
> + }
> }
>
> check () {
> --
> 2.18.0
>

2018-08-13 11:23:55

[permalink] [raw]

Subject: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Changing the logic to compare files with paths relative
to kernel source base dir. This way we can keep the output
message for 2 unrelated files, which is coming in following
patch.

Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/check-headers.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index ea48aa6f8d19..9d466e853aec 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -69,8 +69,8 @@ check_2 () {

test -f $file2 && {
eval $cmd || {
- echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
- echo diff -u tools/$file $file
+ echo "Warning: Kernel ABI header at '$file1' differs from latest version at '$file2'" >&2
+ echo diff -u $file1 $file2
}
}
}
@@ -80,7 +80,7 @@ check () {

shift

- check_2 ../$file ../../$file $*
+ check_2 tools/$file $file $*
}

# Check if we have the kernel headers (tools/perf/../../include), else
@@ -88,6 +88,8 @@ check () {
# differences.
test -d ../../include || exit 0

+pushd ../.. > /dev/null
+
# simple diff check
for i in $HEADERS; do
check $i -B
@@ -98,3 +100,5 @@ check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>"'
check include/uapi/asm-generic/mman.h '-I "^#include <$uapi/$*asm-generic/mman-common.h>"'
check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
+
+popd > /dev/null
--
2.17.1

2018-08-13 11:23:56

[permalink] [raw]

Subject: Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

On Mon, Aug 13, 2018 at 2:16 PM Jiri Olsa <[email protected]> wrote:
>
> On Sat, Aug 11, 2018 at 11:39:15AM +0300, Alexander Kapshuk wrote:
> > The '||' path of execution in the 'test' block of the check_2() function
> > may also be taken if file2 does not exist, in which case the warning
> > message about the ABI headers being different would still be printed
> > where it should not be. See below.
> >
> > % file1=file1; file2=file2
> > % cmd="echo diff $file1 $file2"
> > % test -f $file2 &&
> > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > differs from latest version at '$file2'" >&2
> > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > version at 'file2'
> >
> > The proposed patch converts the code following the '&&' operator into a
> > compound list to be executed in the current process environment only if
> > file2 does exist. Should the files being compared differ, a diff command
> > to compare the files concerned is printed on standard output. E.g.
> >
> > diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
> >
> > Signed-off-by: Alexander Kapshuk <[email protected]>
>
> I posted follow up patches based on this one
> as a reply on this patch
>
> for this one:
>
> Acked-by: Jiri Olsa <[email protected]>
>
> thanks,
> jirka

Noted.
Thanks.

>
> > ---
> > tools/perf/check-headers.sh | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index de28466c0186..ea48aa6f8d19 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -67,8 +67,12 @@ check_2 () {
> >
> > cmd="diff $* $file1 $file2 > /dev/null"
> >
> > - test -f $file2 &&
> > - eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
> > + test -f $file2 && {
> > + eval $cmd || {
> > + echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
> > + echo diff -u tools/$file $file
> > + }
> > + }
> > }
> >
> > check () {
> > --
> > 2.18.0
> >

2018-08-13 11:44:12

[permalink] [raw]

Subject: [PATCH 2/2] perf tools: Move syscall_64.tbl check into check-headers.sh

Probably leftover from the time we introducd the check-headers.sh script.

Link: http://lkml.kernel.org/n/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/arch/x86/Makefile | 3 ---
tools/perf/check-headers.sh | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
index 1a38e78117ce..8cc6642fce7a 100644
--- a/tools/perf/arch/x86/Makefile
+++ b/tools/perf/arch/x86/Makefile
@@ -19,9 +19,6 @@ systbl := $(sys)/syscalltbl.sh
_dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')

$(header): $(sys)/syscall_64.tbl $(systbl)
- @(test -d ../../kernel -a -d ../../tools -a -d ../perf && ( \
- (diff -B arch/x86/entry/syscalls/syscall_64.tbl ../../arch/x86/entry/syscalls/syscall_64.tbl >/dev/null) \
- || echo "Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'" >&2 )) || true
$(Q)$(SHELL) '$(systbl)' $(sys)/syscall_64.tbl 'x86_64' > $@

clean::
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 9d466e853aec..80bf84803677 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -101,4 +101,7 @@ check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
check include/uapi/asm-generic/mman.h '-I "^#include <$uapi/$*asm-generic/mman-common.h>"'
check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'

+# diff non-symmetric files
+check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
+
popd > /dev/null
--
2.17.1

2018-08-13 19:18:20

[permalink] [raw]

Subject: Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

Em Mon, Aug 13, 2018 at 02:19:10PM +0300, Alexander Kapshuk escreveu:
> On Mon, Aug 13, 2018 at 2:16 PM Jiri Olsa <[email protected]> wrote:
> >
> > On Sat, Aug 11, 2018 at 11:39:15AM +0300, Alexander Kapshuk wrote:
> > > The '||' path of execution in the 'test' block of the check_2() function
> > > may also be taken if file2 does not exist, in which case the warning
> > > message about the ABI headers being different would still be printed
> > > where it should not be. See below.
> > >
> > > % file1=file1; file2=file2
> > > % cmd="echo diff $file1 $file2"
> > > % test -f $file2 &&
> > > eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
> > > differs from latest version at '$file2'" >&2
> > > Warning: Kernel ABI header at 'tools/file1' differs from latest
> > > version at 'file2'
> > >
> > > The proposed patch converts the code following the '&&' operator into a
> > > compound list to be executed in the current process environment only if
> > > file2 does exist. Should the files being compared differ, a diff command
> > > to compare the files concerned is printed on standard output. E.g.
> > >
> > > diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
> > >
> > > Signed-off-by: Alexander Kapshuk <[email protected]>
> >
> > I posted follow up patches based on this one
> > as a reply on this patch
> >
> > for this one:
> >
> > Acked-by: Jiri Olsa <[email protected]>
> >
> > thanks,
> > jirka
>
> Noted.
> Thanks.

Thanks, applied the three patches to acme/perf/core.

- Arnaldo

2018-08-14 01:55:48

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Jiri Olsa <[email protected]> writes:
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index ea48aa6f8d19..9d466e853aec 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -88,6 +88,8 @@ check () {
> # differences.
> test -d ../../include || exit 0
>
> +pushd ../.. > /dev/null
> +
> # simple diff check
> for i in $HEADERS; do
> check $i -B

This breaks the build when sh is not bash:

./check-headers.sh: 91: ./check-headers.sh: pushd: not found
./check-headers.sh: 107: ./check-headers.sh: popd: not found
Makefile.perf:205: recipe for target 'sub-make' failed

cheers

2018-08-14 06:14:40

[permalink] [raw]

Subject: Re: [PATCH] perf tools: Fix check-headers.sh AND list path of execution

On Mon, Aug 13, 2018 at 9:58 PM Arnaldo Carvalho de Melo
<[email protected]> wrote:
> Thanks, applied the three patches to acme/perf/core.
>
> - Arnaldo

Thanks.

2018-08-14 07:34:01

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

On Tue, Aug 14, 2018 at 11:47:39AM +1000, Michael Ellerman wrote:
> Jiri Olsa <[email protected]> writes:
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index ea48aa6f8d19..9d466e853aec 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -88,6 +88,8 @@ check () {
> > # differences.
> > test -d ../../include || exit 0
> >
> > +pushd ../.. > /dev/null
> > +
> > # simple diff check
> > for i in $HEADERS; do
> > check $i -B
>
> This breaks the build when sh is not bash:
>
> ./check-headers.sh: 91: ./check-headers.sh: pushd: not found
> ./check-headers.sh: 107: ./check-headers.sh: popd: not found
> Makefile.perf:205: recipe for target 'sub-make' failed

sry.. Arnaldo, would you change it for simple cd (attached below)
or should I send the fix?

thanks,
jirka

---
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 80bf84803677..466540ee8ea7 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -88,7 +88,7 @@ check () {
# differences.
test -d ../../include || exit 0

-pushd ../.. > /dev/null
+cd ../..

# simple diff check
for i in $HEADERS; do
@@ -104,4 +104,4 @@ check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
# diff non-symmetric files
check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl

-popd > /dev/null
+cd tools/perf

2018-08-14 18:10:08

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:
> On Tue, Aug 14, 2018 at 11:47:39AM +1000, Michael Ellerman wrote:
> > Jiri Olsa <[email protected]> writes:
> > > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > > index ea48aa6f8d19..9d466e853aec 100755
> > > --- a/tools/perf/check-headers.sh
> > > +++ b/tools/perf/check-headers.sh
> > > @@ -88,6 +88,8 @@ check () {
> > > # differences.
> > > test -d ../../include || exit 0
> > >
> > > +pushd ../.. > /dev/null
> > > +
> > > # simple diff check
> > > for i in $HEADERS; do
> > > check $i -B
> >
> > This breaks the build when sh is not bash:
> >
> > ./check-headers.sh: 91: ./check-headers.sh: pushd: not found
> > ./check-headers.sh: 107: ./check-headers.sh: popd: not found
> > Makefile.perf:205: recipe for target 'sub-make' failed
>
> sry.. Arnaldo, would you change it for simple cd (attached below)
> or should I send the fix?

Nah, I'm folding this in, to keep it bisectable.

> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index 80bf84803677..466540ee8ea7 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -88,7 +88,7 @@ check () {
> # differences.
> test -d ../../include || exit 0
>
> -pushd ../.. > /dev/null
> +cd ../..
>
> # simple diff check
> for i in $HEADERS; do
> @@ -104,4 +104,4 @@ check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
> # diff non-symmetric files
> check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
>
> -popd > /dev/null
> +cd tools/perf

2018-08-14 23:12:45

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

On Tue, Aug 14, 2018 at 03:06:44PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:
> > On Tue, Aug 14, 2018 at 11:47:39AM +1000, Michael Ellerman wrote:
> > > Jiri Olsa <[email protected]> writes:
> > > > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > > > index ea48aa6f8d19..9d466e853aec 100755
> > > > --- a/tools/perf/check-headers.sh
> > > > +++ b/tools/perf/check-headers.sh
> > > > @@ -88,6 +88,8 @@ check () {
> > > > # differences.
> > > > test -d ../../include || exit 0
> > > >
> > > > +pushd ../.. > /dev/null
> > > > +
> > > > # simple diff check
> > > > for i in $HEADERS; do
> > > > check $i -B
> > >
> > > This breaks the build when sh is not bash:
> > >
> > > ./check-headers.sh: 91: ./check-headers.sh: pushd: not found
> > > ./check-headers.sh: 107: ./check-headers.sh: popd: not found
> > > Makefile.perf:205: recipe for target 'sub-make' failed
> >
> > sry.. Arnaldo, would you change it for simple cd (attached below)
> > or should I send the fix?
>
> Nah, I'm folding this in, to keep it bisectable.

any chance one of your docker tests could run build in sh/zsh? ;-)

thanks,
jirka

2018-08-15 10:03:59

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Jiri Olsa <[email protected]> writes:
> On Tue, Aug 14, 2018 at 03:06:44PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:
>> > On Tue, Aug 14, 2018 at 11:47:39AM +1000, Michael Ellerman wrote:
>> > > Jiri Olsa <[email protected]> writes:
>> > > > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
>> > > > index ea48aa6f8d19..9d466e853aec 100755
>> > > > --- a/tools/perf/check-headers.sh
>> > > > +++ b/tools/perf/check-headers.sh
>> > > > @@ -88,6 +88,8 @@ check () {
>> > > > # differences.
>> > > > test -d ../../include || exit 0
>> > > >
>> > > > +pushd ../.. > /dev/null
>> > > > +
>> > > > # simple diff check
>> > > > for i in $HEADERS; do
>> > > > check $i -B
>> > >
>> > > This breaks the build when sh is not bash:
>> > >
>> > > ./check-headers.sh: 91: ./check-headers.sh: pushd: not found
>> > > ./check-headers.sh: 107: ./check-headers.sh: popd: not found
>> > > Makefile.perf:205: recipe for target 'sub-make' failed
>> >
>> > sry.. Arnaldo, would you change it for simple cd (attached below)
>> > or should I send the fix?
>>
>> Nah, I'm folding this in, to keep it bisectable.
>
> any chance one of your docker tests could run build in sh/zsh? ;-)

Just using an Ubuntu image, where /bin/sh == dash should work, that's
how I hit it.

cheers

2018-08-15 15:05:42

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Em Wed, Aug 15, 2018 at 08:02:48PM +1000, Michael Ellerman escreveu:
> Jiri Olsa <[email protected]> writes:
> > On Tue, Aug 14, 2018 at 03:06:44PM -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:
> >> > sry.. Arnaldo, would you change it for simple cd (attached below)
> >> > or should I send the fix?

> >> Nah, I'm folding this in, to keep it bisectable.

> > any chance one of your docker tests could run build in sh/zsh? ;-)

It does already, see below :-)

> Just using an Ubuntu image, where /bin/sh == dash should work, that's
> how I hit it.

So, I do the tests only prior to pushing to Ingo, so didn't catch this,
lemme check, put that change back on, start a ubuntu:18.04 perf build
container, try to build, see if it would fail, yeah, I'd have detected
this before pushing to Ingo, so probably I have to run the tests before
pushing to my acme/perf/core branch, will try to operate like that from
now on.

[root@jouet perf]# git diff
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 466540ee8ea7..80bf84803677 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -88,7 +88,7 @@ check () {
# differences.
test -d ../../include || exit 0

-cd ../..
+pushd ../.. > /dev/null

# simple diff check
for i in $HEADERS; do
@@ -104,4 +104,4 @@ check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
# diff non-symmetric files
check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl

-cd tools/perf
+popd > /dev/null
[root@jouet perf]# docker run --privileged --entrypoint=/bin/sh -v /home/acme/git:/git:Z --rm -ti docker.io/acmel/linux-perf-tools-build-ubuntu:18.04
$ bash
perfbuilder@65ead4a4734a:/$ cd /git/perf
perfbuilder@65ead4a4734a:/git/perf$ make -C tools/perf O=/tmp/build/perf/ install-bin
make: Entering directory '/git/perf/tools/perf'
BUILD: Doing 'make -j4' parallel build
HOSTCC /tmp/build/perf/fixdep.o
HOSTLD /tmp/build/perf/fixdep-in.o
LINK /tmp/build/perf/fixdep
./check-headers.sh: 91: ./check-headers.sh: pushd: not found
diff: tools/perf/arch/x86/entry/syscalls/syscall_64.tbl: No such file or directory
Warning: Kernel ABI header at 'tools/perf/arch/x86/entry/syscalls/syscall_64.tbl' differs from latest version at 'arch/x86/entry/syscalls/syscall_64.tbl'
diff -u tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
./check-headers.sh: 107: ./check-headers.sh: popd: not found
Makefile.perf:205: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 127
Makefile:109: recipe for target 'install-bin' failed
make: *** [install-bin] Error 2
make: Leaving directory '/git/perf/tools/perf'
perfbuilder@65ead4a4734a:/git/perf$

2018-08-16 05:05:52

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 1/2] perf tools: Make check-headers.sh check based on kernel dir

Arnaldo Carvalho de Melo <[email protected]> writes:
> Em Wed, Aug 15, 2018 at 08:02:48PM +1000, Michael Ellerman escreveu:
>> Jiri Olsa <[email protected]> writes:
>> > On Tue, Aug 14, 2018 at 03:06:44PM -0300, Arnaldo Carvalho de Melo wrote:
>> >> Em Tue, Aug 14, 2018 at 09:27:26AM +0200, Jiri Olsa escreveu:
>> >> > sry.. Arnaldo, would you change it for simple cd (attached below)
>> >> > or should I send the fix?
>
>> >> Nah, I'm folding this in, to keep it bisectable.
>
>> > any chance one of your docker tests could run build in sh/zsh? ;-)
>
> It does already, see below :-)
>
>> Just using an Ubuntu image, where /bin/sh == dash should work, that's
>> how I hit it.
>
> So, I do the tests only prior to pushing to Ingo, so didn't catch this,
> lemme check, put that change back on, start a ubuntu:18.04 perf build
> container, try to build, see if it would fail, yeah, I'd have detected
> this before pushing to Ingo, so probably I have to run the tests before
> pushing to my acme/perf/core branch, will try to operate like that from
> now on.

Or you can tell me to test a different branch :)

cheers

2018-08-18 11:58:00

[permalink] [raw]

Subject: [tip:perf/urgent] perf tools: Fix check-headers.sh AND list path of execution

Commit-ID: 51d8aac236493833acf60d9c3b6c42437a18da36
Gitweb: https://git.kernel.org/tip/51d8aac236493833acf60d9c3b6c42437a18da36
Author: Alexander Kapshuk <[email protected]>
AuthorDate: Sat, 11 Aug 2018 11:39:15 +0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 13 Aug 2018 15:46:19 -0300

perf tools: Fix check-headers.sh AND list path of execution

The '||' path of execution in the 'test' block of the check_2() function
may also be taken if file2 does not exist, in which case the warning
message about the ABI headers being different would still be printed
where it should not be. See below.

% file1=file1; file2=file2
% cmd="echo diff $file1 $file2"
% test -f $file2 && \
eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file1'
differs from latest version at '$file2'" >&2
Warning: Kernel ABI header at 'tools/file1' differs from latest
version at 'file2'

The proposed patch converts the code following the '&&' operator into a
compound list to be executed in the current process environment only if file2
does exist. Should the files being compared differ, a diff command to compare
the files concerned is printed on standard output. E.g.

$ diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S

Committer testing:

Remove a line from that tools/arch/x86/lib/memcpy_64.S file to test
this:

BUILD: Doing 'make -j4' parallel build
Warning: Kernel ABI header at 'tools/arch/x86/lib/memcpy_64.S' differs from latest version at 'arch/x86/lib/memcpy_64.S'
diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
CC /tmp/build/perf/bench/mem-memcpy-x86-64-asm.o

Signed-off-by: Alexander Kapshuk <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/check-headers.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index de28466c0186..ea48aa6f8d19 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -67,8 +67,12 @@ check_2 () {

cmd="diff $* $file1 $file2 > /dev/null"

- test -f $file2 &&
- eval $cmd || echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+ test -f $file2 && {
+ eval $cmd || {
+ echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
+ echo diff -u tools/$file $file
+ }
+ }
}

check () {

2018-08-18 11:58:32

[permalink] [raw]

Subject: [tip:perf/urgent] perf tools: Make check-headers.sh check based on kernel dir

Commit-ID: 7ea6e983b2cce9a4277ea602c2976d18bd75a908
Gitweb: https://git.kernel.org/tip/7ea6e983b2cce9a4277ea602c2976d18bd75a908
Author: Jiri Olsa <[email protected]>
AuthorDate: Mon, 13 Aug 2018 13:15:03 +0200
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 14 Aug 2018 15:08:33 -0300

perf tools: Make check-headers.sh check based on kernel dir

Changing the logic to compare files with paths relative to kernel source
base dir. This way we can keep the output message for 2 unrelated files,
which is coming in following patch.

Committer testing:

Remove a line from tools/arch/x86/lib/memcpy_64.S to have it detected:

make: Entering directory '/home/acme/git/perf/tools/perf'
BUILD: Doing 'make -j4' parallel build
Warning: Kernel ABI header at 'tools/arch/x86/lib/memcpy_64.S' differs from latest version at 'arch/x86/lib/memcpy_64.S'
diff -u tools/arch/x86/lib/memcpy_64.S arch/x86/lib/memcpy_64.S
INSTALL GTK UI
INSTALL binaries

Signed-off-by: Jiri Olsa <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexander Kapshuk <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Link: http://lkml.kernel.org/r/20180814072726.GA13931@krava
[ Do not use pushd/popd, its a bashism, reported by Michael Ellerman, fixed by Jiri Olsa ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/check-headers.sh | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index ea48aa6f8d19..b61f8a4dfca3 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -69,8 +69,8 @@ check_2 () {

test -f $file2 && {
eval $cmd || {
- echo "Warning: Kernel ABI header at 'tools/$file' differs from latest version at '$file'" >&2
- echo diff -u tools/$file $file
+ echo "Warning: Kernel ABI header at '$file1' differs from latest version at '$file2'" >&2
+ echo diff -u $file1 $file2
}
}
}
@@ -80,7 +80,7 @@ check () {

shift

- check_2 ../$file ../../$file $*
+ check_2 tools/$file $file $*
}

# Check if we have the kernel headers (tools/perf/../../include), else
@@ -88,6 +88,8 @@ check () {
# differences.
test -d ../../include || exit 0

+cd ../..
+
# simple diff check
for i in $HEADERS; do
check $i -B
@@ -98,3 +100,5 @@ check arch/x86/lib/memcpy_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/ex
check arch/x86/lib/memset_64.S '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>"'
check include/uapi/asm-generic/mman.h '-I "^#include <$uapi/$*asm-generic/mman-common.h>"'
check include/uapi/linux/mman.h '-I "^#include <$uapi/$*asm/mman.h>"'
+
+cd tools/perf

2018-08-18 11:59:38