Re: [PATCH 2/2] perf/x86/intel/uncore: support IIO freerunning counter for SKX

[Thomas Gleixner]

On Mon, 11 Sep 2017, kan.liang@xxxxxxxxx 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