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

Groups > linux.kernel > #1571459 > unrolled thread

Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing Ingo Molnar <mingo@kernel.org> - 2017-02-01 11:20 +0100
    Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-01 17:50 +0100
      Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-02 11:20 +0100
        [PATCH] perf/x86/intel/pt: Allow disabling branch tracing Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-06 15:50 +0100
          Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing Andi Kleen <andi@firstfloor.org> - 2017-02-06 17:00 +0100
            Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-02-06 17:10 +0100
              Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing Andi Kleen <andi@firstfloor.org> - 2017-02-06 18:20 +0100

#1571459 — Re: [PATCH 2/2] perf/x86/intel/pt: Allow disabling branch tracing

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> Now that Intel PT supports more types of trace content than just branch
> tracing, it may be useful to allow the user to disable branch tracing
> when it is not needed.
> 
> The special case is BDW, where not setting BranchEn is not supported.
> 
> This patch adds 'no_branch' event format string to PT events, which
> will disable setting BranchEn bit in the hardware trace configuration.

> +	/* trying to unset BRANCH_EN where it is not supported */

Please capitalize comments consistently and use the typical tense. This one should 
be something like:

	/* Try to unset BRANCH_EN where it is not supported: */

>  
>  	reg = pt_config_filters(event);
> -	reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN;
> +	reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN;
> +
> +	/*
> +	 * Previously, we had BRANCH_EN on by default, but now that PT has
> +	 * grown features outside of branch tracing, it is useful to allow
> +	 * the user to disable it. So, to keep compatibility, setting
> +	 * BRANCH_EN bit in the event config (no_branch=1) will have the
> +	 * reverse effect and *not* set BRANCH_EN in the hardware
> +	 * configuration.
> +	 */
> +	if (!(event->attr.config & RTIT_CTL_BRANCH_EN))
> +		reg |= RTIT_CTL_BRANCH_EN;
> +	else
> +		event->attr.config &= ~RTIT_CTL_BRANCH_EN;


So I really hate this ABI hack - it's these unclean approaches how ABIs degrade 
over time, by death of a thousand cuts...

Any reason why we couldn't add a separate pt_feature_branch_disable and 
pt_feature_trace_disable bits and interpret them in a straightforward way, or 
something like that?

( This means two more bits, but that's our punishment for not anticipating 
  extensions to the hardware feature. )

Also, rename "RTIT_CTL_BRANCH_EN" to "RTIT_CTL_PT_EN" (but without changing the 
ABI), to more clearly express what that bit realy does.

I.e. we'd have a hierarchy of flags:

	- the old RTIT_CTL_BRANCH_EN bit (now RTIT_CTL_PT_EN) enables all of PT, 
	  with all features

	- individual feature disabling bits, which default to 0 (i.e. the feature 
	  is enabled) in the attr structure control the finegrained 
	  enabling/disabling of PT features. Currently there are two bits: 
	  pt_feature_branch_disable and pt_feature_branch_enable. More are added 
	  in the future if PT grows even more features.

or so?

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1571728

Ingo Molnar <mingo@kernel.org> writes:

> * Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:
>
>> Now that Intel PT supports more types of trace content than just branch
>> tracing, it may be useful to allow the user to disable branch tracing
>> when it is not needed.
>> 
>> The special case is BDW, where not setting BranchEn is not supported.
>> 
>> This patch adds 'no_branch' event format string to PT events, which
>> will disable setting BranchEn bit in the hardware trace configuration.
>
>> +	/* trying to unset BRANCH_EN where it is not supported */
>
> Please capitalize comments consistently and use the typical tense. This one should 
> be something like:
>
> 	/* Try to unset BRANCH_EN where it is not supported: */

Will do.

>> +	if (!(event->attr.config & RTIT_CTL_BRANCH_EN))
>> +		reg |= RTIT_CTL_BRANCH_EN;
>> +	else
>> +		event->attr.config &= ~RTIT_CTL_BRANCH_EN;
>
>
> So I really hate this ABI hack - it's these unclean approaches how ABIs degrade 
> over time, by death of a thousand cuts...

