LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1 0/4] perf: Allow suppressing useless AUX records
@ 2017-11-14 12:30 Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 1/4] perf: Allow suppressing " Alexander Shishkin
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Alexander Shishkin @ 2017-11-14 12:30 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin

Hi guys,

[Fixed the braindamage of the previous version as pointed out by
Adrian]

It's been brought to my attention many times that the AUX records are
not always useful. One instance is the AUX overwrite mode, where we
generate one such record every time the target task schedules out.

This patchset adds a bit to the attribute structure that would enable
suppressing the AUX records when the only the only set flag is OVERWRITE.
In this case, at least for the PT and BTS data we just ignore these
records. But at the same time, AUX overwrite events are likely to be
kept running over longer periods of time, making the buildup of AUX
records an unnecessary annoyance.

Alexander Shishkin (4):
  perf: Allow suppressing AUX records
  tools, perf_event.h: Synchronize
  perf tools: Add 'suppress_aux' attribute bit definition and fallback
  perf intel-pt, intel-bts: Suppress useless AUX records by default

 include/uapi/linux/perf_event.h       |  3 ++-
 kernel/events/core.c                  |  5 +++++
 kernel/events/ring_buffer.c           | 12 ++++++++++--
 tools/include/uapi/linux/perf_event.h |  3 ++-
 tools/perf/arch/x86/util/auxtrace.c   |  2 ++
 tools/perf/util/evsel.c               |  9 +++++++++
 6 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.15.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 1/4] perf: Allow suppressing AUX records
  2017-11-14 12:30 [PATCH v1 0/4] perf: Allow suppressing useless AUX records Alexander Shishkin
@ 2017-11-14 12:30 ` Alexander Shishkin
  2017-11-15 12:00   ` Peter Zijlstra
  2017-11-14 12:30 ` [PATCH v1 2/4] tools, perf_event.h: Synchronize Alexander Shishkin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Alexander Shishkin @ 2017-11-14 12:30 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin, Markus Metzger

It has been pointed out to me many times that it is useful to be able
to switch off AUX records to save the bandwidth for records that actually
matter, for example, in AUX overwrite mode.

The usefulness of PERF_RECORD_AUX is in some of its flags, like the
TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
The OVERWRITE flag, on the other hand will be set on every single record
in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
generated on every target task's sched_out, which over time adds up to
a lot of useless information.

This patch adds an attribute bit that enables suppressing such records.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Markus Metzger <markus.t.metzger@intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 include/uapi/linux/perf_event.h |  3 ++-
 kernel/events/core.c            |  5 +++++
 kernel/events/ring_buffer.c     | 12 ++++++++++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 362493a2f950..fa3821d9dc52 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -370,7 +370,8 @@ struct perf_event_attr {
 				context_switch :  1, /* context switch data */
 				write_backward :  1, /* Write ring buffer from end to beginning */
 				namespaces     :  1, /* include namespaces data */
-				__reserved_1   : 35;
+				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
+				__reserved_1   : 34;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 81dd57b9e5e3..483122c73936 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10014,6 +10014,11 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
+	if (attr.suppress_aux && !pmu->setup_aux) {
+		err = -EINVAL;
+		goto err_context;
+	}
+
 	/*
 	 * Look up the group leader (we will attach this event to it):
 	 */
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index f684d8e5fa2b..ecd8da78d387 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -425,6 +425,12 @@ static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
 	return false;
 }
 
+/*
+ * These flags won't generate a PERF_RECORD_AUX on their own if
+ * attr::suppress_aux is set.
+ */
+#define SUPPRESSABLE_FLAGS	PERF_AUX_FLAG_OVERWRITE
+
 /*
  * Commit the data written by hardware into the ring buffer by adjusting
  * aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
@@ -459,8 +465,10 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		 * Only send RECORD_AUX if we have something useful to communicate
 		 */
 
-		perf_event_aux_event(handle->event, aux_head, size,
-		                     handle->aux_flags);
+		if (!handle->event->attr.suppress_aux ||
+		    (handle->aux_flags & ~(u64)SUPPRESSABLE_FLAGS))
+			perf_event_aux_event(handle->event, aux_head, size,
+					     handle->aux_flags);
 	}
 
 	rb->user_page->aux_head = rb->aux_head;
-- 
2.15.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 2/4] tools, perf_event.h: Synchronize
  2017-11-14 12:30 [PATCH v1 0/4] perf: Allow suppressing useless AUX records Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 1/4] perf: Allow suppressing " Alexander Shishkin
@ 2017-11-14 12:30 ` Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 3/4] perf tools: Add 'suppress_aux' attribute bit definition and fallback Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 4/4] perf intel-pt, intel-bts: Suppress useless AUX records by default Alexander Shishkin
  3 siblings, 0 replies; 15+ messages in thread
