Messages in this thread | | | Date | Fri, 17 Apr 2020 12:30:16 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v9 03/10] perf/x86: Add constraint to create guest LBR event without hw counter |
| |
On Fri, Apr 10, 2020 at 11:03:33AM +0800, Xu, Like wrote: > On 2020/4/10 0:37, Peter Zijlstra wrote:
> > That should sort the branches in order of: gp,fixed,bts,vlbr > > Note the current order is: bts, pebs, fixed, gp.
Yeah, and that means that gp (which is I think the most common case) is the most expensive.
> Sure, let me try to refactor it in this way.
Thanks!
> > > +static inline bool is_guest_event(struct perf_event *event) > > > +{ > > > + if (event->attr.exclude_host && is_kernel_event(event)) > > > + return true; > > > + return false; > > > +} > > I don't like this one, what if another in-kernel users generates an > > event with exclude_host set ? > Thanks for the clear attitude. > > How about: > - remove the is_guest_event() to avoid potential misuse; > - move all checks into is_guest_lbr_event() and make it dedicated: > > static inline bool is_guest_lbr_event(struct perf_event *event) > { > if (is_kernel_event(event) && > event->attr.exclude_host && needs_branch_stack(event)) > return true; > return false; > } > > In this case, it's safe to generate an event with exclude_host set > and also use LBR to count guest or nothing for other in-kernel users > because the intel_guest_lbr_event_constraints() makes LBR exclusive. > > For this generic usage, I may rename: > - is_guest_lbr_event() to is_lbr_no_counter_event(); > - intel_guest_lbr_event_constraints() to > intel_lbr_no_counter_event_constraints(); > > Is this acceptable to you?
I suppose so, please put a comment on top of that function though, so we don't forget.
> > > @@ -181,9 +181,19 @@ struct x86_pmu_capability { > > > #define GLOBAL_STATUS_UNC_OVF BIT_ULL(61) > > > #define GLOBAL_STATUS_ASIF BIT_ULL(60) > > > #define GLOBAL_STATUS_COUNTERS_FROZEN BIT_ULL(59) > > > -#define GLOBAL_STATUS_LBRS_FROZEN BIT_ULL(58) > > > +#define GLOBAL_STATUS_LBRS_FROZEN_BIT 58 > > > +#define GLOBAL_STATUS_LBRS_FROZEN BIT_ULL(GLOBAL_STATUS_LBRS_FROZEN_BIT) > > > #define GLOBAL_STATUS_TRACE_TOPAPMI BIT_ULL(55) > > > +/* > > > + * We model guest LBR event tracing as another fixed-mode PMC like BTS. > > > + * > > > + * We choose bit 58 (LBRS_FROZEN_BIT) which is used to indicate that the LBR > > > + * stack is frozen on a hardware PMI request in the PERF_GLOBAL_STATUS msr, > > > + * and the 59th PMC counter (if any) is not supposed to use it as well. > > Is this saying that STATUS.58 should never be set? I don't really > > understand the language. > My fault, and let me make it more clearly: > > We choose bit 58 because it's used to indicate LBR stack frozen state > not like other overflow conditions in the PERF_GLOBAL_STATUS msr, > and it will not be used for any actual fixed events.
That's only with v4, also we unconditionally mask that bit in handle_pmi_common(), so it'll never be set in the overflow handling.
That's all fine, I suppose, all you want is means of programming the LBR registers, we don't actually do anything with then in the host context. Please write a more elaborate comment here.
| |