* [PATCH] powerpc/papr_scm: Fix nvdimm event mappings
@ 2022-06-10 13:34 Kajol Jain
2022-06-27 6:35 ` Michael Ellerman
0 siblings, 1 reply; 3+ messages in thread
From: Kajol Jain @ 2022-06-10 13:34 UTC (permalink / raw)
To: mpe, linuxppc-dev, vaibhav
Cc: nvdimm, atrajeev, rnsastry, kjain, maddy, Aneesh Kumar K . V,
dan.j.williams, disgoel
Commit 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
adds performance monitoring support for papr-scm nvdimm devices via
perf interface. It also adds one array in papr_scm_priv
structure called "nvdimm_events_map", to dynamically save the stat_id
for events specified in nvdimm driver code "nd_perf.c".
Right now the mapping is done based on the result of
H_SCM_PERFORMANCE_STATS hcall, when all the stats are
requested. Currently there is an assumption, that a
certain stat will always be found at a specific offset
in the stat buffer. The assumption may not be true or
documented as part of PAPR documentation. Fixing it,
by adding a static mapping for nvdimm events to
corresponding stat-id, and removing the map from
papr_scm_priv structure.
Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 59 ++++++++++-------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 181b855b3050..5434c654a797 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -124,9 +124,6 @@ struct papr_scm_priv {
/* The bits which needs to be overridden */
u64 health_bitmap_inject_mask;
-
- /* array to have event_code and stat_id mappings */
- u8 *nvdimm_events_map;
};
static int papr_scm_pmem_flush(struct nd_region *nd_region,
@@ -350,6 +347,26 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
#ifdef CONFIG_PERF_EVENTS
#define to_nvdimm_pmu(_pmu) container_of(_pmu, struct nvdimm_pmu, pmu)
+static const char * const nvdimm_events_map[] = {
+ "N/A",
+ "CtlResCt",
+ "CtlResTm",
+ "PonSecs ",
+ "MemLife ",
+ "CritRscU",
+ "HostLCnt",
+ "HostSCnt",
+ "HostSDur",
+ "HostLDur",
+ "MedRCnt ",
+ "MedWCnt ",
+ "MedRDur ",
+ "MedWDur ",
+ "CchRHCnt",
+ "CchWHCnt",
+ "FastWCnt",
+};
+
static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev, u64 *count)
{
struct papr_scm_perf_stat *stat;
@@ -361,7 +378,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
size = sizeof(struct papr_scm_perf_stats) +
sizeof(struct papr_scm_perf_stat);
- if (!p || !p->nvdimm_events_map)
+ if (!p)
return -EINVAL;
stats = kzalloc(size, GFP_KERNEL);
@@ -370,7 +387,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
stat = &stats->scm_statistic[0];
memcpy(&stat->stat_id,
- &p->nvdimm_events_map[event->attr.config * sizeof(stat->stat_id)],
+ nvdimm_events_map[event->attr.config],
sizeof(stat->stat_id));
stat->stat_val = 0;
@@ -460,10 +477,9 @@ static void papr_scm_pmu_del(struct perf_event *event, int flags)
static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu *nd_pmu)
{
- struct papr_scm_perf_stat *stat;
struct papr_scm_perf_stats *stats;
u32 available_events;
- int index, rc = 0;
+ int rc = 0;
available_events = (p->stat_buffer_len - sizeof(struct papr_scm_perf_stats))
/ sizeof(struct papr_scm_perf_stat);
@@ -473,34 +489,12 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
/* Allocate the buffer for phyp where stats are written */
stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
if (!stats) {
- rc = -ENOMEM;
- return rc;
+ return -ENOMEM;
}
/* Called to get list of events supported */
rc = drc_pmem_query_stats(p, stats, 0);
- if (rc)
- goto out;
- /*
- * Allocate memory and populate nvdimm_event_map.
- * Allocate an extra element for NULL entry
- */
- p->nvdimm_events_map = kcalloc(available_events + 1,
- sizeof(stat->stat_id),
- GFP_KERNEL);
- if (!p->nvdimm_events_map) {
- rc = -ENOMEM;
- goto out;
- }
-
- /* Copy all stat_ids to event map */
- for (index = 0, stat = stats->scm_statistic;
- index < available_events; index++, ++stat) {
- memcpy(&p->nvdimm_events_map[index * sizeof(stat->stat_id)],
- &stat->stat_id, sizeof(stat->stat_id));
- }
-out:
kfree(stats);
return rc;
}
@@ -536,7 +530,7 @@ static void papr_scm_pmu_register(struct papr_scm_priv *p)
rc = register_nvdimm_pmu(nd_pmu, p->pdev);
if (rc)
- goto pmu_register_err;
+ goto pmu_check_events_err;
/*
* Set archdata.priv value to nvdimm_pmu structure, to handle the
@@ -545,8 +539,6 @@ static void papr_scm_pmu_register(struct papr_scm_priv *p)
p->pdev->archdata.priv = nd_pmu;
return;
-pmu_register_err:
- kfree(p->nvdimm_events_map);
pmu_check_events_err:
kfree(nd_pmu);
pmu_err_print:
@@ -1557,7 +1549,6 @@ static int papr_scm_remove(struct platform_device *pdev)
unregister_nvdimm_pmu(pdev->archdata.priv);
pdev->archdata.priv = NULL;
- kfree(p->nvdimm_events_map);
kfree(p->bus_desc.provider_name);
kfree(p);
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/papr_scm: Fix nvdimm event mappings
2022-06-10 13:34 [PATCH] powerpc/papr_scm: Fix nvdimm event mappings Kajol Jain
@ 2022-06-27 6:35 ` Michael Ellerman
2022-06-28 6:04 ` kajoljain
0 siblings, 1 reply; 3+ messages in thread
From: Michael Ellerman @ 2022-06-27 6:35 UTC (permalink / raw)
To: Kajol Jain, linuxppc-dev, vaibhav
Cc: nvdimm, atrajeev, rnsastry, kjain, maddy, Aneesh Kumar K . V,
dan.j.williams, disgoel
Hi Kajol,
A few comments below ...
Kajol Jain <kjain@linux.ibm.com> writes:
> Commit 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
> adds performance monitoring support for papr-scm nvdimm devices via
^
We're talking about a commit that's already happened so we should use
past tense, so "added".
> perf interface. It also adds one array in papr_scm_priv
"added"
> structure called "nvdimm_events_map", to dynamically save the stat_id
> for events specified in nvdimm driver code "nd_perf.c".
>
> Right now the mapping is done based on the result of
> H_SCM_PERFORMANCE_STATS hcall, when all the stats are
> requested. Currently there is an assumption, that a
> certain stat will always be found at a specific offset
> in the stat buffer.
^
"returned by the hypervisor."
To make it clear where the stat buffer comes from, and that it's out of
our control.
> The assumption may not be true or documented as part of PAPR
> documentation.
That reads as the assumption "may not be documented as part of PAPR". I
think what you mean is the assumption *is not* documented by PAPR, and
although it happens to be true on current systems it may not be true in
future.
> Fixing it, by adding a static mapping for nvdimm events to
Fix it
> corresponding stat-id, and removing the map from
> papr_scm_priv structure.
>
> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 59 ++++++++++-------------
> 1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 181b855b3050..5434c654a797 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -350,6 +347,26 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> #ifdef CONFIG_PERF_EVENTS
> #define to_nvdimm_pmu(_pmu) container_of(_pmu, struct nvdimm_pmu, pmu)
>
> +static const char * const nvdimm_events_map[] = {
> + "N/A",
> + "CtlResCt",
> + "CtlResTm",
> + "PonSecs ",
> + "MemLife ",
> + "CritRscU",
> + "HostLCnt",
> + "HostSCnt",
> + "HostSDur",
> + "HostLDur",
> + "MedRCnt ",
> + "MedWCnt ",
> + "MedRDur ",
> + "MedWDur ",
> + "CchRHCnt",
> + "CchWHCnt",
> + "FastWCnt",
> +};
The order of the strings in that array becomes ABI. Because it defines
the mapping from perf_event.attr.config (perf user ABI) to the actual
event we request from the hypervisor.
So I'd like that made more explicit by using designated initialisers, eg:
static const char * const nvdimm_events_map[] = {
[1] = "CtlResCt",
[2] = "CtlResTm",
...
That way an accidental reordering of the array won't break anything.
You shouldn't need to specify 0 either as it's not used.
> @@ -370,7 +387,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
>
> stat = &stats->scm_statistic[0];
> memcpy(&stat->stat_id,
> - &p->nvdimm_events_map[event->attr.config * sizeof(stat->stat_id)],
> + nvdimm_events_map[event->attr.config],
> sizeof(stat->stat_id));
It's not clear that this won't index off the end of the array.
There is a check in papr_scm_pmu_event_init(), but I'd probably be
happier if we did an explicit check in here as well, eg:
if (event->attr.config >= ARRAY_SIZE(nvdimm_events_map))
return -EINVAL;
> stat->stat_val = 0;
>
> @@ -460,10 +477,9 @@ static void papr_scm_pmu_del(struct perf_event *event, int flags)
>
> static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu *nd_pmu)
> {
> - struct papr_scm_perf_stat *stat;
> struct papr_scm_perf_stats *stats;
> u32 available_events;
> - int index, rc = 0;
> + int rc = 0;
You shouldn't need to initialise rc here. It's not used until the call
to drc_pmem_query_stats() below.
> available_events = (p->stat_buffer_len - sizeof(struct papr_scm_perf_stats))
> / sizeof(struct papr_scm_perf_stat);
> @@ -473,34 +489,12 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
> /* Allocate the buffer for phyp where stats are written */
> stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
> if (!stats) {
> - rc = -ENOMEM;
> - return rc;
> + return -ENOMEM;
> }
>
> /* Called to get list of events supported */
> rc = drc_pmem_query_stats(p, stats, 0);
> - if (rc)
> - goto out;
>
> - /*
> - * Allocate memory and populate nvdimm_event_map.
> - * Allocate an extra element for NULL entry
> - */
> - p->nvdimm_events_map = kcalloc(available_events + 1,
> - sizeof(stat->stat_id),
> - GFP_KERNEL);
> - if (!p->nvdimm_events_map) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> - /* Copy all stat_ids to event map */
> - for (index = 0, stat = stats->scm_statistic;
> - index < available_events; index++, ++stat) {
> - memcpy(&p->nvdimm_events_map[index * sizeof(stat->stat_id)],
> - &stat->stat_id, sizeof(stat->stat_id));
> - }
> -out:
> kfree(stats);
> return rc;
> }
cheers
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/papr_scm: Fix nvdimm event mappings
2022-06-27 6:35 ` Michael Ellerman
@ 2022-06-28 6:04 ` kajoljain
0 siblings, 0 replies; 3+ messages in thread
From: kajoljain @ 2022-06-28 6:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev, vaibhav
Cc: nvdimm, atrajeev, rnsastry, Aneesh Kumar K . V, maddy,
dan.j.williams, disgoel
On 6/27/22 12:05, Michael Ellerman wrote:
> Hi Kajol,
>
> A few comments below ...
Hi Michael,
Thanks for reviewing the patch. I will make the changes suggested by
you and send version 2 of this patch.
Thanks,
Kajol Jain
>
> Kajol Jain <kjain@linux.ibm.com> writes:
>> Commit 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
>> adds performance monitoring support for papr-scm nvdimm devices via
> ^
> We're talking about a commit that's already happened so we should use
> past tense, so "added".
>
>> perf interface. It also adds one array in papr_scm_priv
> "added"
>> structure called "nvdimm_events_map", to dynamically save the stat_id
>> for events specified in nvdimm driver code "nd_perf.c".
>>
>> Right now the mapping is done based on the result of
>> H_SCM_PERFORMANCE_STATS hcall, when all the stats are
>> requested. Currently there is an assumption, that a
>> certain stat will always be found at a specific offset
>> in the stat buffer.
> ^
> "returned by the hypervisor."
>
> To make it clear where the stat buffer comes from, and that it's out of
> our control.
>
>> The assumption may not be true or documented as part of PAPR
>> documentation.
>
> That reads as the assumption "may not be documented as part of PAPR". I
> think what you mean is the assumption *is not* documented by PAPR, and
> although it happens to be true on current systems it may not be true in
> future.
>
>> Fixing it, by adding a static mapping for nvdimm events to
> Fix it
>> corresponding stat-id, and removing the map from
>> papr_scm_priv structure.
>>
>> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
>> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 59 ++++++++++-------------
>> 1 file changed, 25 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 181b855b3050..5434c654a797 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -350,6 +347,26 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>> #ifdef CONFIG_PERF_EVENTS
>> #define to_nvdimm_pmu(_pmu) container_of(_pmu, struct nvdimm_pmu, pmu)
>>
>> +static const char * const nvdimm_events_map[] = {
>> + "N/A",
>> + "CtlResCt",
>> + "CtlResTm",
>> + "PonSecs ",
>> + "MemLife ",
>> + "CritRscU",
>> + "HostLCnt",
>> + "HostSCnt",
>> + "HostSDur",
>> + "HostLDur",
>> + "MedRCnt ",
>> + "MedWCnt ",
>> + "MedRDur ",
>> + "MedWDur ",
>> + "CchRHCnt",
>> + "CchWHCnt",
>> + "FastWCnt",
>> +};
>
> The order of the strings in that array becomes ABI. Because it defines
> the mapping from perf_event.attr.config (perf user ABI) to the actual
> event we request from the hypervisor.
>
> So I'd like that made more explicit by using designated initialisers, eg:
>
> static const char * const nvdimm_events_map[] = {
> [1] = "CtlResCt",
> [2] = "CtlResTm",
> ...
>
> That way an accidental reordering of the array won't break anything.
Yes make sense. Will do update it.
>
> You shouldn't need to specify 0 either as it's not used.
>
>> @@ -370,7 +387,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
>>
>> stat = &stats->scm_statistic[0];
>> memcpy(&stat->stat_id,
>> - &p->nvdimm_events_map[event->attr.config * sizeof(stat->stat_id)],
>> + nvdimm_events_map[event->attr.config],
>> sizeof(stat->stat_id));
>
> It's not clear that this won't index off the end of the array.
>
> There is a check in papr_scm_pmu_event_init(), but I'd probably be
> happier if we did an explicit check in here as well, eg:
>
> if (event->attr.config >= ARRAY_SIZE(nvdimm_events_map))
> return -EINVAL;
>
>
>> stat->stat_val = 0;
>>
>> @@ -460,10 +477,9 @@ static void papr_scm_pmu_del(struct perf_event *event, int flags)
>>
>> static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu *nd_pmu)
>> {
>> - struct papr_scm_perf_stat *stat;
>> struct papr_scm_perf_stats *stats;
>> u32 available_events;
>> - int index, rc = 0;
>> + int rc = 0;
>
> You shouldn't need to initialise rc here. It's not used until the call
> to drc_pmem_query_stats() below.
Ok sure.
>
>> available_events = (p->stat_buffer_len - sizeof(struct papr_scm_perf_stats))
>> / sizeof(struct papr_scm_perf_stat);
>> @@ -473,34 +489,12 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
>> /* Allocate the buffer for phyp where stats are written */
>> stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
>> if (!stats) {
>> - rc = -ENOMEM;
>> - return rc;
>> + return -ENOMEM;
>> }
>>
>> /* Called to get list of events supported */
>> rc = drc_pmem_query_stats(p, stats, 0);
>> - if (rc)
>> - goto out;
>>
>> - /*
>> - * Allocate memory and populate nvdimm_event_map.
>> - * Allocate an extra element for NULL entry
>> - */
>> - p->nvdimm_events_map = kcalloc(available_events + 1,
>> - sizeof(stat->stat_id),
>> - GFP_KERNEL);
>> - if (!p->nvdimm_events_map) {
>> - rc = -ENOMEM;
>> - goto out;
>> - }
>> -
>> - /* Copy all stat_ids to event map */
>> - for (index = 0, stat = stats->scm_statistic;
>> - index < available_events; index++, ++stat) {
>> - memcpy(&p->nvdimm_events_map[index * sizeof(stat->stat_id)],
>> - &stat->stat_id, sizeof(stat->stat_id));
>> - }
>> -out:
>> kfree(stats);
>> return rc;
>> }
>
> cheers
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-28 6:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-10 13:34 [PATCH] powerpc/papr_scm: Fix nvdimm event mappings Kajol Jain
2022-06-27 6:35 ` Michael Ellerman
2022-06-28 6:04 ` kajoljain
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).