From: Alexander Shishkin @ 2017-11-14 12:30 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin

I've just added attr::suppress_aux bit to the kernel header, adding
it to the tools' header too.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/include/uapi/linux/perf_event.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index 362493a2f950..fa3821d9dc52 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -370,7 +370,8 @@ struct perf_event_attr {
 				context_switch :  1, /* context switch data */
 				write_backward :  1, /* Write ring buffer from end to beginning */
 				namespaces     :  1, /* include namespaces data */
-				__reserved_1   : 35;
+				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
+				__reserved_1   : 34;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */
-- 
2.15.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 3/4] perf tools: Add 'suppress_aux' attribute bit definition and fallback
  2017-11-14 12:30 [PATCH v1 0/4] perf: Allow suppressing useless AUX records Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 1/4] perf: Allow suppressing " Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 2/4] tools, perf_event.h: Synchronize Alexander Shishkin
@ 2017-11-14 12:30 ` Alexander Shishkin
  2017-11-14 12:30 ` [PATCH v1 4/4] perf intel-pt, intel-bts: Suppress useless AUX records by default Alexander Shishkin
  3 siblings, 0 replies; 15+ messages in thread
From: Alexander Shishkin @ 2017-11-14 12:30 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin

This adds support for suppress_aux, the switch that enables suppressing
not-so-useful PERF_RECORD_AUX records. Also handle kernels where it's
not supported.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/perf/util/evsel.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index f894893c203d..794e56b42c20 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -50,6 +50,7 @@ static struct {
 	bool lbr_flags;
 	bool write_backward;
 	bool group_read;
+	bool suppress_aux;
 } perf_missing_features;
 
 static clockid_t clockid;
@@ -1570,6 +1571,7 @@ int perf_event_attr__fprintf(FILE *fp, struct perf_event_attr *attr,
 	PRINT_ATTRf(use_clockid, p_unsigned);
 	PRINT_ATTRf(context_switch, p_unsigned);
 	PRINT_ATTRf(write_backward, p_unsigned);
+	PRINT_ATTRf(suppress_aux, p_unsigned);
 
 	PRINT_ATTRn("{ wakeup_events, wakeup_watermark }", wakeup_events, p_unsigned);
 	PRINT_ATTRf(bp_type, p_unsigned);
@@ -1686,6 +1688,8 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 				     PERF_SAMPLE_BRANCH_NO_CYCLES);
 	if (perf_missing_features.group_read && evsel->attr.inherit)
 		evsel->attr.read_format &= ~(PERF_FORMAT_GROUP|PERF_FORMAT_ID);
+	if (perf_missing_features.suppress_aux && evsel->attr.suppress_aux)
+		evsel->attr.suppress_aux = 0;
 retry_sample_id:
 	if (perf_missing_features.sample_id_all)
 		evsel->attr.sample_id_all = 0;
@@ -1847,6 +1851,11 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 		perf_missing_features.group_read = true;
 		pr_debug2("switching off group read\n");
 		goto fallback_missing_features;
+	} else if (!perf_missing_features.suppress_aux &&
+		   evsel->attr.suppress_aux) {
+		perf_missing_features.suppress_aux = true;
+		pr_debug2("switching off suppress_aux\n");
+		goto fallback_missing_features;
 	}
 out_close:
 	do {
-- 
2.15.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 4/4] perf intel-pt, intel-bts: Suppress useless AUX records by default
  2017-11-14 12:30 [PATCH v1 0/4] perf: Allow suppressing useless AUX records Alexander Shishkin
                   ` (2 preceding siblings ...)
  2017-11-14 12:30 ` [PATCH v1 3/4] perf tools: Add 'suppress_aux' attribute bit definition and fallback Alexander Shishkin
