[PATCH] powerpc/papr_scm: Support dynamic enable/disable of performance statistics

[Ira Weiny]

On Mon, Sep 14, 2020 at 02:51:15AM +0530, Vaibhav Jain wrote:
> Collection of performance statistics of an NVDIMM can be dynamically
> enabled/disabled from the Hypervisor Management Console even when the
> guest lpar is running. The current implementation however will check if
> the performance statistics collection is supported during NVDIMM probe
> and if yes will assume that to be the case afterwards.
> 
> Hence we update papr_scm to remove this assumption from the code by
> eliminating the 'stat_buffer_len' member from 'struct papr_scm_priv'
> that was used to cache the max buffer size needed to fetch NVDIMM
> performance stats from PHYP. With that struct member gone, various
> functions that depended on it are updated. Specifically
> perf_stats_show() is updated to query the PHYP first for the size of
> buffer needed to hold all performance statistics instead of relying on
> 'stat_buffer_len'
> 
> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 53 +++++++++++------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 27268370dee00..6697e1c3b9ebe 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -112,9 +112,6 @@ struct papr_scm_priv {
>  
>  	/* Health information for the dimm */
>  	u64 health_bitmap;
> -
> -	/* length of the stat buffer as expected by phyp */
> -	size_t stat_buffer_len;
>  };
>  
>  static LIST_HEAD(papr_nd_regions);
> @@ -230,14 +227,15 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
>   * - If buff_stats == NULL the return value is the size in byes of the buffer
>   * needed to hold all supported performance-statistics.
>   * - If buff_stats != NULL and num_stats == 0 then we copy all known
> - * performance-statistics to 'buff_stat' and expect to be large enough to
> - * hold them.
> + * performance-statistics to 'buff_stat' and expect it to be large enough to
> + * hold them. The 'buff_size' args contains the size of the 'buff_stats'
>   * - if buff_stats != NULL and num_stats > 0 then copy the requested
>   * performance-statistics to buff_stats.
>   */
>  static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>  				    struct papr_scm_perf_stats *buff_stats,
> -				    unsigned int num_stats)
> +				    unsigned int num_stats,
> +				    size_t buff_size)
>  {
>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>  	size_t size;
> @@ -261,12 +259,18 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>  			size = sizeof(struct papr_scm_perf_stats) +
>  				num_stats * sizeof(struct papr_scm_perf_stat);
>  		else
> -			size = p->stat_buffer_len;
> +			size = buff_size;
>  	} else {
>  		/* In case of no out buffer ignore the size */
>  		size = 0;
>  	}
>  
> +	/* verify that the buffer size needed is sufficient */
> +	if (size > buff_size) {
> +		__WARN();
> +		return -EINVAL;
> +	}
> +
>  	/* Do the HCALL asking PHYP for info */
>  	rc = plpar_hcall(H_SCM_PERFORMANCE_STATS, ret, p->drc_index,
>  			 buff_stats ? virt_to_phys(buff_stats) : 0,
> @@ -277,6 +281,10 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>  		dev_err(&p->pdev->dev,
>  			"Unknown performance stats, Err:0x%016lX\n", ret[0]);
>  		return -ENOENT;
> +	} else if (rc == H_AUTHORITY) {
> +		dev_dbg(&p->pdev->dev,
> +			"Performance stats in-accessible\n");
> +		return -EPERM;
>  	} else if (rc != H_SUCCESS) {
>  		dev_err(&p->pdev->dev,
>  			"Failed to query performance stats, Err:%lld\n", rc);
> @@ -526,10 +534,6 @@ static int papr_pdsm_fuel_gauge(struct papr_scm_priv *p,
>  	struct papr_scm_perf_stat *stat;
>  	struct papr_scm_perf_stats *stats;
>  
> -	/* Silently fail if fetching performance metrics isn't  supported */
> -	if (!p->stat_buffer_len)
> -		return 0;
> -
>  	/* Allocate request buffer enough to hold single performance stat */
>  	size = sizeof(struct papr_scm_perf_stats) +
>  		sizeof(struct papr_scm_perf_stat);
> @@ -543,9 +547,11 @@ static int papr_pdsm_fuel_gauge(struct papr_scm_priv *p,
>  	stat->stat_val = 0;
>  
>  	/* Fetch the fuel gauge and populate it in payload */
> -	rc = drc_pmem_query_stats(p, stats, 1);
> +	rc = drc_pmem_query_stats(p, stats, 1, size);
>  	if (rc < 0) {
>  		dev_dbg(&p->pdev->dev, "Err(%d) fetching fuel gauge\n", rc);
> +		/* Silently fail if unable to fetch performance metric */
> +		rc = 0;
>  		goto free_stats;
>  	}
>  
> @@ -786,23 +792,25 @@ static ssize_t perf_stats_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
>  	int index;
> -	ssize_t rc;
> +	ssize_t rc, buff_len;
>  	struct seq_buf s;
>  	struct papr_scm_perf_stat *stat;
>  	struct papr_scm_perf_stats *stats;
>  	struct nvdimm *dimm = to_nvdimm(dev);
>  	struct papr_scm_priv *p = nvdimm_provider_data(dimm);
>  
> -	if (!p->stat_buffer_len)
> -		return -ENOENT;
> +	/* fetch the length of buffer needed to get all stats */
> +	buff_len = drc_pmem_query_stats(p, NULL, 0, 0);
> +	if (buff_len <= 0)
> +		return buff_len;

Generally I can't find anything wrong with this patch technically but the
architecture of drc_pmem_query_stats() seems overly complicated.

IOW, I feel like you are overloading drc_pmem_query_stats() in an odd way which
makes it and the callers code confusing.  Why can't you have a separate
function which returns the max buffer length and separate out the logic within
drc_pmem_query_stats() to make it clear how to call plpar_hcall() to get this
information?

Ira

>  
>  	/* Allocate the buffer for phyp where stats are written */
> -	stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
> +	stats = kzalloc(buff_len, GFP_KERNEL);
>  	if (!stats)
>  		return -ENOMEM;
>  
>  	/* Ask phyp to return all dimm perf stats */
> -	rc = drc_pmem_query_stats(p, stats, 0);
> +	rc = drc_pmem_query_stats(p, stats, 0, buff_len);
>  	if (rc)
>  		goto free_stats;
>  	/*
> @@ -891,7 +899,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>  	struct nd_region_desc ndr_desc;
>  	unsigned long dimm_flags;
>  	int target_nid, online_nid;
> -	ssize_t stat_size;
>  
>  	p->bus_desc.ndctl = papr_scm_ndctl;
>  	p->bus_desc.module = THIS_MODULE;
> @@ -962,16 +969,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>  	list_add_tail(&p->region_list, &papr_nd_regions);
>  	mutex_unlock(&papr_ndr_lock);
>  
> -	/* Try retriving the stat buffer and see if its supported */
> -	stat_size = drc_pmem_query_stats(p, NULL, 0);
> -	if (stat_size > 0) {
> -		p->stat_buffer_len = stat_size;
> -		dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
> -			p->stat_buffer_len);
> -	} else {
> -		dev_info(&p->pdev->dev, "Dimm performance stats unavailable\n");
> -	}
> -
>  	return 0;
>  
>  err:	nvdimm_bus_unregister(p->bus);
> -- 
> 2.26.2
>