Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1248755 > unrolled thread

[PATCH 0/1] perf/x86: Fix time_shift in perf_event_mmap_page

Started byAdrian Hunter <adrian.hunter@intel.com>
First post2015-10-16 15:30 +0200
Last post2015-10-20 11:40 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/1] perf/x86: Fix time_shift in perf_event_mmap_page Adrian Hunter <adrian.hunter@intel.com> - 2015-10-16 15:30 +0200
    [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page Adrian Hunter <adrian.hunter@intel.com> - 2015-10-16 15:30 +0200
      Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page Ingo Molnar <mingo@kernel.org> - 2015-10-19 10:10 +0200
        Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page Adrian Hunter <adrian.hunter@intel.com> - 2015-10-19 10:50 +0200
          Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page Ingo Molnar <mingo@kernel.org> - 2015-10-19 13:30 +0200
            [PATCH V2 1/1] perf/x86: Fix time_shift in perf_event_mmap_page Adrian Hunter <adrian.hunter@intel.com> - 2015-10-19 14:10 +0200
      [tip:perf/core] perf/x86: Fix time_shift in perf_event_mmap_page tip-bot for Adrian Hunter <tipbot@zytor.com> - 2015-10-20 11:40 +0200

#1248755 — [PATCH 0/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-10-16 15:30 +0200
Subject[PATCH 0/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qkdf5-7zE-31@gated-at.bofh.it>
Hi

There is a problem with my patch:
commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
which is in tip at the moment.

The problem is that userspace does not expect time_shift in perf_event_mmap_page
to be as much as 32.  Here is a proposed fix.


Adrian Hunter (1):
      perf/x86: Fix time_shift in perf_event_mmap_page

 arch/x86/kernel/tsc.c           | 11 +++++++++++
 include/uapi/linux/perf_event.h |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)


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

[toc] | [next] | [standalone]

#1248761 — [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-10-16 15:30 +0200
Subject[PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qkdf5-7zE-45@gated-at.bofh.it>
In reply to#1248755
Commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
allowed the time_shift value in perf_event_mmap_page to be as much
as 32.  Unfortunately the documented algorithms for using time_shift
have it shifting an integer, whereas to work correctly with the value
32, the type must be u64.

Fix by limiting the shift to 31 and adjusting the multiplier accordingly.

Also update the documentation of perf_event_mmap_page so that new code
based on it will be more future-proof.

Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 arch/x86/kernel/tsc.c           | 11 +++++++++++
 include/uapi/linux/perf_event.h |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 69b84a26ea17..c7c4d9c51e99 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -259,6 +259,17 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
 	clocks_calc_mult_shift(&data->cyc2ns_mul, &data->cyc2ns_shift, cpu_khz,
 			       NSEC_PER_MSEC, 0);
 
+	/*
+	 * cyc2ns_shift is exported via arch_perf_update_userpage() where it is
+	 * not expected to be greater than 31 due to the original published
+	 * conversion algorithm shifting a 32-bit value (now specifies a 64-bit
+	 * value) - refer perf_event_mmap_page documentation in perf_event.h.
+	 */
+	if (data->cyc2ns_shift == 32) {
+		data->cyc2ns_shift = 31;
+		data->cyc2ns_mul >>= 1;
+	}
+
 	data->cyc2ns_offset = ns_now -
 		mul_u64_u32_shr(tsc_now, data->cyc2ns_mul, data->cyc2ns_shift);
 
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 2881145cda86..6c72e72e975c 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -476,7 +476,7 @@ struct perf_event_mmap_page {
 	 *   u64 delta;
 	 *
 	 *   quot = (cyc >> time_shift);
-	 *   rem = cyc & ((1 << time_shift) - 1);
+	 *   rem = cyc & (((u64)1 << time_shift) - 1);
 	 *   delta = time_offset + quot * time_mult +
 	 *              ((rem * time_mult) >> time_shift);
 	 *
@@ -507,7 +507,7 @@ struct perf_event_mmap_page {
 	 * And vice versa:
 	 *
 	 *   quot = cyc >> time_shift;
-	 *   rem  = cyc & ((1 << time_shift) - 1);
+	 *   rem  = cyc & (((u64)1 << time_shift) - 1);
 	 *   timestamp = time_zero + quot * time_mult +
 	 *               ((rem * time_mult) >> time_shift);
 	 */
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1250424 — Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromIngo Molnar <mingo@kernel.org>
Date2015-10-19 10:10 +0200
SubjectRe: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qldG1-6ST-21@gated-at.bofh.it>
In reply to#1248761
* Adrian Hunter <adrian.hunter@intel.com> wrote:

> Commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
> allowed the time_shift value in perf_event_mmap_page to be as much
> as 32.  Unfortunately the documented algorithms for using time_shift
> have it shifting an integer, whereas to work correctly with the value
> 32, the type must be u64.
> 
> Fix by limiting the shift to 31 and adjusting the multiplier accordingly.
> 
> Also update the documentation of perf_event_mmap_page so that new code
> based on it will be more future-proof.
> 
> Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Would be nice to point out via what symptoms the code misbehaves and how users 
notice.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1250461 — Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-10-19 10:50 +0200
SubjectRe: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qleiK-7D6-13@gated-at.bofh.it>
In reply to#1250424
On 19/10/15 11:08, Ingo Molnar wrote:
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
>> Commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
>> allowed the time_shift value in perf_event_mmap_page to be as much
>> as 32.  Unfortunately the documented algorithms for using time_shift
>> have it shifting an integer, whereas to work correctly with the value
>> 32, the type must be u64.
>>
>> Fix by limiting the shift to 31 and adjusting the multiplier accordingly.
>>
>> Also update the documentation of perf_event_mmap_page so that new code
>> based on it will be more future-proof.
>>
>> Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Would be nice to point out via what symptoms the code misbehaves and how users 
> notice.

In the case of perf tools, Intel PT decodes correctly but the timestamps
that are output (for example by perf script) have lost 32-bits of
granularity so they look like they are not changing at all.

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

[toc] | [prev] | [next] | [standalone]

#1250577 — Re: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromIngo Molnar <mingo@kernel.org>
Date2015-10-19 13:30 +0200
SubjectRe: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qlgNA-2V1-19@gated-at.bofh.it>
In reply to#1250461
* Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 19/10/15 11:08, Ingo Molnar wrote:
> > 
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> >> Commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
> >> allowed the time_shift value in perf_event_mmap_page to be as much
> >> as 32.  Unfortunately the documented algorithms for using time_shift
> >> have it shifting an integer, whereas to work correctly with the value
> >> 32, the type must be u64.
> >>
> >> Fix by limiting the shift to 31 and adjusting the multiplier accordingly.
> >>
> >> Also update the documentation of perf_event_mmap_page so that new code
> >> based on it will be more future-proof.
> >>
> >> Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > 
> > Would be nice to point out via what symptoms the code misbehaves and how users 
> > notice.
> 
> In the case of perf tools, Intel PT decodes correctly but the timestamps
> that are output (for example by perf script) have lost 32-bits of
> granularity so they look like they are not changing at all.

Sounds like a nice and very informative paragraph to add to the changelog! :-)

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1250596 — [PATCH V2 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

FromAdrian Hunter <adrian.hunter@intel.com>
Date2015-10-19 14:10 +0200
Subject[PATCH V2 1/1] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qlhqi-3Ur-5@gated-at.bofh.it>
In reply to#1250577
From 96bb79ce53b9373f5b6a212fcc2cd4d364d708af Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Fri, 16 Oct 2015 11:41:56 +0300
Subject: [PATCH 1/1] perf/x86: Fix time_shift in perf_event_mmap_page

Commit b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
allowed the time_shift value in perf_event_mmap_page to be as much
as 32.  Unfortunately the documented algorithms for using time_shift
have it shifting an integer, whereas to work correctly with the value
32, the type must be u64.

In the case of perf tools, Intel PT decodes correctly but the timestamps
that are output (for example by perf script) have lost 32-bits of
granularity so they look like they are not changing at all.

Fix by limiting the shift to 31 and adjusting the multiplier accordingly.

Also update the documentation of perf_event_mmap_page so that new code
based on it will be more future-proof.

Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:

	Add to the commit message symptoms of the code misbehaviour
	and how users notice.


 arch/x86/kernel/tsc.c           | 11 +++++++++++
 include/uapi/linux/perf_event.h |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 69b84a26ea17..c7c4d9c51e99 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -259,6 +259,17 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
 	clocks_calc_mult_shift(&data->cyc2ns_mul, &data->cyc2ns_shift, cpu_khz,
 			       NSEC_PER_MSEC, 0);
 
+	/*
+	 * cyc2ns_shift is exported via arch_perf_update_userpage() where it is
+	 * not expected to be greater than 31 due to the original published
+	 * conversion algorithm shifting a 32-bit value (now specifies a 64-bit
+	 * value) - refer perf_event_mmap_page documentation in perf_event.h.
+	 */
+	if (data->cyc2ns_shift == 32) {
+		data->cyc2ns_shift = 31;
+		data->cyc2ns_mul >>= 1;
+	}
+
 	data->cyc2ns_offset = ns_now -
 		mul_u64_u32_shr(tsc_now, data->cyc2ns_mul, data->cyc2ns_shift);
 
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 2881145cda86..6c72e72e975c 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -476,7 +476,7 @@ struct perf_event_mmap_page {
 	 *   u64 delta;
 	 *
 	 *   quot = (cyc >> time_shift);
-	 *   rem = cyc & ((1 << time_shift) - 1);
+	 *   rem = cyc & (((u64)1 << time_shift) - 1);
 	 *   delta = time_offset + quot * time_mult +
 	 *              ((rem * time_mult) >> time_shift);
 	 *
@@ -507,7 +507,7 @@ struct perf_event_mmap_page {
 	 * And vice versa:
 	 *
 	 *   quot = cyc >> time_shift;
-	 *   rem  = cyc & ((1 << time_shift) - 1);
+	 *   rem  = cyc & (((u64)1 << time_shift) - 1);
 	 *   timestamp = time_zero + quot * time_mult +
 	 *               ((rem * time_mult) >> time_shift);
 	 */
-- 
1.9.1


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

[toc] | [prev] | [next] | [standalone]

#1251531 — [tip:perf/core] perf/x86: Fix time_shift in perf_event_mmap_page

Fromtip-bot for Adrian Hunter <tipbot@zytor.com>
Date2015-10-20 11:40 +0200
Subject[tip:perf/core] perf/x86: Fix time_shift in perf_event_mmap_page
Message-ID<qlByI-87B-51@gated-at.bofh.it>
In reply to#1248761
Commit-ID:  b9511cd761faafca7a1acc059e792c1399f9d7c6
Gitweb:     http://git.kernel.org/tip/b9511cd761faafca7a1acc059e792c1399f9d7c6
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Fri, 16 Oct 2015 16:24:05 +0300
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 20 Oct 2015 10:30:52 +0200

perf/x86: Fix time_shift in perf_event_mmap_page

Commit:

  b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")

allowed the time_shift value in perf_event_mmap_page to be as much
as 32.  Unfortunately the documented algorithms for using time_shift
have it shifting an integer, whereas to work correctly with the value
32, the type must be u64.

In the case of perf tools, Intel PT decodes correctly but the timestamps
that are output (for example by perf script) have lost 32-bits of
granularity so they look like they are not changing at all.

Fix by limiting the shift to 31 and adjusting the multiplier accordingly.

Also update the documentation of perf_event_mmap_page so that new code
based on it will be more future-proof.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Fixes: b20112edeadf ("perf/x86: Improve accuracy of perf/sched clock")
Link: http://lkml.kernel.org/r/1445001845-13688-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/tsc.c           | 11 +++++++++++
 include/uapi/linux/perf_event.h |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 69b84a2..c7c4d9c 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -259,6 +259,17 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
 	clocks_calc_mult_shift(&data->cyc2ns_mul, &data->cyc2ns_shift, cpu_khz,
 			       NSEC_PER_MSEC, 0);
 
+	/*
+	 * cyc2ns_shift is exported via arch_perf_update_userpage() where it is
+	 * not expected to be greater than 31 due to the original published
+	 * conversion algorithm shifting a 32-bit value (now specifies a 64-bit
+	 * value) - refer perf_event_mmap_page documentation in perf_event.h.
+	 */
+	if (data->cyc2ns_shift == 32) {
+		data->cyc2ns_shift = 31;
+		data->cyc2ns_mul >>= 1;
+	}
+
 	data->cyc2ns_offset = ns_now -
 		mul_u64_u32_shr(tsc_now, data->cyc2ns_mul, data->cyc2ns_shift);
 
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 2881145..6c72e72 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -476,7 +476,7 @@ struct perf_event_mmap_page {
 	 *   u64 delta;
 	 *
 	 *   quot = (cyc >> time_shift);
-	 *   rem = cyc & ((1 << time_shift) - 1);
+	 *   rem = cyc & (((u64)1 << time_shift) - 1);
 	 *   delta = time_offset + quot * time_mult +
 	 *              ((rem * time_mult) >> time_shift);
 	 *
@@ -507,7 +507,7 @@ struct perf_event_mmap_page {
 	 * And vice versa:
 	 *
 	 *   quot = cyc >> time_shift;
-	 *   rem  = cyc & ((1 << time_shift) - 1);
+	 *   rem  = cyc & (((u64)1 << time_shift) - 1);
 	 *   timestamp = time_zero + quot * time_mult +
 	 *               ((rem * time_mult) >> time_shift);
 	 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web