@ 2017-11-14 12:30 ` Alexander Shishkin
  3 siblings, 0 replies; 15+ messages in thread
From: Alexander Shishkin @ 2017-11-14 12:30 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin

This makes use of the shiny new attr::suppress_aux that suppresses the
AUX records that don't carry any 'interesting' information for the
decoders, that is PERF_RECORD_AUX[flag==OVERWRITE], which just stack up
in the DATA buffer for no good reason.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 tools/perf/arch/x86/util/auxtrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/arch/x86/util/auxtrace.c b/tools/perf/arch/x86/util/auxtrace.c
index 6aa3f2a38321..5700e6099608 100644
--- a/tools/perf/arch/x86/util/auxtrace.c
+++ b/tools/perf/arch/x86/util/auxtrace.c
@@ -45,6 +45,8 @@ struct auxtrace_record *auxtrace_record__init_intel(struct perf_evlist *evlist,
 			if (intel_bts_pmu &&
 			    evsel->attr.type == intel_bts_pmu->type)
 				found_bts = true;
+			if (found_pt || found_bts)
+				evsel->attr.suppress_aux = 1;
 		}
 	}
 
-- 
2.15.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/4] perf: Allow suppressing AUX records
  2017-11-14 12:30 ` [PATCH v1 1/4] perf: Allow suppressing " Alexander Shishkin
@ 2017-11-15 12:00   ` Peter Zijlstra
  2018-01-15 15:00     ` [PATCH] " Alexander Shishkin
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2017-11-15 12:00 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel, Will Deacon,
	Adrian Hunter, Markus Metzger

On Tue, Nov 14, 2017 at 02:30:21PM +0200, Alexander Shishkin wrote:
> It has been pointed out to me many times that it is useful to be able
> to switch off AUX records to save the bandwidth for records that actually
> matter, for example, in AUX overwrite mode.
> 
> The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> The OVERWRITE flag, on the other hand will be set on every single record
> in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> generated on every target task's sched_out, which over time adds up to
> a lot of useless information.
> 
> This patch adds an attribute bit that enables suppressing such records.

What this fails to explain is why a separate suppression flag makes
sense. Why not suppress the thing on overwrite mode and be done with it?

> +		if (!handle->event->attr.suppress_aux ||
> +		    (handle->aux_flags & ~(u64)SUPPRESSABLE_FLAGS))
> +			perf_event_aux_event(handle->event, aux_head, size,
> +					     handle->aux_flags);

