Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

On Thu, Aug 4, 2016 at 10:27 AM, Peter Zijlstra  wrote:
> On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
>> >> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
>> >> allows a PMU to signal the generic perf code that an event is readable
>> >> on the current CPU if the event is:
>> >>   - active in a CPU in the same package as the current CPU (local CPU)
>> >
>> > Ok that I get..
>> >
>> >>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>> >>   same package as the current CPU.
>> >
>> > but this, not so much. Why would you want to read an inactive counter?
>>
>> Uncore counters are active even if its event is not.
>
> Not in general they are not, and if they are (freerunning msr counters
> for example) we should not include the counts when the event is
> inactive.
>
True that. I conflated Intel CQM specific behavior with other uncore
(In CQM we want to read when the event is inactive). I'll fix that in
next version.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

On Thu, Aug 4, 2016 at 10:27 AM, Peter Zijlstra  wrote:
> On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
>> >> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
>> >> allows a PMU to signal the generic perf code that an event is readable
>> >> on the current CPU if the event is:
>> >>   - active in a CPU in the same package as the current CPU (local CPU)
>> >
>> > Ok that I get..
>> >
>> >>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>> >>   same package as the current CPU.
>> >
>> > but this, not so much. Why would you want to read an inactive counter?
>>
>> Uncore counters are active even if its event is not.
>
> Not in general they are not, and if they are (freerunning msr counters
> for example) we should not include the counts when the event is
> inactive.
>
True that. I conflated Intel CQM specific behavior with other uncore
(In CQM we want to read when the event is inactive). I'll fix that in
next version.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

>> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
>> allows a PMU to signal the generic perf code that an event is readable
>> on the current CPU if the event is:
>>   - active in a CPU in the same package as the current CPU (local CPU)
>
> Ok that I get..
>
>>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>>   same package as the current CPU.
>
> but this, not so much. Why would you want to read an inactive counter?

Uncore counters are active even if its event is not.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

>> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
>> allows a PMU to signal the generic perf code that an event is readable
>> on the current CPU if the event is:
>>   - active in a CPU in the same package as the current CPU (local CPU)
>
> Ok that I get..
>
>>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>>   same package as the current CPU.
>
> but this, not so much. Why would you want to read an inactive counter?

Uncore counters are active even if its event is not.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
> >> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
> >> allows a PMU to signal the generic perf code that an event is readable
> >> on the current CPU if the event is:
> >>   - active in a CPU in the same package as the current CPU (local CPU)
> >
> > Ok that I get..
> >
> >>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
> >>   same package as the current CPU.
> >
> > but this, not so much. Why would you want to read an inactive counter?
> 
> Uncore counters are active even if its event is not.

Not in general they are not, and if they are (freerunning msr counters
for example) we should not include the counts when the event is
inactive.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Thu, Aug 04, 2016 at 10:23:43AM -0700, David Carrillo-Cisneros wrote:
> >> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
> >> allows a PMU to signal the generic perf code that an event is readable
> >> on the current CPU if the event is:
> >>   - active in a CPU in the same package as the current CPU (local CPU)
> >
> > Ok that I get..
> >
> >>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
> >>   same package as the current CPU.
> >
> > but this, not so much. Why would you want to read an inactive counter?
> 
> Uncore counters are active even if its event is not.

Not in general they are not, and if they are (freerunning msr counters
for example) we should not include the counts when the event is
inactive.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

>>   struct list_headsb_list;
>> +
>> + /* Per-event flags to generic code set by PMU. */
>> + int pmu_event_flags;
>> +
>
> It appears to me we already have group_flags and attach_state which both
> are serialized by ctx->lock.
>
> Could we maybe merge the lot into one flags field?

Sounds good, I will do that.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[David Carrillo-Cisneros]

>>   struct list_headsb_list;
>> +
>> + /* Per-event flags to generic code set by PMU. */
>> + int pmu_event_flags;
>> +
>
> It appears to me we already have group_flags and attach_state which both
> are serialized by ctx->lock.
>
> Could we maybe merge the lot into one flags field?

Sounds good, I will do that.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +/*
> + * Flags for pmu_event_flags.
> + *
> + * PMUEF_READ_CPU_PKG: A CPU event (or cgroup event) that can be read in
> + * any CPU in event->cpu's package, even if inactive.
> + */
> +#define PMUEF_READ_CPU_PKG   BIT(0)

So I don't object to the idea, but this flags thing seems somewhat
awkward to express 'random' topology constraints.

For example, the AMD Fam15 NB driver could use something similar, but
I'm not sure their NB-id matches our topo package id.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +/*
> + * Flags for pmu_event_flags.
> + *
> + * PMUEF_READ_CPU_PKG: A CPU event (or cgroup event) that can be read in
> + * any CPU in event->cpu's package, even if inactive.
> + */
> +#define PMUEF_READ_CPU_PKG   BIT(0)

So I don't object to the idea, but this flags thing seems somewhat
awkward to express 'random' topology constraints.

For example, the AMD Fam15 NB driver could use something similar, but
I'm not sure their NB-id matches our topo package id.

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +++ b/include/linux/perf_event.h
> @@ -681,9 +681,21 @@ struct perf_event {
>  #endif
>  
>   struct list_headsb_list;
> +
> + /* Per-event flags to generic code set by PMU. */
> + int pmu_event_flags;
> +

It appears to me we already have group_flags and attach_state which both
are serialized by ctx->lock.

Could we maybe merge the lot into one flags field?

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> +++ b/include/linux/perf_event.h
> @@ -681,9 +681,21 @@ struct perf_event {
>  #endif
>  
>   struct list_headsb_list;
> +
> + /* Per-event flags to generic code set by PMU. */
> + int pmu_event_flags;
> +

It appears to me we already have group_flags and attach_state which both
are serialized by ctx->lock.

Could we maybe merge the lot into one flags field?

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
> allows a PMU to signal the generic perf code that an event is readable
> on the current CPU if the event is:
>   - active in a CPU in the same package as the current CPU (local CPU)

Ok that I get..

>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>   same package as the current CPU.

but this, not so much. Why would you want to read an inactive counter?

Re: [PATCH 2/3] perf/core: introduce pmu_event_flags and PMUEF_READ_CPU_PKG

[Peter Zijlstra]

On Mon, Aug 01, 2016 at 07:44:54PM -0700, David Carrillo-Cisneros wrote:
> Introduce the flag PMUEF_READ_CPU_PKG, useful for uncore events, that
> allows a PMU to signal the generic perf code that an event is readable
> on the current CPU if the event is:
>   - active in a CPU in the same package as the current CPU (local CPU)

Ok that I get..

>   - not active but is attached to a CPU (i.e. event->cpu != -1) in the
>   same package as the current CPU.

but this, not so much. Why would you want to read an inactive counter?