Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jin, Yao]

On 5/9/2017 8:39 PM, Jiri Olsa wrote:

On Tue, May 09, 2017 at 07:57:11PM +0800, Jin, Yao wrote:

SNIP


+
+   type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
+   mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?

you did not comment on this one

Sorry, I misunderstood that this comment and the next comment had the same
meaning.

In the previous version, I used the switch/case to convert from X86_BR to
PERF_BR. I got a comment from community that it'd better use a lookup table
for conversion.

Since each bit in type represents a X86_BR type so I use a mask (0x1) to
filter the bit. Yes, it looks I can also directly set 0x1 to mask.

I write the code "mask = ~(~0 << 1)" according to my coding habits. If you
think I should change the code to "mask = 0x1", that's OK  :)

im ok with that.. was just wondering for the reason
I guess compiler will make it single constant assignment anyway


I think so.  The compiler should be clever enough for this optimization.

+
+   for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
+   if (type & mask)
+   return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka

I just think the branch_map[] doesn't contain many entries (16 entries
here), so maybe checking 1 bit one time should be acceptable. I just want to
keep the code simple.

But if the number of entries is more (e.g. 64), maybe it'd better check 2 or
4 bits one time.

ook

jirka

Sorry, what's the meaning of ook? Does it mean "OK"?

just means ok ;-)

thanks,
jirka


Thanks so much!

Jin Yao

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Tue, May 09, 2017 at 07:57:11PM +0800, Jin, Yao wrote:

SNIP

> > > > > +
> > > > > + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> > > > > + mask = ~(~0 << 1);
> > > > is that a fancy way to get 1 into the mask? what do I miss?
> > you did not comment on this one
> 
> Sorry, I misunderstood that this comment and the next comment had the same
> meaning.
> 
> In the previous version, I used the switch/case to convert from X86_BR to
> PERF_BR. I got a comment from community that it'd better use a lookup table
> for conversion.
> 
> Since each bit in type represents a X86_BR type so I use a mask (0x1) to
> filter the bit. Yes, it looks I can also directly set 0x1 to mask.
> 
> I write the code "mask = ~(~0 << 1)" according to my coding habits. If you
> think I should change the code to "mask = 0x1", that's OK  :)

im ok with that.. was just wondering for the reason
I guess compiler will make it single constant assignment anyway

> 
> > > > > +
> > > > > + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> > > > > + if (type & mask)
> > > > > + return branch_map[i];
> > > > I wonder some bit search would be faster in here, but maybe not big deal
> > > > 
> > > > jirka
> > > I just think the branch_map[] doesn't contain many entries (16 entries
> > > here), so maybe checking 1 bit one time should be acceptable. I just want 
> > > to
> > > keep the code simple.
> > > 
> > > But if the number of entries is more (e.g. 64), maybe it'd better check 2 
> > > or
> > > 4 bits one time.
> > ook
> > 
> > jirka
> Sorry, what's the meaning of ook? Does it mean "OK"?

just means ok ;-)

thanks,
jirka

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jin, Yao]

On 5/9/2017 4:26 PM, Jiri Olsa wrote:

On Mon, Apr 24, 2017 at 08:47:14AM +0800, Jin, Yao wrote:


On 4/23/2017 9:55 PM, Jiri Olsa wrote:

On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:

SNIP


+#define X86_BR_TYPE_MAP_MAX16
+
+static int
+common_branch_type(int type)
+{
+   int i, mask;
+   const int branch_map[X86_BR_TYPE_MAP_MAX] = {
+   PERF_BR_CALL,   /* X86_BR_CALL */
+   PERF_BR_RET,/* X86_BR_RET */
+   PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
+   PERF_BR_SYSRET, /* X86_BR_SYSRET */
+   PERF_BR_INT,/* X86_BR_INT */
+   PERF_BR_IRET,   /* X86_BR_IRET */
+   PERF_BR_JCC,/* X86_BR_JCC */
+   PERF_BR_JMP,/* X86_BR_JMP */
+   PERF_BR_IRQ,/* X86_BR_IRQ */
+   PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
+   PERF_BR_NONE,   /* X86_BR_ABORT */
+   PERF_BR_NONE,   /* X86_BR_IN_TX */
+   PERF_BR_NONE,   /* X86_BR_NO_TX */
+   PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
+   PERF_BR_NONE,   /* X86_BR_CALL_STACK */
+   PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
+   };
+
+   type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
+   mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?

you did not comment on this one


Sorry, I misunderstood that this comment and the next comment had the 
same meaning.


In the previous version, I used the switch/case to convert from X86_BR 
to PERF_BR. I got a comment from community that it'd better use a lookup 
table for conversion.


Since each bit in type represents a X86_BR type so I use a mask (0x1) to 
filter the bit. Yes, it looks I can also directly set 0x1 to mask.


I write the code "mask = ~(~0 << 1)" according to my coding habits. If 
you think I should change the code to "mask = 0x1", that's OK  :)



+
+   for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
+   if (type & mask)
+   return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka

I just think the branch_map[] doesn't contain many entries (16 entries
here), so maybe checking 1 bit one time should be acceptable. I just want to
keep the code simple.