That wants { }

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] perf: Allow suppressing AUX records
  2017-11-15 12:00   ` Peter Zijlstra
@ 2018-01-15 15:00     ` Alexander Shishkin
  2018-03-29 11:54       ` Peter Zijlstra
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Shishkin @ 2018-01-15 15:00 UTC (permalink / raw)
  To: Peter Zijlstra, Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, linux-kernel, Will Deacon, Adrian Hunter,
	Alexander Shishkin, Markus Metzger

It has been pointed out to me many times that it is useful to be able
to switch off AUX records to save the bandwidth for records that actually
matter, for example, in AUX overwrite mode.

The usefulness of PERF_RECORD_AUX is in some of its flags, like the
TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
The OVERWRITE flag, on the other hand will be set on every single record
in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
generated on every target task's sched_out, which over time adds up to
a lot of useless information.

In case the existing userspace depends on AUX records in the overwrite
mode, we preserve the original behavior and add an opt-in for the new
behavior, wherein the 'useless' records get suppressed.

This patch adds an attribute bit to the described effect.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Markus Metzger <markus.t.metzger@intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 include/uapi/linux/perf_event.h |  3 ++-
 kernel/events/core.c            |  5 +++++
 kernel/events/ring_buffer.c     | 13 +++++++++++--
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index c77c9a2ebbbb..d7a981130561 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -370,7 +370,8 @@ struct perf_event_attr {
 				context_switch :  1, /* context switch data */
 				write_backward :  1, /* Write ring buffer from end to beginning */
 				namespaces     :  1, /* include namespaces data */
-				__reserved_1   : 35;
+				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
+				__reserved_1   : 34;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4e1a1bf8d867..6245a88c2bda 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10012,6 +10012,11 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
+	if (attr.suppress_aux && !pmu->setup_aux) {
+		err = -EINVAL;
+		goto err_context;
+	}
+
 	/*
 	 * Look up the group leader (we will attach this event to it):
 	 */
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 141aa2ca8728..381f080e6409 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -426,6 +426,12 @@ static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
 	return false;
 }
 
+/*
+ * These flags won't generate a PERF_RECORD_AUX on their own if
+ * attr::suppress_aux is set.
+ */
+#define SUPPRESSABLE_FLAGS	PERF_AUX_FLAG_OVERWRITE
+
 /*
  * Commit the data written by hardware into the ring buffer by adjusting
  * aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
@@ -460,8 +466,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		 * Only send RECORD_AUX if we have something useful to communicate
 		 */
 
-		perf_event_aux_event(handle->event, aux_head, size,
-		                     handle->aux_flags);
+		if (!handle->event->attr.suppress_aux ||
+		    (handle->aux_flags & ~(u64)SUPPRESSABLE_FLAGS)) {
+			perf_event_aux_event(handle->event, aux_head, size,
+					     handle->aux_flags);
+		}
 	}
 
 	rb->user_page->aux_head = rb->aux_head;
-- 
2.15.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] perf: Allow suppressing AUX records
  2018-01-15 15:00     ` [PATCH] " Alexander Shishkin
@ 2018-03-29 11:54       ` Peter Zijlstra
  2018-03-31  9:35         ` Ingo Molnar
  0 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2018-03-29 11:54 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, linux-kernel, Will Deacon,
	Adrian Hunter, Markus Metzger

