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

[kajoljain <kjain@linux.ibm.com>]

On 7/12/22 10:45, Vaibhav Jain wrote:
> 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:
> 
> 

Hi Vaibhav,
    Thanks for reviewing the patch. Yes it make sense, as mainly we want
to make sure, in case stat buffer is empty we will not register the
nvdimm pmu. I will do the change and send next version of the patch.

Thanks,
Kajol Jain

> 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>
>