Re: [PATCH v3] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
On 5/8/21 10:06 AM, Vaibhav Jain wrote: > Currently drc_pmem_qeury_stats() generates a dev_err in case > "Enable Performance Information Collection" feature is disabled from > HMC or performance stats are not available for an nvdimm. The error is > of the form below: > > papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query >performance stats, Err:-10 > > This error message confuses users as it implies a possible problem > with the nvdimm even though its due to a disabled/unavailable > feature. We fix this by explicitly handling the H_AUTHORITY and > H_UNSUPPORTED errors from the H_SCM_PERFORMANCE_STATS hcall. > > In case of H_AUTHORITY error an info message is logged instead of an > error, saying that "Permission denied while accessing performance > stats" and an EPERM error is returned back. > > In case of H_UNSUPPORTED error we return a EOPNOTSUPP error back from > drc_pmem_query_stats() indicating that performance stats-query > operation is not supported on this nvdimm. > > Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from > PHYP') > Signed-off-by: Vaibhav Jain > --- > Changelog > > v3: > * Return EOPNOTSUPP error in case of H_UNSUPPORTED [ Ira ] > * Return EPERM in case of H_AUTHORITY [ Ira ] > * Updated patch description > Patch looks good to me. Reviewed-By: Kajol Jain Thanks, Kajol Jain > v2: > * Updated the message logged in case of H_AUTHORITY error [ Ira ] > * Switched from dev_warn to dev_info in case of H_AUTHORITY error. > * Instead of -EPERM return -EACCESS for H_AUTHORITY error. > * Added explicit handling of H_UNSUPPORTED error. > --- > arch/powerpc/platforms/pseries/papr_scm.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c > b/arch/powerpc/platforms/pseries/papr_scm.c > index ef26fe40efb0..e2b69cc3beaf 100644 > --- a/arch/powerpc/platforms/pseries/papr_scm.c > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > @@ -310,6 +310,13 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv > *p, > dev_err(>pdev->dev, > "Unknown performance stats, Err:0x%016lX\n", ret[0]); > return -ENOENT; > + } else if (rc == H_AUTHORITY) { > + dev_info(>pdev->dev, > + "Permission denied while accessing performance stats"); > + return -EPERM; > + } else if (rc == H_UNSUPPORTED) { > + dev_dbg(>pdev->dev, "Performance stats unsupported\n"); > + return -EOPNOTSUPP; > } else if (rc != H_SUCCESS) { > dev_err(>pdev->dev, > "Failed to query performance stats, Err:%lld\n", rc); > ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFC 1/4] drivers/nvdimm: Add perf interface to expose nvdimm performance stats
On 5/14/21 5:17 PM, Peter Zijlstra wrote: > On Thu, May 13, 2021 at 05:56:14PM +0530, kajoljain wrote: > >> But yes the current read/add/del functions are not adding value. We >> could add an arch/platform specific function which could handle the >> capturing of the counter data and do the rest of the operation here, >> is this approach better? > > Right; have your register_nvdimm_pmu() set pmu->{add,del,read} to > nd_pmu->{add,del,read} directly, don't bother with these intermediates. > Also you can WARN_ON_ONCE() if any of them are NULL and fail > registration at that point. > Hi Peter, I will make all required changes and send next version of this patchset soon. Thanks, Kajol Jain ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFC 1/4] drivers/nvdimm: Add perf interface to expose nvdimm performance stats
On 5/12/21 10:57 PM, Peter Zijlstra wrote: > On Wed, May 12, 2021 at 10:08:21PM +0530, Kajol Jain wrote: >> +static void nvdimm_pmu_read(struct perf_event *event) >> +{ >> +struct nvdimm_pmu *nd_pmu = to_nvdimm_pmu(event->pmu); >> + >> +/* jump to arch/platform specific callbacks if any */ >> +if (nd_pmu && nd_pmu->read) >> +nd_pmu->read(event, nd_pmu->dev); >> +} >> + >> +static void nvdimm_pmu_del(struct perf_event *event, int flags) >> +{ >> +struct nvdimm_pmu *nd_pmu = to_nvdimm_pmu(event->pmu); >> + >> +/* jump to arch/platform specific callbacks if any */ >> +if (nd_pmu && nd_pmu->del) >> +nd_pmu->del(event, flags, nd_pmu->dev); >> +} >> + >> +static int nvdimm_pmu_add(struct perf_event *event, int flags) >> +{ >> +struct nvdimm_pmu *nd_pmu = to_nvdimm_pmu(event->pmu); >> + >> +if (flags & PERF_EF_START) >> +/* jump to arch/platform specific callbacks if any */ >> +if (nd_pmu && nd_pmu->add) >> +return nd_pmu->add(event, flags, nd_pmu->dev); >> +return 0; >> +} > > What's the value add here? Why can't you directly set driver pointers? I > also don't really believe ->{add,del,read} can be optional and still > have a sane driver. > Hi Peter, The intend for adding these callbacks is to give flexibility to the arch/platform specific driver code to use its own routine for getting counter data or specific checks/operations. Arch/platform driver code would have different method to get the counter data like IBM pseries nmem* device which uses a hypervisor call(hcall). But yes the current read/add/del functions are not adding value. We could add an arch/platform specific function which could handle the capturing of the counter data and do the rest of the operation here, is this approach better? Thanks, Kajol Jain ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
On 4/15/21 5:18 PM, Vaibhav Jain wrote: > Ira Weiny writes: > >> On Wed, Apr 14, 2021 at 09:51:40PM +0530, Vaibhav Jain wrote: >>> Thanks for looking into this patch Ira, >>> >>> Ira Weiny writes: >>> On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote: > Currently drc_pmem_qeury_stats() generates a dev_err in case > "Enable Performance Information Collection" feature is disabled from > HMC. The error is of the form below: > > papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query >performance stats, Err:-10 > > This error message confuses users as it implies a possible problem > with the nvdimm even though its due to a disabled feature. > > So we fix this by explicitly handling the H_AUTHORITY error from the > H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an > error, saying that "Performance stats in-accessible". > > Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats > from PHYP') > Signed-off-by: Vaibhav Jain > --- > arch/powerpc/platforms/pseries/papr_scm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c > b/arch/powerpc/platforms/pseries/papr_scm.c > index 835163f54244..9216424f8be3 100644 > --- a/arch/powerpc/platforms/pseries/papr_scm.c > +++ b/arch/powerpc/platforms/pseries/papr_scm.c > @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct > papr_scm_priv *p, > dev_err(>pdev->dev, > "Unknown performance stats, Err:0x%016lX\n", ret[0]); > return -ENOENT; > + } else if (rc == H_AUTHORITY) { > + dev_warn(>pdev->dev, "Performance stats in-accessible"); > + return -EPERM; Is this because of a disabled feature or because of permissions? >>> >>> Its because of a disabled feature that revokes permission for a guest to >>> retrieve performance statistics. >>> >>> The feature is called "Enable Performance Information Collection" and >>> once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error >>> H_AUTHORITY indicating that the guest doesn't have permission to retrieve >>> performance statistics. >> >> In that case would it be appropriate to have the error message indicate a >> permission issue? >> >> Something like 'permission denied'? > > Yes, Something like "Permission denied while accessing performance > stats" might be more clear and intuitive. Hi Vaibhav, Thanks for the patch. I agree with Ira and above warning message with "Permission denied" looks more clear. With that change, patch looks good to me. Reviewed-By: Kajol Jain Thanks, Kajol Jain > > Will update the warn message in v2. > >> >> Ira >> > ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org