On Mon, Jan 15, 2018 at 05:00:20PM +0200, Alexander Shishkin wrote:
> It has been pointed out to me many times that it is useful to be able
> to switch off AUX records to save the bandwidth for records that actually
> matter, for example, in AUX overwrite mode.
> 
> The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> The OVERWRITE flag, on the other hand will be set on every single record
> in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> generated on every target task's sched_out, which over time adds up to
> a lot of useless information.
> 
> In case the existing userspace depends on AUX records in the overwrite
> mode, we preserve the original behavior and add an opt-in for the new
> behavior, wherein the 'useless' records get suppressed.
> 
> This patch adds an attribute bit to the described effect.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Markus Metzger <markus.t.metzger@intel.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  include/uapi/linux/perf_event.h |  3 ++-
>  kernel/events/core.c            |  5 +++++
>  kernel/events/ring_buffer.c     | 13 +++++++++++--
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index c77c9a2ebbbb..d7a981130561 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -370,7 +370,8 @@ struct perf_event_attr {
>  				context_switch :  1, /* context switch data */
>  				write_backward :  1, /* Write ring buffer from end to beginning */
>  				namespaces     :  1, /* include namespaces data */
> -				__reserved_1   : 35;
> +				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
> +				__reserved_1   : 34;
>  
>  	union {
>  		__u32		wakeup_events;	  /* wakeup every n events */

So I'm basically fine with this patch, however I wonder if we really
need this suppress flag and can't just unconditionally drop these
events.

Ash said that as far as he knows no Intel-PT user actually relies on it;
Will is there anything ARM that is known to rely on them?

In anycase, tentative ACK on this, unless we wants to be brave and forgo
this flag.

Ingo, any opinions?


> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4e1a1bf8d867..6245a88c2bda 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10012,6 +10012,11 @@ SYSCALL_DEFINE5(perf_event_open,
>  		goto err_context;
>  	}
>  
> +	if (attr.suppress_aux && !pmu->setup_aux) {
> +		err = -EINVAL;
> +		goto err_context;
> +	}
> +
>  	/*
>  	 * Look up the group leader (we will attach this event to it):
>  	 */
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 141aa2ca8728..381f080e6409 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -426,6 +426,12 @@ static bool __always_inline rb_need_aux_wakeup(struct ring_buffer *rb)
>  	return false;
>  }
>  
> +/*
> + * These flags won't generate a PERF_RECORD_AUX on their own if
> + * attr::suppress_aux is set.
> + */
> +#define SUPPRESSABLE_FLAGS	PERF_AUX_FLAG_OVERWRITE
> +
>  /*
>   * Commit the data written by hardware into the ring buffer by adjusting
>   * aux_head and posting a PERF_RECORD_AUX into the perf buffer. It is the
> @@ -460,8 +466,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  		 * Only send RECORD_AUX if we have something useful to communicate
>  		 */
>  
> -		perf_event_aux_event(handle->event, aux_head, size,
> -		                     handle->aux_flags);
> +		if (!handle->event->attr.suppress_aux ||
> +		    (handle->aux_flags & ~(u64)SUPPRESSABLE_FLAGS)) {
> +			perf_event_aux_event(handle->event, aux_head, size,
> +					     handle->aux_flags);
> +		}
>  	}
>  
>  	rb->user_page->aux_head = rb->aux_head;
> -- 
> 2.15.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] perf: Allow suppressing AUX records
  2018-03-29 11:54       ` Peter Zijlstra
@ 2018-03-31  9:35         ` Ingo Molnar
  2018-04-03 17:32           ` Will Deacon
  0 siblings, 1 reply; 15+ messages in thread
From: Ingo Molnar @ 2018-03-31  9:35 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Alexander Shishkin, Arnaldo Carvalho de Melo, Ingo Molnar,
	linux-kernel, Will Deacon, Adrian Hunter, Markus Metzger


* Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Jan 15, 2018 at 05:00:20PM +0200, Alexander Shishkin wrote:
> > It has been pointed out to me many times that it is useful to be able
> > to switch off AUX records to save the bandwidth for records that actually
> > matter, for example, in AUX overwrite mode.
> > 
> > The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> > TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> > The OVERWRITE flag, on the other hand will be set on every single record
> > in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> > generated on every target task's sched_out, which over time adds up to
> > a lot of useless information.
> > 
> > In case the existing userspace depends on AUX records in the overwrite
> > mode, we preserve the original behavior and add an opt-in for the new
> > behavior, wherein the 'useless' records get suppressed.
> > 
> > This patch adds an attribute bit to the described effect.
> > 
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Markus Metzger <markus.t.metzger@intel.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > ---
> >  include/uapi/linux/perf_event.h |  3 ++-
> >  kernel/events/core.c            |  5 +++++
> >  kernel/events/ring_buffer.c     | 13 +++++++++++--
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index c77c9a2ebbbb..d7a981130561 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -370,7 +370,8 @@ struct perf_event_attr {
> >  				context_switch :  1, /* context switch data */
> >  				write_backward :  1, /* Write ring buffer from end to beginning */
> >  				namespaces     :  1, /* include namespaces data */
> > -				__reserved_1   : 35;
> > +				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
> > +				__reserved_1   : 34;
> >  
> >  	union {
> >  		__u32		wakeup_events;	  /* wakeup every n events */
> 
> So I'm basically fine with this patch, however I wonder if we really
> need this suppress flag and can't just unconditionally drop these
> events.
> 
> Ash said that as far as he knows no Intel-PT user actually relies on it;
> Will is there anything ARM that is known to rely on them?
> 
> In anycase, tentative ACK on this, unless we wants to be brave and forgo
> this flag.
> 
> Ingo, any opinions?

Yeah, I'd suggest we just supress those record, and wait for complaints - let's 
not complicate the ABI if not necessary?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] perf: Allow suppressing AUX records
  2018-03-31  9:35         ` Ingo Molnar
@ 2018-04-03 17:32           ` Will Deacon
  2018-04-04 14:53             ` [PATCH v2] perf: Suppress AUX/OVERWRITE records Alexander Shishkin
  0 siblings, 1 reply; 15+ messages in thread
