Re: [PATCH v6 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.

[Ganapatrao Kulkarni]

Hi John,

On Wed, Oct 16, 2019 at 7:01 PM John Garry <john.garry@xxxxxxxxxx> wrote:
>
>
> > +TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
> > +TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
> > +TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
> > +TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
> > +
> > +static struct attribute *ccpi2_pmu_events_attrs[] = {
> > +     &tx2_pmu_event_attr_req_pktsent.attr.attr,
> > +     &tx2_pmu_event_attr_snoop_pktsent.attr.attr,
> > +     &tx2_pmu_event_attr_data_pktsent.attr.attr,
> > +     &tx2_pmu_event_attr_gic_pktsent.attr.attr,
> > +     NULL,
> > +};
>
> Hi Ganapatrao,
>
> Have you considered adding these as uncore pmu-events in the perf tool?
>
At the moment no, since the number of events exposed/listed are very few.

> Some advantages I see:
> - perf list is not swamped with all these uncore events per PMU
> For the Hisi uncore events, we get 100s of events (>600 on the board I
> just tested, which is crazy)
> - can add more description in the JSON files
> - less stuff in the kernel
>
> > +
> >  static const struct attribute_group l3c_pmu_events_attr_group = {
> >       .name = "events",
> >       .attrs = l3c_pmu_events_attrs,
> > @@ -174,6 +240,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
> >       .attrs = dmc_pmu_events_attrs,
> >  };
>
> [...]
>
> >               tx2_pmu->attr_groups = l3c_pmu_attr_groups;
> >               tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
> >                               "uncore_l3c_%d", tx2_pmu->node);
> > @@ -665,10 +846,13 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
> >               tx2_pmu->stop_event = uncore_stop_event_l3c;
> >               break;
> >       case PMU_TYPE_DMC:
> > -             tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
> > +             tx2_pmu->max_counters = TX2_PMU_DMC_L3C_MAX_COUNTERS;
> > +             tx2_pmu->counters_mask = 0x3;
> >               tx2_pmu->prorate_factor = TX2_PMU_DMC_CHANNELS;
> >               tx2_pmu->max_events = DMC_EVENT_MAX;
> > +             tx2_pmu->events_mask = 0x1f;
> >               tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
> > +             tx2_pmu->hrtimer_callback = tx2_hrtimer_callback;
> >               tx2_pmu->attr_groups = dmc_pmu_attr_groups;
> >               tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
> >                               "uncore_dmc_%d", tx2_pmu->node);
> > @@ -676,6 +860,21 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
> >               tx2_pmu->start_event = uncore_start_event_dmc;
> >               tx2_pmu->stop_event = uncore_stop_event_dmc;
> >               break;
> > +     case PMU_TYPE_CCPI2:
> > +             /* CCPI2 has 8 counters */
> > +             tx2_pmu->max_counters = TX2_PMU_CCPI2_MAX_COUNTERS;
> > +             tx2_pmu->counters_mask = 0x7;
> > +             tx2_pmu->prorate_factor = 1;
> > +             tx2_pmu->max_events = CCPI2_EVENT_MAX;
> > +             tx2_pmu->events_mask = 0x1ff;
> > +             tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
> > +             tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
> > +                             "uncore_ccpi2_%d", tx2_pmu->node);
>
> Do you need to check this for name == NULL?
>
> > +             tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
> > +             tx2_pmu->start_event = uncore_start_event_ccpi2;
> > +             tx2_pmu->stop_event = uncore_stop_event_ccpi2;
> > +             tx2_pmu->hrtimer_callback = NULL;
> > +             break;
> >       case PMU_TYPE_INVALID:
> >               devm_kfree(dev, tx2_pmu);
> >               return NULL;
> > @@ -744,7 +943,9 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
> >       if (cpu != tx2_pmu->cpu)
> >               return 0;
> >
> > -     hrtimer_cancel(&tx2_pmu->hrtimer);
> > +     if (tx2_pmu->hrtimer_callback)
> > +             hrtimer_cancel(&tx2_pmu->hrtimer);
> > +
> >       cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
> >       cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
> >       new_cpu = cpumask_any_and(
> >
>
> Thanks,
> John
>
>

Thanks,
Ganapat