lkml.org 
[lkml]   [2017]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2017-10-22 17:15    [W:1.648 / U:0.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site