Hi Kajol,
Thanks for the patch. Minor review comment below:
Kajol Jain <kj...@linux.ibm.com> writes:
> Commit 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
> added performance monitoring support for papr-scm nvdimm devices via
> perf interface. Commit also added an array in papr_scm_priv
> structure called "nvdimm_events_map", which got filled based on the
> result of H_SCM_PERFORMANCE_STATS hcall.
>
> Currently there is an assumption that the order of events in the
> stats buffer, returned by the hypervisor is same. And that order also
> matches with the events specified in nvdimm driver code.
> But this assumption is not documented anywhere in Power Architecture
> Platform Requirements (PAPR) document. Although the order
> of events happens to be same on current systems, but it might
> not be true in future generation systems. Fix the issue, by
> adding a static mapping for nvdimm events to corresponding stat-id,
> and removing the dynamic map from papr_scm_priv structure.
>
> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
> Reported-by: Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com>
> Signed-off-by: Kajol Jain <kj...@linux.ibm.com>
<snip>
> @@ -460,10 +480,9 @@ static void papr_scm_pmu_del(struct perf_event *event,
> int flags)
>
> static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct
> nvdimm_pmu *nd_pmu)
> {
> - struct papr_scm_perf_stat *stat;
> struct papr_scm_perf_stats *stats;
> u32 available_events;
> - int index, rc = 0;
> + int rc;
>
> if (!p->stat_buffer_len)
> return -ENOENT;
> @@ -476,34 +495,12 @@ static int papr_scm_pmu_check_events(struct
> papr_scm_priv *p, struct nvdimm_pmu
> /* Allocate the buffer for phyp where stats are written */
> stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
> if (!stats) {
> - rc = -ENOMEM;
> - return rc;
> + return -ENOMEM;
> }
>
> /* Called to get list of events supported */
> rc = drc_pmem_query_stats(p, stats, 0);
> - if (rc)
> - goto out;
> -
> - /*
> - * Allocate memory and populate nvdimm_event_map.
> - * Allocate an extra element for NULL entry
> - */
> - p->nvdimm_events_map = kcalloc(available_events + 1,
> - sizeof(stat->stat_id),
> - GFP_KERNEL);
> - if (!p->nvdimm_events_map) {
> - rc = -ENOMEM;
> - goto out;
> - }
>
> - /* Copy all stat_ids to event map */
> - for (index = 0, stat = stats->scm_statistic;
> - index < available_events; index++, ++stat) {
> - memcpy(&p->nvdimm_events_map[index * sizeof(stat->stat_id)],
> - &stat->stat_id, sizeof(stat->stat_id));
> - }
> -out:
> kfree(stats);
> return rc;
> }
Earlier implementation of papr_scm_pmu_check_events() would copy the
contents of returned stat-ids to struct papr_scm_priv->nvdimm_events_map,
hence it was needed.
With static events map you dont really need to call
drc_pmem_query_stats() as that would have been already being done once
in papr_scm_probe() before papr_scm_pmu_register() is called:
papr_scm_probe()
{
...
/* Try retrieving the stat buffer and see if its supported */
stat_size = drc_pmem_query_stats(p, NULL, 0);
...
papr_scm_pmu_register(p);
...
}
I would suggest replacing single callsite of papr_scm_pmu_check_events()
with the check
if (!p->stat_buffer_len)
goto pmu_check_events_err;
<snip>
--
Cheers
~ Vaibhav
