Re: [PATCH 6/6] perf/x86/rapl: Add per-core energy counter support for AMD CPUs

[Zhang, Rui]

> @@ -345,9 +353,14 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>         int bit, ret = 0;
>         struct rapl_pmu *rapl_pmu;
> +       struct rapl_pmus *curr_rapl_pmus;
>  
>         /* only look at RAPL events */
> -       if (event->attr.type != rapl_pmus->pmu.type)
> +       if (event->attr.type == rapl_pmus->pmu.type)
> +               curr_rapl_pmus = rapl_pmus;
> +       else if (rapl_pmus_per_core && event->attr.type ==
> rapl_pmus_per_core->pmu.type)
> +               curr_rapl_pmus = rapl_pmus_per_core;
> +       else
>                 return -ENOENT;

can we use container_of(event->pmu, struct rapl_pmus, pmu)?

>  
>         /* check only supported bits are set */
> @@ -374,9 +387,14 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>                 return -EINVAL;
>  
>         /* must be done before validate_group */
> -       rapl_pmu = cpu_to_rapl_pmu(event->cpu);
> +       if (curr_rapl_pmus == rapl_pmus_per_core)
> +               rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[topology_core_id(event->cpu)];
> +       else
> +               rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[get_rapl_pmu_idx(event->cpu)];
> +
>         if (!rapl_pmu)
>                 return -EINVAL;

Current code has PERF_EV_CAP_READ_ACTIVE_PKG flag set.
Can you help me understand why it does not affect the new per-core pmu?

> +
>         event->cpu = rapl_pmu->cpu;
>         event->pmu_private = rapl_pmu;
>         event->hw.event_base = rapl_msrs[bit].msr;
> @@ -408,17 +426,38 @@ static struct attribute_group
> rapl_pmu_attr_group = {
>         .attrs = rapl_pmu_attrs,
>  };
>  
> +static ssize_t rapl_get_attr_per_core_cpumask(struct device *dev,
> +                                            struct device_attribute
> *attr, char *buf)
> +{
> +       return cpumap_print_to_pagebuf(true, buf,
> &rapl_pmus_per_core->cpumask);
> +}
> +
> +static struct device_attribute dev_attr_per_core_cpumask =
> __ATTR(cpumask, 0444,
> +                                                               
> rapl_get_attr_per_core_cpumask,
> +                                                               
> NULL);

DEVICE_ATTR

> +
> +static struct attribute *rapl_pmu_per_core_attrs[] = {
> +       &dev_attr_per_core_cpumask.attr,
> +       NULL,
> +};
> +
> +static struct attribute_group rapl_pmu_per_core_attr_group = {
> +       .attrs = rapl_pmu_per_core_attrs,
> +};
> +
>  RAPL_EVENT_ATTR_STR(energy-cores, rapl_cores, "event=0x01");
>  RAPL_EVENT_ATTR_STR(energy-pkg  ,   rapl_pkg, "event=0x02");
>  RAPL_EVENT_ATTR_STR(energy-ram  ,   rapl_ram, "event=0x03");
>  RAPL_EVENT_ATTR_STR(energy-gpu  ,   rapl_gpu, "event=0x04");
>  RAPL_EVENT_ATTR_STR(energy-psys,   rapl_psys, "event=0x05");
> +RAPL_EVENT_ATTR_STR(energy-per-core,   rapl_per_core, "event=0x06");

energy-per-core is for a separate pmu, so the event id does not need to
be 6. The same applies to PERF_RAPL_PERCORE.

>  
>  static struct rapl_model model_amd_hygon = {
> -       .events         = BIT(PERF_RAPL_PKG),
> +       .events         = BIT(PERF_RAPL_PKG) |
> +                         BIT(PERF_RAPL_PERCORE),
>         .msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
>         .rapl_msrs      = amd_rapl_msrs,
> +       .per_core = true,
>  };

can we use bit PERF_RAPL_PERCORE to check per_core pmu suppot?

Just FYI, arch/x86/events/intel/cstate.c handles package/module/core
scope cstate pmus. It uses a different approach in the probing part,
which IMO is clearer.

thanks,
rui