Re: [PATCH v3] perf/x86/amd/power: Add AMD accumulated power reporting mechanism

[Ingo Molnar]

* Huang Rui <ray.huang@xxxxxxx> wrote:

> +/*
> + * Acc power status counters
> + */
> +#define AMD_POWER_PKG_ID		0
> +#define AMD_POWER_EVENTSEL_PKG		1

> +/*
> + * the ratio of compute unit power accumulator sample period to the
> + * PTSC period
> + */

> +/*
> + * Accumulated power is to measure the sum of each compute unit's
> + * power consumption. So it picks only one core from each compute unit
> + * to get the power with MSR_F15H_CU_PWR_ACCUMULATOR. The cpu_mask
> + * represents CPU bit map of all cores which are picked to measure the
> + * power for the compute units that they belong to.
> + */
> +static cpumask_t cpu_mask;

> +	/*
> +	 * calculate the power comsumption for each compute unit over
> +	 * a time period, the unit of final value (delta) is
> +	 * micro-Watts. Then add it into event count.
> +	 */

Please capitalize sentences consistently - half of the comments you added start 
lower-case.

> +static void __pmu_event_start(struct power_pmu *pmu,
> +			      struct perf_event *event)

So this looks better either on a single line, or as:

static void
__pmu_event_start(struct power_pmu *pmu, struct perf_event *event)

Breaking the argument list combines the worst of the two worlds.

> +	if ((mode & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> +		/*
> +		 * Drain the remaining delta count out of a event
> +		 * that we are disabling:

s/an event

(Please re-read all the comments - I saw a few other typos and spelling mistakes 
as well.)

> +	if (cfg == AMD_POWER_EVENTSEL_PKG)
> +		bit = AMD_POWER_PKG_ID;
> +	else
> +		return -EINVAL;
> +
> +	event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR;
> +	event->hw.config = cfg;
> +	event->hw.idx = bit;
> +
> +	return ret;

so this control flow looks pretty weird. Why not:

> +	if (cfg != AMD_POWER_EVENTSEL_PKG)
> +		return -EINVAL;
> +
> +	event->hw.event_base = MSR_F15H_CU_PWR_ACCUMULATOR;
> +	event->hw.config = cfg;
> +	event->hw.idx = AMD_POWER_PKG_ID;
> +
> +	return ret;

?

> +static struct attribute_group pmu_attr_group = {
> +	.attrs = pmu_attrs,
> +};
> +
> +
> +/*
> + * at current, it only supports to report the power of each

s/currently it only supports power reporting of each

> + * processor/package
> + */
> +EVENT_ATTR_STR(power-pkg, power_pkg, "event=0x01");
> +
> +EVENT_ATTR_STR(power-pkg.unit, power_pkg_unit, "mWatts");
> +
> +/* convert the count from micro-Watts to milli-Watts */
> +EVENT_ATTR_STR(power-pkg.scale, power_pkg_scale, "1.000000e-3");
> +
> +
> +static struct attribute *events_attr[] = {
> +	EVENT_PTR(power_pkg),
> +	EVENT_PTR(power_pkg_unit),
> +	EVENT_PTR(power_pkg_scale),
> +	NULL,
> +};
> +
> +static struct attribute_group pmu_events_group = {
> +	.name = "events",
> +	.attrs = events_attr,

> +static struct attribute_group pmu_format_group = {
> +	.name = "format",
> +	.attrs = formats_attr,
> +};

Please initialize structures in a vertically aligned manner, like you did it later 
on:

> +static struct pmu pmu_class = {
> +	.attr_groups	= attr_groups,
> +	.task_ctx_nr	= perf_invalid_context, /* system-wide only */
> +	.event_init	= pmu_event_init,
> +	.add		= pmu_event_add, /* must have */
> +	.del		= pmu_event_del, /* must have */
> +	.start		= pmu_event_start,
> +	.stop		= pmu_event_stop,
> +	.read		= pmu_event_read,
> +};

Btw., I don't think the 'must have' comment adds any important information needed 
to understand this new PMU driver.

> +static int power_cpu_init(int cpu)
> +{
> +	struct power_pmu *pmu = per_cpu(amd_power_pmu, cpu);
> +
> +	if (pmu)
> +		return 0;
> +
> +	if (!cpumask_and(pmu->mask, topology_sibling_cpumask(cpu),
> +			 &cpu_mask))
> +		cpumask_set_cpu(cpu, &cpu_mask);
> +
> +	return 0;
> +}

Hm, has this function ever been runtime tested? This function either does nothing 
(contrary to the clear intention of twiddling the cpu_mask), or crashes on a NULL 
pointer.

( Also, the code has an annoying line-break. Don't pacify checkpatch by making the 
  code harder to read. )

Thanks,

	Ingo