linux-nvdimm.lists.01.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
@ 2021-05-05 19:17 Vaibhav Jain
  2021-05-05 22:53 ` kernel test robot
  2021-05-06  5:02 ` Aneesh Kumar K.V
  0 siblings, 2 replies; 4+ messages in thread
From: Vaibhav Jain @ 2021-05-05 19:17 UTC (permalink / raw)
  To: linuxppc-dev, linux-nvdimm
  Cc: Vaibhav Jain, Aneesh Kumar K . V, Michael Ellerman

In case performance stats for an nvdimm are not available, reading the
'perf_stats' sysfs file returns an -ENOENT error. A better approach is
to make the 'perf_stats' file entirely invisible to indicate that
performance stats for an nvdimm are unavailable.

So this patch updates 'papr_nd_attribute_group' to add a 'is_visible'
callback implemented as newly introduced 'papr_nd_attribute_visible()'
that returns an appropriate mode in case performance stats aren't
supported in a given nvdimm.

Also the initialization of 'papr_scm_priv.stat_buffer_len' is moved
from papr_scm_nvdimm_init() to papr_scm_probe() so that it value is
available when 'papr_nd_attribute_visible()' is called during nvdimm
initialization.

Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++-------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 12f1513f0fca..90f0af8fefe8 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -907,6 +907,20 @@ static ssize_t flags_show(struct device *dev,
 }
 DEVICE_ATTR_RO(flags);
 
+umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
+				  int n)
+{
+	struct device *dev = container_of(kobj, typeof(*dev), kobj);
+	struct nvdimm *nvdimm = to_nvdimm(dev);
+	struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
+
+	/* For if perf-stats not available remove perf_stats sysfs */
+	if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
+		return 0;
+
+	return attr->mode;
+}
+
 /* papr_scm specific dimm attributes */
 static struct attribute *papr_nd_attributes[] = {
 	&dev_attr_flags.attr,
@@ -916,6 +930,7 @@ static struct attribute *papr_nd_attributes[] = {
 
 static struct attribute_group papr_nd_attribute_group = {
 	.name = "papr",
+	.is_visible = papr_nd_attribute_visible,
 	.attrs = papr_nd_attributes,
 };
 
@@ -931,7 +946,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;
@@ -1016,16 +1030,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);
@@ -1102,6 +1106,7 @@ static int papr_scm_probe(struct platform_device *pdev)
 	u64 blocks, block_size;
 	struct papr_scm_priv *p;
 	const char *uuid_str;
+	ssize_t stat_size;
 	u64 uuid[2];
 	int rc;
 
@@ -1179,6 +1184,16 @@ static int papr_scm_probe(struct platform_device *pdev)
 	p->res.name  = pdev->name;
 	p->res.flags = IORESOURCE_MEM;
 
+	/* 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");
+	}
+
 	rc = papr_scm_nvdimm_init(p);
 	if (rc)
 		goto err2;
-- 
2.31.1
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
  2021-05-05 19:17 [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable Vaibhav Jain
@ 2021-05-05 22:53 ` kernel test robot
  2021-05-06  5:02 ` Aneesh Kumar K.V
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-05-05 22:53 UTC (permalink / raw)
  To: Vaibhav Jain, linuxppc-dev, linux-nvdimm
  Cc: kbuild-all, Vaibhav Jain, Aneesh Kumar K . V, Michael Ellerman

Hi Vaibhav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.12 next-20210505]
[cannot apply to scottwood/next mpe/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Make-perf_stats-invisible-if-perf-stats-unavailable/20210506-031853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/8e7ce04ec36dea95fad178585b697a9c5b5c259d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vaibhav-Jain/powerpc-papr_scm-Make-perf_stats-invisible-if-perf-stats-unavailable/20210506-031853
        git checkout 8e7ce04ec36dea95fad178585b697a9c5b5c259d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/powerpc/platforms/pseries/papr_scm.c:903:9: warning: no previous prototype for 'papr_nd_attribute_visible' [-Wmissing-prototypes]
     903 | umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/papr_nd_attribute_visible +903 arch/powerpc/platforms/pseries/papr_scm.c

   902	
 > 903	umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
   904					  int n)
   905	{
   906		struct device *dev = container_of(kobj, typeof(*dev), kobj);
   907		struct nvdimm *nvdimm = to_nvdimm(dev);
   908		struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
   909	
   910		/* For if perf-stats not available remove perf_stats sysfs */
   911		if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
   912			return 0;
   913	
   914		return attr->mode;
   915	}
   916	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
  2021-05-05 19:17 [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable Vaibhav Jain
  2021-05-05 22:53 ` kernel test robot
@ 2021-05-06  5:02 ` Aneesh Kumar K.V
  2021-05-07 11:40   ` Vaibhav Jain
  1 sibling, 1 reply; 4+ messages in thread
From: Aneesh Kumar K.V @ 2021-05-06  5:02 UTC (permalink / raw)
  To: Vaibhav Jain, linuxppc-dev, linux-nvdimm; +Cc: Vaibhav Jain, Michael Ellerman

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> In case performance stats for an nvdimm are not available, reading the
> 'perf_stats' sysfs file returns an -ENOENT error. A better approach is
> to make the 'perf_stats' file entirely invisible to indicate that
> performance stats for an nvdimm are unavailable.
>
> So this patch updates 'papr_nd_attribute_group' to add a 'is_visible'
> callback implemented as newly introduced 'papr_nd_attribute_visible()'
> that returns an appropriate mode in case performance stats aren't
> supported in a given nvdimm.
>
> Also the initialization of 'papr_scm_priv.stat_buffer_len' is moved
> from papr_scm_nvdimm_init() to papr_scm_probe() so that it value is
> available when 'papr_nd_attribute_visible()' is called during nvdimm
> initialization.
>
> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++-------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 12f1513f0fca..90f0af8fefe8 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -907,6 +907,20 @@ static ssize_t flags_show(struct device *dev,
>  }
>  DEVICE_ATTR_RO(flags);
>  
> +umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
> +				  int n)
> +{
> +	struct device *dev = container_of(kobj, typeof(*dev), kobj);
> +	struct nvdimm *nvdimm = to_nvdimm(dev);
> +	struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
> +
> +	/* For if perf-stats not available remove perf_stats sysfs */
> +	if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
>  /* papr_scm specific dimm attributes */
>  static struct attribute *papr_nd_attributes[] = {
>  	&dev_attr_flags.attr,
> @@ -916,6 +930,7 @@ static struct attribute *papr_nd_attributes[] = {
>  
>  static struct attribute_group papr_nd_attribute_group = {
>  	.name = "papr",
> +	.is_visible = papr_nd_attribute_visible,
>  	.attrs = papr_nd_attributes,
>  };
>  
> @@ -931,7 +946,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;
> @@ -1016,16 +1030,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);
> @@ -1102,6 +1106,7 @@ static int papr_scm_probe(struct platform_device *pdev)
>  	u64 blocks, block_size;
>  	struct papr_scm_priv *p;
>  	const char *uuid_str;
> +	ssize_t stat_size;
>  	u64 uuid[2];
>  	int rc;
>  
> @@ -1179,6 +1184,16 @@ static int papr_scm_probe(struct platform_device *pdev)
>  	p->res.name  = pdev->name;
>  	p->res.flags = IORESOURCE_MEM;
>  
> +	/* 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");
> +	}

With this patch https://lore.kernel.org/linuxppc-dev/20210505191606.51666-1-vaibhav@linux.ibm.com
We are adding details of whyy performance stat query hcall failed. Do we
need to print again here?  Are we being more verbose here?

-aneesh
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
  2021-05-06  5:02 ` Aneesh Kumar K.V
@ 2021-05-07 11:40   ` Vaibhav Jain
  0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2021-05-07 11:40 UTC (permalink / raw)
  To: Aneesh Kumar K.V, linuxppc-dev, linux-nvdimm; +Cc: Michael Ellerman

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
>> In case performance stats for an nvdimm are not available, reading the
>> 'perf_stats' sysfs file returns an -ENOENT error. A better approach is
>> to make the 'perf_stats' file entirely invisible to indicate that
>> performance stats for an nvdimm are unavailable.
>>
>> So this patch updates 'papr_nd_attribute_group' to add a 'is_visible'
>> callback implemented as newly introduced 'papr_nd_attribute_visible()'
>> that returns an appropriate mode in case performance stats aren't
>> supported in a given nvdimm.
>>
>> Also the initialization of 'papr_scm_priv.stat_buffer_len' is moved
>> from papr_scm_nvdimm_init() to papr_scm_probe() so that it value is
>> available when 'papr_nd_attribute_visible()' is called during nvdimm
>> initialization.
>>
>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 12f1513f0fca..90f0af8fefe8 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -907,6 +907,20 @@ static ssize_t flags_show(struct device *dev,
>>  }
>>  DEVICE_ATTR_RO(flags);
>>  
>> +umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
>> +				  int n)
>> +{
>> +	struct device *dev = container_of(kobj, typeof(*dev), kobj);
>> +	struct nvdimm *nvdimm = to_nvdimm(dev);
>> +	struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
>> +
>> +	/* For if perf-stats not available remove perf_stats sysfs */
>> +	if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
>> +		return 0;
>> +
>> +	return attr->mode;
>> +}
>> +
>>  /* papr_scm specific dimm attributes */
>>  static struct attribute *papr_nd_attributes[] = {
>>  	&dev_attr_flags.attr,
>> @@ -916,6 +930,7 @@ static struct attribute *papr_nd_attributes[] = {
>>  
>>  static struct attribute_group papr_nd_attribute_group = {
>>  	.name = "papr",
>> +	.is_visible = papr_nd_attribute_visible,
>>  	.attrs = papr_nd_attributes,
>>  };
>>  
>> @@ -931,7 +946,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;
>> @@ -1016,16 +1030,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);
>> @@ -1102,6 +1106,7 @@ static int papr_scm_probe(struct platform_device *pdev)
>>  	u64 blocks, block_size;
>>  	struct papr_scm_priv *p;
>>  	const char *uuid_str;
>> +	ssize_t stat_size;
>>  	u64 uuid[2];
>>  	int rc;
>>  
>> @@ -1179,6 +1184,16 @@ static int papr_scm_probe(struct platform_device *pdev)
>>  	p->res.name  = pdev->name;
>>  	p->res.flags = IORESOURCE_MEM;
>>  
>> +	/* 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");
>> +	}
>
> With this patch https://lore.kernel.org/linuxppc-dev/20210505191606.51666-1-vaibhav@linux.ibm.com
> We are adding details of whyy performance stat query hcall failed. Do we
> need to print again here?  Are we being more verbose here?
>
Yes agree this looks more verbose with the other patch you mentioned. I
have sent out a v2 of this patch with this dev_info removed.


> -aneesh
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

-- 
Cheers
~ Vaibhav
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-05-07 11:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 19:17 [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable Vaibhav Jain
2021-05-05 22:53 ` kernel test robot
2021-05-06  5:02 ` Aneesh Kumar K.V
2021-05-07 11:40   ` Vaibhav Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).