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 <vaib...@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
>
