Re: [PATCH V2 2/3] powercap: intel_rapl: Introduce APIs for PMU support

[Zhang, Rui]

Hi, Rafael,

Thanks for reviewing.
Will refresh the patch based on your feedback, just a few coments
below.

> > +
> > +static bool is_rp_pmu_cpu(struct rapl_package *rp, int cpu)
> > +{
> > +       if (!rp->has_pmu)
> > +               return false;
> > +
> > +       if (rp->lead_cpu >= 0)
> > +               return cpu == rp->lead_cpu;
> 
> So if the given CPU is not the lead CPU, but it is located in the
> same
> package as the lead CPU, the function will return 'false'.  TBH, this
> is somewhat confusing.
> 
The above code actually applies to MSR RAPL because TPMI RAPL has
lead_cpu < 0.

Instead, I can use something like below to avoid the confusion.
	if (rp->priv->type != RAPL_IF_TPMI)
		return false;
and do future improvements when adding support for MSR RAPL.

> > +static void __rapl_pmu_event_start(struct perf_event *event)
> > +{
> > +       struct rapl_package_pmu_data *data =
> > event_to_pmu_data(event);
> > +
> > +       if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
> > +               return;
> > +
> > +       event->hw.state = 0;
> > +
> > +       list_add_tail(&event->active_entry, &data->active_list);
> > +
> > +       local64_set(&event->hw.prev_count,
> > event_read_counter(event));
> > +       if (++data->n_active == 1)
> > +               hrtimer_start(&data->hrtimer, data->timer_interval,
> > +                             HRTIMER_MODE_REL_PINNED);
> > +}
> > +
> > +static void rapl_pmu_event_start(struct perf_event *event, int
> > mode)
> > +{
> > +       struct rapl_package_pmu_data *data =
> > event_to_pmu_data(event);
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&data->lock, flags);
> > +       __rapl_pmu_event_start(event);
> > +       raw_spin_unlock_irqrestore(&data->lock, flags);
> 
> Why does it need to be raw_spin_lock_?
> 
> What exactly is protected by data->lock?
> 
This is copied from MSR RAPL PMU, which exists from day 1 of the code.

Let me double check.

> 
> > +
> > +static ssize_t cpumask_show(struct device *dev,
> > +                           struct device_attribute *attr, char
> > *buf)
> > +{
> > +       struct rapl_package *rp;
> > +       int cpu;
> > +
> > +       cpus_read_lock();
> 
> Is rapl_packages protected by this?

yes.
> 
> > +       cpumask_clear(&rapl_pmu.cpu_mask);
> 
> It doesn't look like rapl_pmu.cpu_mask is used outside this function,
> so why is it global?

Good catch, will fix it.
> 
> > +static int rapl_pmu_update(struct rapl_package *rp)
> > +{
> > +       int ret;
> > +
> > +       /* Return if PMU already covers all events supported by
> > current RAPL Package */
> > +       if (rapl_pmu.registered && !(rp->domain_map &
> > (~rapl_pmu.domain_map)))
> > +               return 0;
> > +
> > +       /* Unregister previous registered PMU */
> > +       if (rapl_pmu.registered) {
> > +               perf_pmu_unregister(&rapl_pmu.pmu);
> > +               memset(&rapl_pmu.pmu, 0, sizeof(struct pmu));
> > +       }
> > +
> > +       rapl_pmu.domain_map |= rp->domain_map;
> > +
> > +       memset(&rapl_pmu.pmu, 0, sizeof(struct pmu));
> > +       rapl_pmu.pmu.attr_groups = pmu_attr_groups;
> > +       rapl_pmu.pmu.attr_update = pmu_attr_update;
> > +       rapl_pmu.pmu.task_ctx_nr = perf_invalid_context;
> > +       rapl_pmu.pmu.event_init = rapl_pmu_event_init;
> > +       rapl_pmu.pmu.add = rapl_pmu_event_add;
> > +       rapl_pmu.pmu.del = rapl_pmu_event_del;
> > +       rapl_pmu.pmu.start = rapl_pmu_event_start;
> > +       rapl_pmu.pmu.stop = rapl_pmu_event_stop;
> > +       rapl_pmu.pmu.read = rapl_pmu_event_read;
> > +       rapl_pmu.pmu.module = THIS_MODULE;
> > +       rapl_pmu.pmu.capabilities = PERF_PMU_CAP_NO_EXCLUDE |
> > PERF_PMU_CAP_NO_INTERRUPT;
> > +       ret = perf_pmu_register(&rapl_pmu.pmu, "power", -1);
> > +       if (ret)
> > +               pr_warn("Failed to register PMU\n");
> > +
> > +       rapl_pmu.registered = !ret;
> 
> Why don't you set rp->has_pmu here?
> 
> > +
> > +       return ret;
> 
> It looks like this could be rearranged overall for more clarity:
> 
> ret = perf_pmu_register(&rapl_pmu.pmu, "power", -1);
> if (ret) {
>         pr_warn("Failed to register PMU\n");
>         return ret;
> }
> 
> rapl_pmu.registered = true;
> rp->has_pmu = true;
> 
> return 0;
> 
Sure.

In my previous design,
rapl_pmu_update() updates generic RAPL PMU.
rapl_package_add_pmu() updates a given RAPL package.
that is why I put
	rp->has_pmu = true;
in rapl_package_add_pmu().

> Also, the "Failed to register PMU\n" message is not particularly
> useful AFAICS.  It would be good to add some more context to it and
> maybe make it pr_info()?
> 
sure.

> > +}
> > +
> > +int rapl_package_add_pmu(struct rapl_package *rp)
> > +{
> > +       struct rapl_package_pmu_data *data = &rp->pmu_data;
> > +       int idx;
> > +       int ret;
> > +
> > +       if (rp->has_pmu)
> > +               return -EEXIST;
> > +
> > +       guard(cpus_read_lock)();
> 
> Why does this lock need to be held around the entire code below?
> 

This guaranteed that the RAPL Package is always valid and rapl_pmu
global variable is protected when updating the PMU.

> > +
> > +       for (idx = 0; idx < rp->nr_domains; idx++) {
> > +               struct rapl_domain *rd = &rp->domains[idx];
> > +               int domain = rd->id;
> > +               u64 val;
> > +
> > +               if (!test_bit(domain, &rp->domain_map))
> > +                       continue;
> > +
> > +               /*
> > +                * The RAPL PMU granularity is 2^-32 Joules
> > +                * data->scale[]: times of 2^-32 Joules for each
> > ENERGY COUNTER increase
> > +                */
> > +               val = rd->energy_unit * (1ULL << 32);
> > +               do_div(val, ENERGY_UNIT_SCALE * 1000000);
> > +               data->scale[domain] = val;
> > +
> > +               if (!rapl_pmu.timer_ms) {
> > +                       struct rapl_primitive_info *rpi =
> > get_rpi(rp, ENERGY_COUNTER);
> > +
> > +                       /*
> > +                        * Calculate the timer rate:
> > +                        * Use reference of 200W for scaling the
> > timeout to avoid counter
> > +                        * overflows.
> > +                        *
> > +                        * max_count = rpi->mask >> rpi->shift + 1
> > +                        * max_energy_pj = max_count * rd-
> > >energy_unit
> > +                        * max_time_sec = (max_energy_pj /
> > 1000000000) / 200w
> > +                        *
> > +                        * rapl_pmu.timer_ms = max_time_sec * 1000
> > / 2
> > +                        */
> > +                       val = (rpi->mask >> rpi->shift) + 1;
> > +                       val *= rd->energy_unit;
> > +                       do_div(val, 1000000 * 200 * 2);
> > +                       rapl_pmu.timer_ms = val;
> > +
> > +                       pr_info("%llu ms ovfl timer\n",
> > rapl_pmu.timer_ms);
> 
> s/ovfl/overflow/
> 
> And use pr_debug()?
> 
> > +               }
> > +
> > +               pr_info("Domain %s: hw unit %lld * 2^-32 Joules\n",
> > rd->name, data->scale[domain]);
> 
> pr_debug() here too?

These all follow the MSR RAPL PMU code, so that we see the same output
no matter using MSR RAPL or TPMI RAPL. I can change them to pr_debug().

Thanks,
rui