Re: [PATCH v2] powerpc/papr_scm: Fix nvdimm event mappings

[Vaibhav Jain <vaibhav@linux.ibm.com>]

Hi Kajol,

Thanks for the patch. Minor review comment below:

Kajol Jain <kjain@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.kumar@linux.ibm.com>
> Signed-off-by: Kajol Jain <kjain@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