Agreed.

> Any reason why we couldn't add a separate pt_feature_branch_disable and 
> pt_feature_trace_disable bits and interpret them in a straightforward way, or 
> something like that?
>
> ( This means two more bits, but that's our punishment for not anticipating 
>   extensions to the hardware feature. )

Most features (all but BranchEn) are at the moment set-to-enable. It is
only this unfortunate default of BranchEn=1 that is a pain. Another
relatively painless thing we can do is add a 'passthrough' bit, which
will allow the user to (not) set BranchEn bit when passthrough=1 and
behave like it did before, when passthrough=0. Like below:

From d28174b582aba285baa1ab9fea73b554e8937f89 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Thu, 19 Jan 2017 10:00:52 +0200
Subject: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

Now that Intel PT supports more types of trace content than just branch
tracing, it may be useful to allow the user to disable branch tracing
when it is not needed.

The special case is BDW, where not setting BranchEn is not supported.

This is slightly trickier than necessary, because up to this moment
the driver has been setting BranchEn automatically and the userspace
assumes as much. Instead of reversing the semantics of BranchEn, we
introduce a 'passthrough' bit, which will forego the default and allow
the user to set BranchEn to their heart's content. A new synthetic
capability is also introduced to advertise the availability of this
'passthrough' bit.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/events/intel/pt.c | 84 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/x86/events/intel/pt.h |  2 ++
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index ece5fb06db..a31b570577 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -28,6 +28,7 @@
 #include <asm/insn.h>
 #include <asm/io.h>
 #include <asm/intel_pt.h>
+#include <asm/intel-family.h>
 
 #include "../perf_event.h"
 #include "pt.h"
@@ -50,6 +51,10 @@ static struct pt_pmu pt_pmu;
 #define PT_CAP(_n, _l, _r, _m)						\
 	[PT_CAP_ ## _n] = { .name = __stringify(_n), .leaf = _l,	\
 			    .reg = _r, .mask = _m }
