linux-nvdimm.lists.01.org archive mirror
 help / color / mirror / Atom feed
From: Vaibhav Jain <vaibhav@linux.ibm.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
	Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH v2] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
Date: Thu, 13 May 2021 14:56:32 +0530	[thread overview]
Message-ID: <87h7j7ass7.fsf@vajain21.in.ibm.com> (raw)
In-Reply-To: <CAPcyv4g7E39Hqe+fuv-J3gX1kiNVuenUfV4td_k9QtCNMV=C1w@mail.gmail.com>

Thanks for looking into this patch Dan,

Dan Williams <dan.j.williams@intel.com> writes:

> On Fri, May 7, 2021 at 4:40 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>>
>> 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')
>
> Since this has been the exposed ABI since v5.9 perhaps a note /
> analysis is needed here that the disappearance of this file will not
> break any existing scripts/tools.
>
Yes, I have added a note in patch description and also updated
Documentation/ABI/testing/sysfs-bus-papr-pmem with a note on when this
sysfs attr would be available. Thanks for this suggestion.

>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>>
>> v2:
>> * Removed a redundant dev_info() from pap_scm_nvdimm_init() [ Aneesh ]
>> * Marked papr_nd_attribute_visible() as static which also fixed the
>>   build warning reported by kernel build robot
>> ---
>>  arch/powerpc/platforms/pseries/papr_scm.c | 35 ++++++++++++++++-------
>>  1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index e2b69cc3beaf..11e7b90a3360 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);
>>
>> +static umode_t papr_nd_attribute_visible(struct kobject *kobj,
>> +                                        struct attribute *attr, int n)
>> +{
>> +       struct device *dev = container_of(kobj, typeof(*dev), kobj);
>
> This can use the kobj_to_dev() helper.
>
Fixed in v3
>> +       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,14 @@ 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 */
>
> s/retriving/retrieving/
>
Fixed in v3.
>> +       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);
>> +       }
>> +
>>         rc = papr_scm_nvdimm_init(p);
>>         if (rc)
>>                 goto err2;
>> --
>> 2.31.1
>>
>
> After the minor fixups above you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks

>
> ...I assume this will go through the PPC tree.
Yes

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

      reply	other threads:[~2021-05-13  9:26 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07 11:39 Vaibhav Jain
2021-05-12 19:48 ` Dan Williams
2021-05-13  9:26   ` Vaibhav Jain [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87h7j7ass7.fsf@vajain21.in.ibm.com \
    --to=vaibhav@linux.ibm.com \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=dan.j.williams@intel.com \
    --cc=linux-nvdimm@lists.01.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --subject='Re: [PATCH v2] powerpc/papr_scm: Make '\''perf_stats'\'' invisible if perf-stats unavailable' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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