But if the number of entries is more (e.g. 64), maybe it'd better check 2 or
4 bits one time.

ook

jirka

Sorry, what's the meaning of ook? Does it mean "OK"?

Thanks
Jin Yao

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Mon, Apr 24, 2017 at 08:47:14AM +0800, Jin, Yao wrote:
> 
> 
> On 4/23/2017 9:55 PM, Jiri Olsa wrote:
> > On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:
> > 
> > SNIP
> > 
> > > +#define X86_BR_TYPE_MAP_MAX  16
> > > +
> > > +static int
> > > +common_branch_type(int type)
> > > +{
> > > + int i, mask;
> > > + const int branch_map[X86_BR_TYPE_MAP_MAX] = {
> > > + PERF_BR_CALL,   /* X86_BR_CALL */
> > > + PERF_BR_RET,/* X86_BR_RET */
> > > + PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
> > > + PERF_BR_SYSRET, /* X86_BR_SYSRET */
> > > + PERF_BR_INT,/* X86_BR_INT */
> > > + PERF_BR_IRET,   /* X86_BR_IRET */
> > > + PERF_BR_JCC,/* X86_BR_JCC */
> > > + PERF_BR_JMP,/* X86_BR_JMP */
> > > + PERF_BR_IRQ,/* X86_BR_IRQ */
> > > + PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
> > > + PERF_BR_NONE,   /* X86_BR_ABORT */
> > > + PERF_BR_NONE,   /* X86_BR_IN_TX */
> > > + PERF_BR_NONE,   /* X86_BR_NO_TX */
> > > + PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
> > > + PERF_BR_NONE,   /* X86_BR_CALL_STACK */
> > > + PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
> > > + };
> > > +
> > > + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> > > + mask = ~(~0 << 1);
> > is that a fancy way to get 1 into the mask? what do I miss?

you did not comment on this one

> > 
> > > +
> > > + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> > > + if (type & mask)
> > > + return branch_map[i];
> > I wonder some bit search would be faster in here, but maybe not big deal
> > 
> > jirka
> 
> I just think the branch_map[] doesn't contain many entries (16 entries
> here), so maybe checking 1 bit one time should be acceptable. I just want to
> keep the code simple.
> 
> But if the number of entries is more (e.g. 64), maybe it'd better check 2 or
> 4 bits one time.

ook

jirka

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jin, Yao]

On 4/24/2017 8:47 AM, Jin, Yao wrote:



On 4/23/2017 9:55 PM, Jiri Olsa wrote:

On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:

SNIP


  +#define X86_BR_TYPE_MAP_MAX16
+
+static int
+common_branch_type(int type)
+{
+int i, mask;
+const int branch_map[X86_BR_TYPE_MAP_MAX] = {
+PERF_BR_CALL,/* X86_BR_CALL */
+PERF_BR_RET,/* X86_BR_RET */
+PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
+PERF_BR_SYSRET,/* X86_BR_SYSRET */
+PERF_BR_INT,/* X86_BR_INT */
+PERF_BR_IRET,/* X86_BR_IRET */
+PERF_BR_JCC,/* X86_BR_JCC */
+PERF_BR_JMP,/* X86_BR_JMP */
+PERF_BR_IRQ,/* X86_BR_IRQ */
+PERF_BR_IND_CALL,/* X86_BR_IND_CALL */
+PERF_BR_NONE,/* X86_BR_ABORT */
+PERF_BR_NONE,/* X86_BR_IN_TX */
+PERF_BR_NONE,/* X86_BR_NO_TX */
+PERF_BR_CALL,/* X86_BR_ZERO_CALL */
+PERF_BR_NONE,/* X86_BR_CALL_STACK */
+PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
+};
+
+type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
+mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?


+
+for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
+if (type & mask)
+return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka


I just think the branch_map[] doesn't contain many entries (16 entries 
here), so maybe checking 1 bit one time should be acceptable. I just 
want to keep the code simple.


But if the number of entries is more (e.g. 64), maybe it'd better 
check 2 or 4 bits one time.


Thanks
Jin Yao



Hi,

Is this explanation OK? Since for tools part, it's Acked-by: Jiri Olsa. 
I just want to know  if the kernel part is OK either?


Thanks
Jin Yao

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jin, Yao]

On 4/23/2017 9:55 PM, Jiri Olsa wrote:

On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:

SNIP

  
+#define X86_BR_TYPE_MAP_MAX	16

+
+static int
+common_branch_type(int type)
+{
+   int i, mask;
+   const int branch_map[X86_BR_TYPE_MAP_MAX] = {
+   PERF_BR_CALL,   /* X86_BR_CALL */
+   PERF_BR_RET,/* X86_BR_RET */
+   PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
+   PERF_BR_SYSRET, /* X86_BR_SYSRET */
+   PERF_BR_INT,/* X86_BR_INT */
+   PERF_BR_IRET,   /* X86_BR_IRET */
+   PERF_BR_JCC,/* X86_BR_JCC */
+   PERF_BR_JMP,/* X86_BR_JMP */
+   PERF_BR_IRQ,/* X86_BR_IRQ */
+   PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
+   PERF_BR_NONE,   /* X86_BR_ABORT */
+   PERF_BR_NONE,   /* X86_BR_IN_TX */
+   PERF_BR_NONE,   /* X86_BR_NO_TX */
+   PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
+   PERF_BR_NONE,   /* X86_BR_CALL_STACK */
+   PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
+   };
+
+   type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
+   mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?


+
+   for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
+   if (type & mask)
+   return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka


I just think the branch_map[] doesn't contain many entries (16 entries 
here), so maybe checking 1 bit one time should be acceptable. I just 
want to keep the code simple.


But if the number of entries is more (e.g. 64), maybe it'd better check 
2 or 4 bits one time.


Thanks
Jin Yao




+
+   type >>= 1;
+   }
+
+   return PERF_BR_NONE;
+}
+
  /*

Re: [PATCH v6 2/7] perf/x86/intel: Record branch type

[Jiri Olsa]

On Thu, Apr 20, 2017 at 08:07:50PM +0800, Jin Yao wrote:

SNIP

>  
> +#define X86_BR_TYPE_MAP_MAX  16
> +
> +static int
> +common_branch_type(int type)
> +{
> + int i, mask;
> + const int branch_map[X86_BR_TYPE_MAP_MAX] = {
> + PERF_BR_CALL,   /* X86_BR_CALL */
> + PERF_BR_RET,/* X86_BR_RET */
> + PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
> + PERF_BR_SYSRET, /* X86_BR_SYSRET */
> + PERF_BR_INT,/* X86_BR_INT */
> + PERF_BR_IRET,   /* X86_BR_IRET */
> + PERF_BR_JCC,/* X86_BR_JCC */
> + PERF_BR_JMP,/* X86_BR_JMP */
> + PERF_BR_IRQ,/* X86_BR_IRQ */
> + PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
> + PERF_BR_NONE,   /* X86_BR_ABORT */
> + PERF_BR_NONE,   /* X86_BR_IN_TX */
> + PERF_BR_NONE,   /* X86_BR_NO_TX */
> + PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
> + PERF_BR_NONE,   /* X86_BR_CALL_STACK */
> + PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
> + };
> +
> + type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
> + mask = ~(~0 << 1);

is that a fancy way to get 1 into the mask? what do I miss?

> +
> + for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
> + if (type & mask)
> + return branch_map[i];

I wonder some bit search would be faster in here, but maybe not big deal

jirka

> +
> + type >>= 1;
> + }
> +
> + return PERF_BR_NONE;
> +}
> +
>  /*

[PATCH v6 2/7] perf/x86/intel: Record branch type

[Jin Yao]

Perf already has support for disassembling the branch instruction
and using the branch type for filtering. The patch just records
the branch type in perf_branch_entry.

Before recording, the patch converts the x86 branch type to
common branch type.

Change log
--

v6: Not changed.

v5: Just fix the merge error. No other update.

v4: Comparing to previous version, the major changes are:

1. Uses a lookup table to convert x86 branch type to common branch
   type.

2. Move the JCC forward/JCC backward and cross page computing to
   user space.

3. Initialize branch type to 0 in intel_pmu_lbr_read_32 and
   intel_pmu_lbr_read_64

Signed-off-by: Jin Yao 
---
 arch/x86/events/intel/lbr.c | 53 -
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index f924629..f10a7ed 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -109,6 +109,9 @@ enum {
X86_BR_ZERO_CALL= 1 << 15,/* zero length call */
X86_BR_CALL_STACK   = 1 << 16,/* call stack */
X86_BR_IND_JMP  = 1 << 17,/* indirect jump */
+
+   X86_BR_TYPE_SAVE= 1 << 18,/* indicate to save branch type */
+
 };
 
 #define X86_BR_PLM (X86_BR_USER | X86_BR_KERNEL)
@@ -510,6 +513,7 @@ static void intel_pmu_lbr_read_32(struct cpu_hw_events 
*cpuc)
cpuc->lbr_entries[i].in_tx  = 0;
cpuc->lbr_entries[i].abort  = 0;
cpuc->lbr_entries[i].cycles = 0;
+   cpuc->lbr_entries[i].type   = 0;
cpuc->lbr_entries[i].reserved   = 0;
}
cpuc->lbr_stack.nr = i;
@@ -596,6 +600,7 @@ static void intel_pmu_lbr_read_64(struct cpu_hw_events 
*cpuc)
cpuc->lbr_entries[out].in_tx = in_tx;
cpuc->lbr_entries[out].abort = abort;
cpuc->lbr_entries[out].cycles= cycles;
+   cpuc->lbr_entries[out].type  = 0;
cpuc->lbr_entries[out].reserved  = 0;
out++;
}
@@ -673,6 +678,10 @@ static int intel_pmu_setup_sw_lbr_filter(struct perf_event 
*event)
 
if (br_type & PERF_SAMPLE_BRANCH_CALL)
mask |= X86_BR_CALL | X86_BR_ZERO_CALL;
+
+   if (br_type & PERF_SAMPLE_BRANCH_TYPE_SAVE)
+   mask |= X86_BR_TYPE_SAVE;
+
/*
 * stash actual user request into reg, it may
 * be used by fixup code for some CPU
@@ -926,6 +935,44 @@ static int branch_type(unsigned long from, unsigned long 
to, int abort)
return ret;
 }
 
+#define X86_BR_TYPE_MAP_MAX16
+
+static int
+common_branch_type(int type)
+{
+   int i, mask;
+   const int branch_map[X86_BR_TYPE_MAP_MAX] = {
+   PERF_BR_CALL,   /* X86_BR_CALL */
+   PERF_BR_RET,/* X86_BR_RET */
+   PERF_BR_SYSCALL,/* X86_BR_SYSCALL */
+   PERF_BR_SYSRET, /* X86_BR_SYSRET */
+   PERF_BR_INT,/* X86_BR_INT */
+   PERF_BR_IRET,   /* X86_BR_IRET */
+   PERF_BR_JCC,/* X86_BR_JCC */
+   PERF_BR_JMP,/* X86_BR_JMP */
+   PERF_BR_IRQ,/* X86_BR_IRQ */
+   PERF_BR_IND_CALL,   /* X86_BR_IND_CALL */
+   PERF_BR_NONE,   /* X86_BR_ABORT */
+   PERF_BR_NONE,   /* X86_BR_IN_TX */
+   PERF_BR_NONE,   /* X86_BR_NO_TX */
+   PERF_BR_CALL,   /* X86_BR_ZERO_CALL */
+   PERF_BR_NONE,   /* X86_BR_CALL_STACK */
+   PERF_BR_IND_JMP,/* X86_BR_IND_JMP */
+   };
+
+   type >>= 2; /* skip X86_BR_USER and X86_BR_KERNEL */
+   mask = ~(~0 << 1);
+
+   for (i = 0; i < X86_BR_TYPE_MAP_MAX; i++) {
+   if (type & mask)
+   return branch_map[i];
+
+   type >>= 1;
+   }
+
+   return PERF_BR_NONE;
+}
+
 /*
  * implement actual branch filter based on user demand.
  * Hardware may not exactly satisfy that request, thus
@@ -942,7 +989,8 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
bool compress = false;
 
/* if sampling all branches, then nothing to filter */
-   if ((br_sel & X86_BR_ALL) == X86_BR_ALL)
+   if (((br_sel & X86_BR_ALL) == X86_BR_ALL) &&
+   ((br_sel & X86_BR_TYPE_SAVE) != X86_BR_TYPE_SAVE))
return;
 
for (i = 0; i < cpuc->lbr_stack.nr; i++) {
@@ -963,6 +1011,9 @@ intel_pmu_lbr_filter(struct cpu_hw_events *cpuc)
cpuc->lbr_entries[i].from = 0;
compress = true;
}
+
+   if ((br_sel & X86_BR_TYPE_SAVE) == X86_BR_TYPE_SAVE)
+   cpuc->lbr_entries[i].type = common_branch_type(type);
}
 
if (!compress)
-- 
2.7.4