+/*
+ * Software-defined/driver capabilities
+ */
+#define PT_SW_CAP	0xffffffff
 
 static struct pt_cap_desc {
 	const char	*name;
@@ -72,12 +77,14 @@ static struct pt_cap_desc {
 	PT_CAP(mtc_periods,		1, CPUID_EAX, 0xffff0000),
 	PT_CAP(cycle_thresholds,	1, CPUID_EBX, 0xffff),
 	PT_CAP(psb_periods,		1, CPUID_EBX, 0xffff0000),
+	PT_CAP(passthrough,		PT_SW_CAP, 0, 1),
 };
 
 static u32 pt_cap_get(enum pt_capabilities cap)
 {
 	struct pt_cap_desc *cd = &pt_caps[cap];
-	u32 c = pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
+	u32 c = cd->leaf == PT_SW_CAP ? 1 :
+		pt_pmu.caps[cd->leaf * PT_CPUID_REGS_NUM + cd->reg];
 	unsigned int shift = __ffs(cd->mask);
 
 	return (c & cd->mask) >> shift;
@@ -98,6 +105,7 @@ static struct attribute_group pt_cap_group = {
 	.name	= "caps",
 };
 
+PMU_FORMAT_ATTR(passthrough,	"config:0"	);
 PMU_FORMAT_ATTR(cyc,		"config:1"	);
 PMU_FORMAT_ATTR(pwr_evt,	"config:4"	);
 PMU_FORMAT_ATTR(fup_on_ptw,	"config:5"	);
@@ -105,11 +113,13 @@ PMU_FORMAT_ATTR(mtc,		"config:9"	);
 PMU_FORMAT_ATTR(tsc,		"config:10"	);
 PMU_FORMAT_ATTR(noretcomp,	"config:11"	);
 PMU_FORMAT_ATTR(ptw,		"config:12"	);
+PMU_FORMAT_ATTR(branch,		"config:13"	);
 PMU_FORMAT_ATTR(mtc_period,	"config:14-17"	);
 PMU_FORMAT_ATTR(cyc_thresh,	"config:19-22"	);
 PMU_FORMAT_ATTR(psb_period,	"config:24-27"	);
 
 static struct attribute *pt_formats_attr[] = {
+	&format_attr_passthrough.attr,
 	&format_attr_cyc.attr,
 	&format_attr_pwr_evt.attr,
 	&format_attr_fup_on_ptw.attr,
@@ -117,6 +127,7 @@ static struct attribute *pt_formats_attr[] = {
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
 	&format_attr_ptw.attr,
+	&format_attr_branch.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,
@@ -197,6 +208,19 @@ static int __init pt_pmu_hw_init(void)
 		pt_pmu.tsc_art_den = eax;
 	}
 
+	/* model-specific quirks */
+	switch (boot_cpu_data.x86_model) {
+	case INTEL_FAM6_BROADWELL_CORE:
+	case INTEL_FAM6_BROADWELL_XEON_D:
+	case INTEL_FAM6_BROADWELL_GT3E:
+	case INTEL_FAM6_BROADWELL_X:
+		/* not setting BRANCH_EN will #GP, erratum BDM106 */
+		pt_pmu.branch_en_always_on = true;
+		break;
+	default:
+		break;
+	}
+
 	if (boot_cpu_has(X86_FEATURE_VMX)) {
 		/*
 		 * Intel SDM, 36.5 "Tracing post-VMXON" says that
@@ -263,8 +287,22 @@ static int __init pt_pmu_hw_init(void)
 #define RTIT_CTL_PTW	(RTIT_CTL_PTW_EN	| \
 			 RTIT_CTL_FUP_ON_PTW)
 
-#define PT_CONFIG_MASK (RTIT_CTL_TSC_EN		| \
+/*
+ * Bit 0 (TraceEn) in the attr.config is meaningless as the
+ * corresponding bit in the RTIT_CTL can only be controlled
+ * by the driver; therefore, repurpose it to mean: pass
+ * through the bit that was previously assumed to be always
+ * on for PT, thereby allowing the user to *not* set it if
+ * they so wish. See also pt_event_valid() and pt_config().
+ * The synthetic "passthough" capability tells the userspace
+ * that this feature is available.
+ */
+#define RTIT_CTL_PASSTHROUGH RTIT_CTL_TRACEEN
+
+#define PT_CONFIG_MASK (RTIT_CTL_TRACEEN	| \
+			RTIT_CTL_TSC_EN		| \
 			RTIT_CTL_DISRETC	| \
+			RTIT_CTL_BRANCH_EN	| \
 			RTIT_CTL_CYC_PSB	| \
 			RTIT_CTL_MTC		| \
 			RTIT_CTL_PWR_EVT_EN	| \
@@ -332,6 +370,33 @@ static bool pt_event_valid(struct perf_event *event)
 			return false;
 	}
 
+	/*
+	 * Setting bit 0 (TraceEn in RTIT_CTL MSR) in the attr.config
+	 * clears the assomption that BranchEn must always be enabled,
+	 * as was the case with the first implementation of PT.
+	 * If this bit is not set, the legacy behavior is preserved
+	 * for compatibility with the older userspace.
+	 *
+	 * Re-using bit 0 for this purpose is fine because it is never
+	 * directly set by the user; previous attempts at setting it in
+	 * the attr.config resulted in -EINVAL.
+	 */
+	if (config & RTIT_CTL_PASSTHROUGH) {
+		/*
+		 * Disallow not setting BRANCH_EN where BRANCH_EN is
+		 * always required.
+		 */
+		if (pt_pmu.branch_en_always_on &&
+		    !(config & RTIT_CTL_BRANCH_EN))
+			return false;
+	} else {
+		/*
+		 * Disallow BRANCH_EN without the PASSTHROUGH.
+		 */
+		if (config & RTIT_CTL_BRANCH_EN)
+			return false;
+	}
+
 	return true;
 }
 
@@ -419,7 +484,20 @@ static void pt_config(struct perf_event *event)
 	}
 
 	reg = pt_config_filters(event);
-	reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN;
+	reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN;
+
+	/*
+	 * Previously, we had BRANCH_EN on by default, but now that PT has
+	 * grown features outside of branch tracing, it is useful to allow
+	 * the user to disable it. Setting bit 0 in the event's attr.config
+	 * allows BRANCH_EN to pass through instead of being always on. See
+	 * also the comment in pt_event_valid().
+	 */
+	if (event->attr.config & BIT(0)) {
+		reg |= event->attr.config & RTIT_CTL_BRANCH_EN;
+	} else {
+		reg |= RTIT_CTL_BRANCH_EN;
+	}
 
 	if (!event->attr.exclude_kernel)
 		reg |= RTIT_CTL_OS;
diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h
index 53473c21b5..0f17367b7b 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -104,12 +104,14 @@ enum pt_capabilities {
 	PT_CAP_mtc_periods,
 	PT_CAP_cycle_thresholds,
 	PT_CAP_psb_periods,
+	PT_CAP_passthrough,
 };
 
 struct pt_pmu {
 	struct pmu		pmu;
 	u32			caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES];
 	bool			vmx;
+	bool			branch_en_always_on;
 	unsigned long		max_nonturbo_ratio;
 	unsigned int		tsc_art_num;
 	unsigned int		tsc_art_den;
-- 
2.11.0

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

#1572244

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> @@ -72,12 +77,14 @@ static struct pt_cap_desc {
>  	PT_CAP(mtc_periods,		1, CPUID_EAX, 0xffff0000),
>  	PT_CAP(cycle_thresholds,	1, CPUID_EBX, 0xffff),
>  	PT_CAP(psb_periods,		1, CPUID_EBX, 0xffff0000),
> +	PT_CAP(passthrough,		PT_SW_CAP, 0, 1),

Thinking some more: we don't actually need this as the format string
already serves this purpose.

Regards,
--
Alex

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

#1574848 — [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

Now that Intel PT supports more types of trace content than just branch
tracing, it may be useful to allow the user to disable branch tracing
when it is not needed.

The special case is BDW, where not setting BranchEn is not supported.

This is slightly trickier than necessary, because up to this moment
the driver has been setting BranchEn automatically and the userspace
assumes as much. Instead of reversing the semantics of BranchEn, we
introduce a 'passthrough' bit, which will forego the default and allow
the user to set BranchEn to their heart's content.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/events/intel/pt.c | 74 ++++++++++++++++++++++++++++++++++++++++++++--
 arch/x86/events/intel/pt.h |  1 +
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 9e92a4097d..d92a60ef08 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -28,6 +28,7 @@
 #include <asm/insn.h>
 #include <asm/io.h>
 #include <asm/intel_pt.h>
+#include <asm/intel-family.h>
 
 #include "../perf_event.h"
 #include "pt.h"
@@ -98,6 +99,7 @@ static struct attribute_group pt_cap_group = {
 	.name	= "caps",
 };
 
+PMU_FORMAT_ATTR(passthrough,	"config:0"	);
 PMU_FORMAT_ATTR(cyc,		"config:1"	);
 PMU_FORMAT_ATTR(pwr_evt,	"config:4"	);
 PMU_FORMAT_ATTR(fup_on_ptw,	"config:5"	);
@@ -105,11 +107,13 @@ PMU_FORMAT_ATTR(mtc,		"config:9"	);
 PMU_FORMAT_ATTR(tsc,		"config:10"	);
 PMU_FORMAT_ATTR(noretcomp,	"config:11"	);
 PMU_FORMAT_ATTR(ptw,		"config:12"	);
+PMU_FORMAT_ATTR(branch,		"config:13"	);
 PMU_FORMAT_ATTR(mtc_period,	"config:14-17"	);
 PMU_FORMAT_ATTR(cyc_thresh,	"config:19-22"	);
 PMU_FORMAT_ATTR(psb_period,	"config:24-27"	);
 
 static struct attribute *pt_formats_attr[] = {
+	&format_attr_passthrough.attr,
 	&format_attr_cyc.attr,
 	&format_attr_pwr_evt.attr,
 	&format_attr_fup_on_ptw.attr,
@@ -117,6 +121,7 @@ static struct attribute *pt_formats_attr[] = {
 	&format_attr_tsc.attr,
 	&format_attr_noretcomp.attr,
 	&format_attr_ptw.attr,
+	&format_attr_branch.attr,
 	&format_attr_mtc_period.attr,
 	&format_attr_cyc_thresh.attr,
 	&format_attr_psb_period.attr,
@@ -197,6 +202,19 @@ static int __init pt_pmu_hw_init(void)
 		pt_pmu.tsc_art_den = eax;
 	}
 
+	/* model-specific quirks */
+	switch (boot_cpu_data.x86_model) {
+	case INTEL_FAM6_BROADWELL_CORE:
+	case INTEL_FAM6_BROADWELL_XEON_D:
+	case INTEL_FAM6_BROADWELL_GT3E:
+	case INTEL_FAM6_BROADWELL_X:
+		/* not setting BRANCH_EN will #GP, erratum BDM106 */
+		pt_pmu.branch_en_always_on = true;
+		break;
+	default:
+		break;
+	}
+
 	if (boot_cpu_has(X86_FEATURE_VMX)) {
 		/*
 		 * Intel SDM, 36.5 "Tracing post-VMXON" says that
@@ -263,8 +281,20 @@ static int __init pt_pmu_hw_init(void)
 #define RTIT_CTL_PTW	(RTIT_CTL_PTW_EN	| \
 			 RTIT_CTL_FUP_ON_PTW)
 
-#define PT_CONFIG_MASK (RTIT_CTL_TSC_EN		| \
+/*
+ * Bit 0 (TraceEn) in the attr.config is meaningless as the
+ * corresponding bit in the RTIT_CTL can only be controlled
+ * by the driver; therefore, repurpose it to mean: pass
+ * through the bit that was previously assumed to be always
+ * on for PT, thereby allowing the user to *not* set it if
+ * they so wish. See also pt_event_valid() and pt_config().
+ */
+#define RTIT_CTL_PASSTHROUGH RTIT_CTL_TRACEEN
+
+#define PT_CONFIG_MASK (RTIT_CTL_TRACEEN	| \
+			RTIT_CTL_TSC_EN		| \
 			RTIT_CTL_DISRETC	| \
+			RTIT_CTL_BRANCH_EN	| \
 			RTIT_CTL_CYC_PSB	| \
 			RTIT_CTL_MTC		| \
 			RTIT_CTL_PWR_EVT_EN	| \
@@ -332,6 +362,33 @@ static bool pt_event_valid(struct perf_event *event)
 			return false;
 	}
 
+	/*
+	 * Setting bit 0 (TraceEn in RTIT_CTL MSR) in the attr.config
+	 * clears the assomption that BranchEn must always be enabled,
+	 * as was the case with the first implementation of PT.
+	 * If this bit is not set, the legacy behavior is preserved
+	 * for compatibility with the older userspace.
+	 *
+	 * Re-using bit 0 for this purpose is fine because it is never
+	 * directly set by the user; previous attempts at setting it in
+	 * the attr.config resulted in -EINVAL.
+	 */
+	if (config & RTIT_CTL_PASSTHROUGH) {
+		/*
+		 * Disallow not setting BRANCH_EN where BRANCH_EN is
+		 * always required.
+		 */
+		if (pt_pmu.branch_en_always_on &&
+		    !(config & RTIT_CTL_BRANCH_EN))
+			return false;
+	} else {
+		/*
+		 * Disallow BRANCH_EN without the PASSTHROUGH.
+		 */
+		if (config & RTIT_CTL_BRANCH_EN)
+			return false;
+	}
+
 	return true;
 }
 
@@ -419,7 +476,20 @@ static void pt_config(struct perf_event *event)
 	}
 
 	reg = pt_config_filters(event);
-	reg |= RTIT_CTL_TOPA | RTIT_CTL_BRANCH_EN | RTIT_CTL_TRACEEN;
+	reg |= RTIT_CTL_TOPA | RTIT_CTL_TRACEEN;
+
+	/*
+	 * Previously, we had BRANCH_EN on by default, but now that PT has
+	 * grown features outside of branch tracing, it is useful to allow
+	 * the user to disable it. Setting bit 0 in the event's attr.config
+	 * allows BRANCH_EN to pass through instead of being always on. See
+	 * also the comment in pt_event_valid().
+	 */
+	if (event->attr.config & BIT(0)) {
+		reg |= event->attr.config & RTIT_CTL_BRANCH_EN;
+	} else {
+		reg |= RTIT_CTL_BRANCH_EN;
+	}
 
 	if (!event->attr.exclude_kernel)
 		reg |= RTIT_CTL_OS;
diff --git a/arch/x86/events/intel/pt.h b/arch/x86/events/intel/pt.h
index 53473c21b5..93a3633e4c 100644
--- a/arch/x86/events/intel/pt.h
+++ b/arch/x86/events/intel/pt.h
@@ -110,6 +110,7 @@ struct pt_pmu {
 	struct pmu		pmu;
 	u32			caps[PT_CPUID_REGS_NUM * PT_CPUID_LEAVES];
 	bool			vmx;
+	bool			branch_en_always_on;
 	unsigned long		max_nonturbo_ratio;
 	unsigned int		tsc_art_num;
 	unsigned int		tsc_art_den;
-- 
2.11.0

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

#1574931 — Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> Now that Intel PT supports more types of trace content than just branch
> tracing, it may be useful to allow the user to disable branch tracing
> when it is not needed.
>
> The special case is BDW, where not setting BranchEn is not supported.
>
> This is slightly trickier than necessary, because up to this moment
> the driver has been setting BranchEn automatically and the userspace
> assumes as much. Instead of reversing the semantics of BranchEn, we
> introduce a 'passthrough' bit, which will forego the default and allow
> the user to set BranchEn to their heart's content.

cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
than the original cpu/nobranch=1/

Just think how you would explain it to the user in the manpage.

-Andi

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

#1574935 — Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

Andi Kleen <andi@firstfloor.org> writes:

> Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
>
>> Now that Intel PT supports more types of trace content than just branch
>> tracing, it may be useful to allow the user to disable branch tracing
>> when it is not needed.
>>
>> The special case is BDW, where not setting BranchEn is not supported.
>>
>> This is slightly trickier than necessary, because up to this moment
>> the driver has been setting BranchEn automatically and the userspace
>> assumes as much. Instead of reversing the semantics of BranchEn, we
>> introduce a 'passthrough' bit, which will forego the default and allow
>> the user to set BranchEn to their heart's content.
>
> cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
> than the original cpu/nobranch=1/

It's /passthrough=1,branch=0/ or simply /passthrough=1/.

Regards,
--
Alex

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

#1575013 — Re: [PATCH] perf/x86/intel/pt: Allow disabling branch tracing

On Mon, Feb 06, 2017 at 06:05:29PM +0200, Alexander Shishkin wrote:
> Andi Kleen <andi@firstfloor.org> writes:
> 
> > Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:
> >
> >> Now that Intel PT supports more types of trace content than just branch
> >> tracing, it may be useful to allow the user to disable branch tracing
> >> when it is not needed.
> >>
> >> The special case is BDW, where not setting BranchEn is not supported.
> >>
> >> This is slightly trickier than necessary, because up to this moment
> >> the driver has been setting BranchEn automatically and the userspace
> >> assumes as much. Instead of reversing the semantics of BranchEn, we
> >> introduce a 'passthrough' bit, which will forego the default and allow
> >> the user to set BranchEn to their heart's content.
> >
> > cpu/passthrough=1,branchen=1/ seems far uglier/more complicanted to me
> > than the original cpu/nobranch=1/
> 
> It's /passthrough=1,branch=0/ or simply /passthrough=1/.

Ok, but still you have to list exactly to which flags passthrough
applies to, and it will only ever be branchen.

So basically you turned nobranch=1 into two more difficult to
explain flags without any future advantage.

That is why nobranch=1 is better. It is far easier to explain
and logical to the user.

-Andi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web