From: Will Deacon @ 2018-04-03 17:32 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Alexander Shishkin, Arnaldo Carvalho de Melo,
	Ingo Molnar, linux-kernel, Adrian Hunter, Markus Metzger

On Sat, Mar 31, 2018 at 11:35:46AM +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Jan 15, 2018 at 05:00:20PM +0200, Alexander Shishkin wrote:
> > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > > index c77c9a2ebbbb..d7a981130561 100644
> > > --- a/include/uapi/linux/perf_event.h
> > > +++ b/include/uapi/linux/perf_event.h
> > > @@ -370,7 +370,8 @@ struct perf_event_attr {
> > >  				context_switch :  1, /* context switch data */
> > >  				write_backward :  1, /* Write ring buffer from end to beginning */
> > >  				namespaces     :  1, /* include namespaces data */
> > > -				__reserved_1   : 35;
> > > +				suppress_aux   :  1, /* don't generate PERF_RECORD_AUX */
> > > +				__reserved_1   : 34;
> > >  
> > >  	union {
> > >  		__u32		wakeup_events;	  /* wakeup every n events */
> > 
> > So I'm basically fine with this patch, however I wonder if we really
> > need this suppress flag and can't just unconditionally drop these
> > events.
> > 
> > Ash said that as far as he knows no Intel-PT user actually relies on it;
> > Will is there anything ARM that is known to rely on them?
> > 
> > In anycase, tentative ACK on this, unless we wants to be brave and forgo
> > this flag.
> > 
> > Ingo, any opinions?
> 
> Yeah, I'd suggest we just supress those record, and wait for complaints - let's 
> not complicate the ABI if not necessary?

Works for me. We've not had SPE support in mainline perf for very long and
the availability of hardware is extremely limited at the moment, so I don't
anticipate any ABI implications on the arm64 side.

Cheers,

Will

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] perf: Suppress AUX/OVERWRITE records
  2018-04-03 17:32           ` Will Deacon
@ 2018-04-04 14:53             ` Alexander Shishkin
  2018-05-04 12:09               ` Alexander Shishkin
  2018-09-25  9:27               ` [tip:perf/core] " tip-bot for Alexander Shishkin
  0 siblings, 2 replies; 15+ messages in thread
From: Alexander Shishkin @ 2018-04-04 14:53 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Will Deacon,
	Adrian Hunter, Alexander Shishkin, Markus Metzger

It has been pointed out to me many times that it is useful to be able
to switch off AUX records to save the bandwidth for records that actually
matter, for example, in AUX overwrite mode.

The usefulness of PERF_RECORD_AUX is in some of its flags, like the
TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
The OVERWRITE flag, on the other hand will be set on every single record
in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
generated on every target task's sched_out, which over time adds up to
a lot of useless information.

If any folks out there have userspace that depends on a constant stream of
OVERWRITE records for a good reason, they'll have to let us know.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Markus Metzger <markus.t.metzger@intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 kernel/events/ring_buffer.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 6c6b3c48db71..c4edd8fbc5d9 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -458,10 +458,20 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 	if (size || handle->aux_flags) {
 		/*
 		 * Only send RECORD_AUX if we have something useful to communicate
+		 *
+		 * Note: the OVERWRITE records by themselves are not considered
+		 * useful, as they don't communicate any *new* information,
+		 * aside from the short-lived offset, that becomes history at
+		 * the next event sched-in and therefore isn't useful.
+		 * The userspace that needs to copy out AUX data in overwrite
+		 * mode should know to use user_page::aux_head for the actual
+		 * offset. So, from now on we don't output AUX records that
+		 * have *only* OVERWRITE flag set.
 		 */
 
-		perf_event_aux_event(handle->event, aux_head, size,
-		                     handle->aux_flags);
+		if (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE)
+			perf_event_aux_event(handle->event, aux_head, size,
+			                     handle->aux_flags);
 	}
 
 	rb->user_page->aux_head = rb->aux_head;
