Re: [PATCH] drm/i915/pmu: Only enumerate available counters in sysfs

Tvrtko Ursulin
2017-12-22T17:14:13Z

On 22/12/2017 14:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-12-21 17:13:16)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Switch over to dynamically creating device attributes, which are in turn
>> used by the perf core to expose available counters in sysfs.
>>
>> This way we do not expose counters which are not avaiable on the current
>> platform, and are so more consistent between what we reply to open
>> attempts via the perf_event_open(2), and what is discoverable in sysfs.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>> +#define __event(__config, __name, __unit) \
>> +{ \
>> + .config = (__config), \
>> + .name = (__name), \
>> + .unit = (__unit), \
>> +}
>> +
>> +#define __engine_event(__sample, __name) \
>> +{ \
>> + .sample = (__sample), \
>> + .name = (__name), \
>> +}
>> +
>> +#define __i915_attr(__p, __name, __config) \
>> +{ \
>> + (__p)->attr.attr.name = (__name); \
>> + (__p)->attr.attr.mode = 0444; \
>> + (__p)->attr.show = i915_pmu_event_show; \
>> + (__p)->val = (__config); \
>> +}
>> +
>> +#define __pmu_attr(__p, __name, __str) \
>> +{ \
>> + (__p)->attr.attr.name = (__name); \
>> + (__p)->attr.attr.mode = 0444; \
>> + (__p)->attr.show = perf_event_sysfs_show; \
>> + (__p)->event_str = (__str); \
>> +}
>> +
>> +static struct attribute **
>> +create_event_attributes(struct drm_i915_private *i915)
>> +{
>> + static const struct {
>> + u64 config;
>> + const char *name;
>> + const char *unit;
>> + } events[] = {
>> + __event(I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "MHz"),
>> + __event(I915_PMU_REQUESTED_FREQUENCY, "requested-frequency", "MHz"),
>> + __event(I915_PMU_INTERRUPTS, "interrupts", NULL),
>> + __event(I915_PMU_RC6_RESIDENCY, "rc6-residency", "ns"),
>> + };
>> + static const struct {
>> + enum drm_i915_pmu_engine_sample sample;
>> + char *name;
>> + } engine_events[] = {
>> + __engine_event(I915_SAMPLE_BUSY, "busy"),
>> + __engine_event(I915_SAMPLE_SEMA, "sema"),
>> + __engine_event(I915_SAMPLE_WAIT, "wait"),
>
> Are these macros that useful? vs { I915_SAMPLE_BUSY, "busy" } and
> { I915_PMU_ACTUAL_FREQUENCY, "actual-frequency", "MHz" },

They are marginal.. I preferred to used named initialization to be more
mistake proof, in which case they do shorten the lines here a bit. It's
50-50 for me, or don't really care.

>> + };
>> + unsigned int count = 0;
>> + struct perf_pmu_events_attr *pmu_attr, *pmu_p;
>> + struct i915_ext_attribute *i915_attr, *i915_p;
>> + struct intel_engine_cs *engine;
>> + struct attribute **attr, **p;
>> + enum intel_engine_id id;
>> + unsigned int i;
>> +
>> + /* Count how many counters we will be exposing. */
>> + for (i = 0; i < ARRAY_SIZE(events); i++) {
>> + if (!config_status(i915, events[i].config))
>> + count++;
>> + }
>> +
>> + for_each_engine(engine, i915, id) {
>> + for (i = 0; i < ARRAY_SIZE(engine_events); i++) {
>> + if (!engine_event_status(engine,
>> + engine_events[i].sample))
>> + count++;
>> + }
>> + }
>> +
>> + /* Allocate attribute objects and table. */
>> + i915_attr = kzalloc(count * sizeof(*i915_attr), GFP_KERNEL);
>> + if (!i915_attr)
>> + return NULL;
>> +
>> + pmu_attr = kzalloc(count * sizeof(*pmu_attr), GFP_KERNEL);
>> + if (!pmu_attr) {
>> + kfree(i915_attr);
>> + return NULL;
>> + }
>> +
>> + /* Max one pointer of each attribute type plus a termination entry. */
>> + attr = kzalloc((count * 2 + 1) * sizeof(attr), GFP_KERNEL);
>> + if (!attr) {
>> + kfree(pmu_attr);
>> + kfree(i915_attr);
>> + return NULL;
>
> Joonas wants you to feed through the same error unwind.

Yeah, I've almost done it at one point. :)

>> + }
>> +
>> + i915_p = i915_attr;
>> + pmu_p = pmu_attr;
>> + p = attr;
>
> i915_attr_iter, pmu_attr_iter and attr_iter?

Shrug, can do.

>
>> + /* Initialize supported non-engine counters. */
>> + for (i = 0; i < ARRAY_SIZE(events); i++) {
>> + char *str;
>> +
>> + if (config_status(i915, events[i].config))
>> + continue;
>> +
>> + str = kstrdup(events[i].name, GFP_KERNEL);
>> + if (!str)
>> + goto err;
>> +
>> + __i915_attr(i915_p, str, events[i].config);
>
> *attr_iter++ = &i915_attr_iter->attr.attr;
> *i915_attr_iter++ = (struct i915_ext_attribute) {
> .attr.attr.name = str,
> .attr.attr.mode = 0444,
> .attr.show = i915_pmu_event_show,
> .val = events[i].config,
> }
>
> ?

Partially prettier but since I had two instances of each attr
initialization I liked how the macro shrank the code. Perhaps I make the
macro increment the pointer and return it? Or make it a static inline even.

i915_attr_iter = __add_i915_attr(i915_attr_iter, str, ...);

?

> Mere suggestions. Doesn't look that bad

Thanks for looking. I think the largest benefit is future proofing the
maintenance. Secondary, but also attractive, that the unsupported events
do not list.

Regards,

Tvrtko