Re: [Xen-devel] [PATCH v2 2/2] x86/VPMU: implement ipc and arch filter flags

[Brendan Gregg <bgregg@xxxxxxxxxxx>]

On Wed, Nov 25, 2015 at 7:13 AM, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:

On 11/24/2015 06:53 PM, Brendan Gregg wrote:

This introduces a way to have a restricted VPMU, by specifying one of two
predefined groups of PMCs to make available. For secure environments, this
allows the VPMU to be used without needing to enable all PMCs.

Signed-off-by: Brendan Gregg <bgregg@xxxxxxxxxxx>
---
 docs/misc/xen-command-line.markdown | 14 +++++++++-
 xen/arch/x86/cpu/vpmu.c      Â| 51 +++++++++++++++++++++++++++++--------
 xen/arch/x86/cpu/vpmu_intel.c   Â| 48 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/msr-index.h  Â| 1 +
 xen/include/public/pmu.h      | 14 ++++++++--
 5 files changed, 115 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 70daa84..6055a68 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1452,7 +1452,7 @@ Use Virtual Processor ID support if available. This prevents the need for TLB
 flushes on VM entry and exit, increasing performance.
  ### vpmu
-> `= ( bts )`
+> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
  > Default: `off`
 @@ -1468,6 +1468,18 @@ wrong behaviour (see handle\_pmc\_quirk()).
 If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
 feature is switched on on Intel processors supporting this feature.
 +vpmu=ipc enables performance monitoring, but restricts the counters to the
+most minimum set possible: instructions, cycles, and reference cycles. These
+can be used to calculate instructions per cycle (IPC).
+
+vpmu=arch enables performance monitoring, but restricts the counters to the
+pre-defined architectural events only. These are exposed by cpuid, and listed
+in Table 18-1 from the Intel 64 and IA-32 Architectures Software Developer's
+Manual, Volume 3B, System Programming Guide, Part 2.
+
+If a boolean is not used, combinations of flags are allowed, comma separated.
+For example, vpmu=arch,bts.
+
 Note that if **watchdog** option is also specified vpmu will be turned off.
  *Warning:*
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 2f5156a..bb0ca37 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -43,33 +43,64 @@ CHECK_pmu_data;
 CHECK_pmu_params;
  /*
- * "vpmu" :Â Â Âvpmu generally enabled
- * "vpmu=off" : vpmu generally disabled
- * "vpmu=bts" : vpmu enabled and Intel BTS feature switched on.
+ * "vpmu" :Â Â Âvpmu generally enabled (all counters)
+ * "vpmu=off"Â : vpmu generally disabled
+ * "vpmu=bts"Â : vpmu enabled and Intel BTS feature switched on.
+ * "vpmu=ipc"Â : vpmu enabled for IPC counters only (most restrictive)
+ * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
+ * flag combinations are allowed, eg, "vpmu=ipc,bts".
 Â*/
 static unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
-static void parse_vpmu_param(char *s);
-custom_param("vpmu", parse_vpmu_param);
+static void parse_vpmu_params(char *s);
+custom_param("vpmu", parse_vpmu_params);
  static DEFINE_SPINLOCK(vpmu_lock);
 static unsigned vpmu_count;
  static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
 -static void __init parse_vpmu_param(char *s)
+static int parse_vpmu_param(char *s, int len)
 {
+Â Â if ( ! *s || ! len )
+Â Â Â Â return 0;
+Â Â if ( !strncmp(s, "bts", len) )
+Â Â Â Â vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
+Â Â else if ( !strncmp(s, "ipc", len) )
+Â Â Â Â vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
+Â Â else if ( !strncmp(s, "arch", len) )
+Â Â Â Â vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
+Â Â else if ( *s )

Why not just "else return 1;" ? We've already tested above that *s is not '\0'. (And you don't need curly braces for single-line clauses)

Ok, thanks.

+Â Â {
+Â Â Â Â return 1;
+Â Â }
+Â Â return 0;
+}
+
+static void __init parse_vpmu_params(char *s)
+{
+Â Â bool_t badflag = 0;
+Â Â char *sep, *p = s;
+
   switch ( parse_bool(s) )
   {
   case 0:
     break;
   default:
-Â Â Â Â if ( !strcmp(s, "bts") )
-Â Â Â Â Â Â vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
-Â Â Â Â else if ( *s )
+Â Â Â Â sep = strchr(p, ',');
+Â Â Â Â while (sep != NULL)
+Â Â Â Â {
+Â Â Â Â Â Â if ( parse_vpmu_param(p, sep - p) )
+Â Â Â Â Â Â Â Â badflag = 1;
+Â Â Â Â Â Â p = sep + 1;
+Â Â Â Â Â Â sep = strchr(p, ',');
+Â Â Â Â }
+Â Â Â Â sep = strchr(p, 0);
+Â Â Â Â parse_vpmu_param(p, sep - p);

This can find unsupported flag too but we are not setting badflag so we will miss it (i.e. "vpmu=foo").

Can you just say something like
  sep = strchr(p, ',')
  if ( sep == NULL )
    sep = strchr(p, 0);

and keep both parse_vpmu_param() invocations in a single loop? And then, instead of having badflags simply print the warning and break.

Ah, thanks, I'll tidy it up. I still need to do something to avoid the fall through and enabling the VPMU, so either use of a badflags, or a goto error, or something.

+Â Â Â Â if ( badflag )
     {
-Â Â Â Â Â Â printk("VPMU: unknown flag: %s - vpmu disabled!\n", s);
+Â Â Â Â Â Â printk("VPMU: unknown flags: %s - vpmu disabled!\n", s);
       break;
     }
     /* fall through */
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 8d83a1a..b26a20b 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -602,12 +602,19 @@ static int core2_vpmu_do_wrmsr(unsigned int msr, uint64_t msr_content,
         Â"MSR_PERF_GLOBAL_STATUS(0x38E)!\n");
     return -EINVAL;
   case MSR_IA32_PEBS_ENABLE:
+Â Â Â Â if ( vpmu_features & XENPMU_FEATURE_IPC_ONLY ||
+Â Â Â Â Â Â Âvpmu_features & XENPMU_FEATURE_ARCH_ONLY )

It's is a matter of taste but you could also use
  vpmu_features & (XENPMU_FEATURE_IPC_ONLY | XENPMU_FEATURE_ARCH_ONLY)

Ok, thanks!

Brendan

Â

-boris

--

Brendan Gregg, Senior Performance Architect, Netflix

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel