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

["Zhang, Rui" <rui.zhang@intel.com>]

> 
> @@ -352,9 +384,13 @@ 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_pkg->pmu.type)
> +       if (event->attr.type == rapl_pmus_pkg->pmu.type ||
> +               (rapl_pmus_core && event->attr.type ==
> rapl_pmus_core->pmu.type))
> +               curr_rapl_pmus = container_of(event->pmu, struct
> rapl_pmus, pmu);
> +       else
>                 return -ENOENT;
>  
>         /* check only supported bits are set */
> @@ -364,7 +400,8 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         if (event->cpu < 0)
>                 return -EINVAL;
>  
> -       event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
> +       if (curr_rapl_pmus == rapl_pmus_pkg)
> +               event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
>  
>         if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>                 return -EINVAL;

this sanity check becomes bogus for per_core event.

> @@ -373,7 +410,8 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
>         bit = cfg - 1;
>  
>         /* check event supported */
> -       if (!(rapl_pkg_cntr_mask & (1 << bit)))
> +       if (!(rapl_pkg_cntr_mask & (1 << bit)) &&
> +           !(rapl_core_cntr_mask & (1 << bit)))
>                 return -EINVAL;

what if bit > 1 for a per_core event?

>  
>         /* unsupported modes and filters */
> @@ -381,12 +419,18 @@ 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_core) {
> +               rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[topology_logical_core_id(event->cpu)];
> +               event->hw.event_base = rapl_model-
> >rapl_core_msrs[bit].msr;
> +       } else {
> +               rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[get_rapl_pmu_idx(event->cpu)];
> +               event->hw.event_base = rapl_model-
> >rapl_pkg_msrs[bit].msr;
> +       }
> +

To avoid the above issues and check for (curr_rapl_pmus ==
rapl_pmus_core) all over the places, I'd suggest we do the
per_core/per_pkg sanity checks and handlings altogether, say something
like

if (event->attr.type == rapl_pmus_pkg->pmu.type) {
	all sanity checks
	rapl_pmu = ...
	event->hw.event_base = ...
} else if (rapl_pmus_core && event->attr.type ==
rapl_pmus_core->pmu.type) {
	all sanity checks
	rapl_pmu = ...
	event->hw.event_base = ...
} else {
	return --ENOENT;
}

[...]

>  static int rapl_cpu_offline(unsigned int cpu)
>  {
> -       return __rapl_cpu_offline(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> +       int ret =  __rapl_cpu_offline(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
>                                   get_rapl_pmu_cpumask(cpu), cpu);

extra space after '='?
[...]

> +
> +       if (ret == 0 && rapl_model->core_events)
> +               ret = __rapl_cpu_offline(rapl_pmus_core,
> topology_logical_core_id(cpu),
> +                                  topology_sibling_cpumask(cpu),
> cpu);
> +
> +       return ret;
>  }
>  
>  static int __rapl_cpu_online(struct rapl_pmus *rapl_pmus, unsigned
> int rapl_pmu_idx,
> @@ -629,8 +725,14 @@ static int __rapl_cpu_online(struct rapl_pmus
> *rapl_pmus, unsigned int rapl_pmu_
>  
>  static int rapl_cpu_online(unsigned int cpu)
>  {
> -       return __rapl_cpu_online(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> +       int ret =  __rapl_cpu_online(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
>                                  get_rapl_pmu_cpumask(cpu), cpu);

extra space after '='?
[...]


> +
> +       if (rapl_core_cntr_mask & (1 << PERF_RAPL_PER_CORE))
> +               pr_info("hw unit of domain %s 2^-%d Joules\n",
> +                       rapl_core_domain_names[PERF_RAPL_PER_CORE],
> rapl_core_hw_unit);
>  }

Are we expecting to have more than one Domain for per_core power PMU?
if no, we don't need introduce
+enum perf_rapl_core_events {
+	PERF_RAPL_PER_CORE = 0,		/* per-core */
+
+	PERF_RAPL_CORE_EVENTS_MAX,
+	NR_RAPL_CORE_DOMAINS = PERF_RAPL_CORE_EVENTS_MAX,
+};
+
and check for NR_RAPL_CORE_DOMAINS all over the place.

Or else, we should use a loop here to advertise all possible per_core
domains. Either is okay with me but the code needs to be consistent.

>  
>  static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
> @@ -712,14 +820,16 @@ static const struct attribute_group
> *rapl_attr_update[] = {
>         NULL,
>  };
>  
> -static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr)
> +static const struct attribute_group *rapl_per_core_attr_update[] = {
> +       &rapl_events_per_core_group,
> +};
> +
> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr,
> int nr_rapl_pmu,
> +                                const struct attribute_group
> **rapl_attr_groups,
> +                                const struct attribute_group
> **rapl_attr_update)
>  {
> -       int nr_rapl_pmu = topology_max_packages();
>         struct rapl_pmus *rapl_pmus;
>  
> -       if (!rapl_pmu_is_pkg_scope())
> -               nr_rapl_pmu *= topology_max_dies_per_package();
> -
>         rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu,
> nr_rapl_pmu), GFP_KERNEL);
>         if (!rapl_pmus)
>                 return -ENOMEM;
> @@ -809,8 +919,10 @@ static struct rapl_model model_spr = {
>  
>  static struct rapl_model model_amd_hygon = {
>         .pkg_events     = BIT(PERF_RAPL_PKG),
> +       .core_events    = BIT(PERF_RAPL_PER_CORE),
>         .msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
>         .rapl_pkg_msrs  = amd_rapl_pkg_msrs,
> +       .rapl_core_msrs = amd_rapl_core_msrs,
>  };
>  
>  static const struct x86_cpu_id rapl_model_match[] __initconst = {
> @@ -867,6 +979,11 @@ static int __init rapl_pmu_init(void)
>  {
>         const struct x86_cpu_id *id;
>         int ret;
> +       int nr_rapl_pmu = topology_max_packages() *
> topology_max_dies_per_package();
> +       int nr_cores = topology_max_packages() *
> topology_num_cores_per_package();
> +

I thought we agreed to use one variable for all three cases.

thanks,
rui