-- 
2.16.3

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] perf: Suppress AUX/OVERWRITE records
  2018-04-04 14:53             ` [PATCH v2] perf: Suppress AUX/OVERWRITE records Alexander Shishkin
@ 2018-05-04 12:09               ` Alexander Shishkin
  2018-05-04 15:35                 ` Arnaldo Carvalho de Melo
  2018-09-25  9:27               ` [tip:perf/core] " tip-bot for Alexander Shishkin
  1 sibling, 1 reply; 15+ messages in thread
From: Alexander Shishkin @ 2018-05-04 12:09 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	linux-kernel, Will Deacon, Adrian Hunter, Markus Metzger

On Wed, Apr 04, 2018 at 05:53:23PM +0300, Alexander Shishkin wrote:
> It has been pointed out to me many times that it is useful to be able
> to switch off AUX records to save the bandwidth for records that actually
> matter, for example, in AUX overwrite mode.
> 
> The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> The OVERWRITE flag, on the other hand will be set on every single record
> in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> generated on every target task's sched_out, which over time adds up to
> a lot of useless information.
> 
> If any folks out there have userspace that depends on a constant stream of
> OVERWRITE records for a good reason, they'll have to let us know.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Markus Metzger <markus.t.metzger@intel.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>

This one seems to be slipping through the cracks.

Cheers,
--
Alex

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] perf: Suppress AUX/OVERWRITE records
  2018-05-04 12:09               ` Alexander Shishkin
@ 2018-05-04 15:35                 ` Arnaldo Carvalho de Melo
  2018-05-04 15:36                   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-05-04 15:35 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Will Deacon,
	Adrian Hunter, Markus Metzger

Em Fri, May 04, 2018 at 03:09:59PM +0300, Alexander Shishkin escreveu:
> On Wed, Apr 04, 2018 at 05:53:23PM +0300, Alexander Shishkin wrote:
> > It has been pointed out to me many times that it is useful to be able
> > to switch off AUX records to save the bandwidth for records that actually
> > matter, for example, in AUX overwrite mode.
> > 
> > The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> > TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> > The OVERWRITE flag, on the other hand will be set on every single record
> > in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> > generated on every target task's sched_out, which over time adds up to
> > a lot of useless information.
> > 
> > If any folks out there have userspace that depends on a constant stream of
> > OVERWRITE records for a good reason, they'll have to let us know.
> > 
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Markus Metzger <markus.t.metzger@intel.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> 
> This one seems to be slipping through the cracks.

So, did you got Acked-by  or tested-by from anyone?


- Arnaldo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] perf: Suppress AUX/OVERWRITE records
  2018-05-04 15:35                 ` Arnaldo Carvalho de Melo
@ 2018-05-04 15:36                   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-05-04 15:36 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Peter Zijlstra, Ingo Molnar, linux-kernel, Will Deacon,
	Adrian Hunter, Markus Metzger

Em Fri, May 04, 2018 at 12:35:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, May 04, 2018 at 03:09:59PM +0300, Alexander Shishkin escreveu:
> > On Wed, Apr 04, 2018 at 05:53:23PM +0300, Alexander Shishkin wrote:
> > > It has been pointed out to me many times that it is useful to be able
> > > to switch off AUX records to save the bandwidth for records that actually
> > > matter, for example, in AUX overwrite mode.
> > > 
> > > The usefulness of PERF_RECORD_AUX is in some of its flags, like the
> > > TRUNCATED flag that tells the decoder where exactly gaps in the trace are.
> > > The OVERWRITE flag, on the other hand will be set on every single record
> > > in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
> > > generated on every target task's sched_out, which over time adds up to
> > > a lot of useless information.
> > > 
> > > If any folks out there have userspace that depends on a constant stream of
> > > OVERWRITE records for a good reason, they'll have to let us know.
> > > 
> > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Markus Metzger <markus.t.metzger@intel.com>
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > 
> > This one seems to be slipping through the cracks.
> 
> So, did you got Acked-by  or tested-by from anyone?

Yeah, tons of them, I'll pick this up

- Arnaldo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [tip:perf/core] perf: Suppress AUX/OVERWRITE records
  2018-04-04 14:53             ` [PATCH v2] perf: Suppress AUX/OVERWRITE records Alexander Shishkin
  2018-05-04 12:09               ` Alexander Shishkin
@ 2018-09-25  9:27               ` tip-bot for Alexander Shishkin
  1 sibling, 0 replies; 15+ messages in thread
