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

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

[Vaibhav Jain]

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@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;
 
 	/* 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
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

Re: [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@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
> 
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org