Messages in this thread Patch in this message | | | Date | Wed, 18 Oct 2017 17:29:05 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/2] perf/x86/intel/uncore: support IIO freerunning counter for SKX |
| |
On Mon, 11 Sep 2017, kan.liang@intel.com wrote: > - if (event->hw.idx >= UNCORE_PMC_IDX_FIXED) > + if (event->hw.idx >= UNCORE_PMC_IDX_FREERUNNING) > + shift = 64 - uncore_free_running_bits(box, event); > + else if (event->hw.idx == UNCORE_PMC_IDX_FIXED)
I have no idea why the original check was
event->hw.idx >= UNCORE_PMC_IDX_FIXED
and why the new one is
event->hw.idx >= UNCORE_PMC_IDX_FREERUNNING
when you check in the next conditional:
> + if (event->hw.idx == UNCORE_PMC_IDX_FREERUNNING) > + continue;
This stinks. Anything > UNCORE_PMC_IDX_FREERUNNING is bogus.
> @@ -454,6 +459,12 @@ static void uncore_pmu_event_start(struct perf_event *event, int flags) > struct intel_uncore_box *box = uncore_event_to_box(event); > int idx = event->hw.idx; > > + if (event->hw.idx == UNCORE_PMC_IDX_FREERUNNING) { > + local64_set(&event->hw.prev_count, > + uncore_read_counter(box, event));
A comment why this has special treatment would be helpful. Here and in other places.
> +/* > + * Free running MSR events have the same event code 0xff as fixed events. > + * The Free running events umask starts from 0x10. > + * The umask which is less than 0x10 is reserved for fixed events. > + * > + * The Free running events are divided into different types according to > + * MSR location, bit width or definition. Each type is limited to only have > + * at most 16 events. > + * So the umask of first type starts from 0x10, the second starts from 0x20, > + * the rest can be done in the same manner. > + */ > +#define UNCORE_FREE_RUNNING_MSR_START 0x10
> +#define UNCORE_FREE_RUNNING_MSR_IDX(config) ((config >> 8) & 0xf) > +#define UNCORE_FREE_RUNNING_MSR_TYPE_IDX(config) \ > + ((((config >> 8) - UNCORE_FREE_RUNNING_MSR_START) >> 4) & 0xf) > +
Any reason why these two need to be fugly macros instead of inlines which simply take an event or an attribute pointer?
> @@ -34,6 +52,7 @@ struct intel_uncore_ops; > struct intel_uncore_pmu; > struct intel_uncore_box; > struct uncore_event_desc; > +struct free_running_msr; > > struct intel_uncore_type { > const char *name; > @@ -41,6 +60,7 @@ struct intel_uncore_type { > int num_boxes; > int perf_ctr_bits; > int fixed_ctr_bits; > + int num_free_running_type;
s/type/types/
> @@ -128,6 +149,13 @@ struct uncore_event_desc { > const char *config; > }; > > +struct free_running_msr { > + unsigned msr_base;
unsigned int please
> + unsigned msr_off; > + unsigned num_counters; > + unsigned bits; > +}; > + > struct pci2phy_map { > struct list_head list; > int segment; > @@ -214,6 +242,18 @@ static inline unsigned uncore_msr_fixed_ctr(struct intel_uncore_box *box) > }
> +enum perf_uncore_iio_free_running_msr_type_id { > + SKX_IIO_MSR_IOCLK = 0, > + SKX_IIO_MSR_BW = 1, > + SKX_IIO_MSR_UTIL = 2, > + > + SKX_IIO_FREE_RUNNING_MSR_TYPE_MAX, > +};
Please split the patch in two parts:
1) Add infrastructure for free running counters
2) Add SKX support Thanks,
tglx
| |