From: tip-bot for Alexander Shishkin @ 2018-09-25  9:27 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: markus.t.metzger, will.deacon, tglx, linux-kernel, hpa,
	alexander.shishkin, mingo, peterz, acme, adrian.hunter

Commit-ID:  1627314fb54a33ebd23bd08f2e215eaed0f44712
Gitweb:     https://git.kernel.org/tip/1627314fb54a33ebd23bd08f2e215eaed0f44712
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Wed, 4 Apr 2018 17:53:23 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 18 Sep 2018 17:21:13 -0300

perf: Suppress AUX/OVERWRITE records

It has been pointed out to me many times that it is useful to be able to
switch off AUX records to save the bandwidth for records that actually
matter, for example, in AUX overwrite mode.

The usefulness of PERF_RECORD_AUX is in some of its flags, like the
TRUNCATED flag that tells the decoder where exactly gaps in the trace
are.  The OVERWRITE flag, on the other hand will be set on every single
record in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is
generated on every target task's sched_out, which over time adds up to a
lot of useless information.

If any folks out there have userspace that depends on a constant stream
of OVERWRITE records for a good reason, they'll have to let us know.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Markus T Metzger <markus.t.metzger@intel.com>
Link: http://lkml.kernel.org/r/20180404145323.28651-1-alexander.shishkin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 kernel/events/ring_buffer.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 5d3cf407e374..4a9937076331 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -459,10 +459,20 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 	if (size || handle->aux_flags) {
 		/*
 		 * Only send RECORD_AUX if we have something useful to communicate
+		 *
+		 * Note: the OVERWRITE records by themselves are not considered
+		 * useful, as they don't communicate any *new* information,
+		 * aside from the short-lived offset, that becomes history at
+		 * the next event sched-in and therefore isn't useful.
+		 * The userspace that needs to copy out AUX data in overwrite
+		 * mode should know to use user_page::aux_head for the actual
+		 * offset. So, from now on we don't output AUX records that
+		 * have *only* OVERWRITE flag set.
 		 */
 
-		perf_event_aux_event(handle->event, aux_head, size,
-		                     handle->aux_flags);
+		if (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE)
+			perf_event_aux_event(handle->event, aux_head, size,
+			                     handle->aux_flags);
 	}
 
 	rb->user_page->aux_head = rb->aux_head;

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-09-25  9:29 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-14 12:30 [PATCH v1 0/4] perf: Allow suppressing useless AUX records Alexander Shishkin
2017-11-14 12:30 ` [PATCH v1 1/4] perf: Allow suppressing " Alexander Shishkin
2017-11-15 12:00   ` Peter Zijlstra
2018-01-15 15:00     ` [PATCH] " Alexander Shishkin
2018-03-29 11:54       ` Peter Zijlstra
2018-03-31  9:35         ` Ingo Molnar
2018-04-03 17:32           ` Will Deacon
2018-04-04 14:53             ` [PATCH v2] perf: Suppress AUX/OVERWRITE records Alexander Shishkin
2018-05-04 12:09               ` Alexander Shishkin
2018-05-04 15:35                 ` Arnaldo Carvalho de Melo
2018-05-04 15:36                   ` Arnaldo Carvalho de Melo
2018-09-25  9:27               ` [tip:perf/core] " tip-bot for Alexander Shishkin
2017-11-14 12:30 ` [PATCH v1 2/4] tools, perf_event.h: Synchronize Alexander Shishkin
2017-11-14 12:30 ` [PATCH v1 3/4] perf tools: Add 'suppress_aux' attribute bit definition and fallback Alexander Shishkin
2017-11-14 12:30 ` [PATCH v1 4/4] perf intel-pt, intel-bts: Suppress useless AUX records by default Alexander Shishkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).