* [PATCH 1/2] powerpc/nvdimm: Use HCALL error as the return value
@ 2019-08-29 6:33 Aneesh Kumar K.V
2019-08-29 6:33 ` [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error Aneesh Kumar K.V
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2019-08-29 6:33 UTC (permalink / raw)
To: mpe; +Cc: Vaibhav Jain, Oliver O'Halloran, linuxppc-dev,
Aneesh Kumar K.V
This simplifies the error handling and also enable us to switch to
H_SCM_QUERY hcall in a later patch on H_OVERLAP error.
We also do some kernel print formatting fixup in this patch.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux•ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 24 ++++++++++-------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index a5ac371a3f06..220e595cb579 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -66,28 +66,22 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
} while (rc == H_BUSY);
if (rc) {
- /* H_OVERLAP needs a separate error path */
- if (rc == H_OVERLAP)
- return -EBUSY;
-
dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
- return -ENXIO;
+ return rc;
}
p->bound_addr = saved;
-
- dev_dbg(&p->pdev->dev, "bound drc %x to %pR\n", p->drc_index, &p->res);
-
+ dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
return 0;
}
-static int drc_pmem_unbind(struct papr_scm_priv *p)
+static void drc_pmem_unbind(struct papr_scm_priv *p)
{
unsigned long ret[PLPAR_HCALL_BUFSIZE];
uint64_t token = 0;
int64_t rc;
- dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
+ dev_dbg(&p->pdev->dev, "unbind drc 0x%x\n", p->drc_index);
/* NB: unbind has the same retry requirements as drc_pmem_bind() */
do {
@@ -110,10 +104,10 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
if (rc)
dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
else
- dev_dbg(&p->pdev->dev, "unbind drc %x complete\n",
+ dev_dbg(&p->pdev->dev, "unbind drc 0x%x complete\n",
p->drc_index);
- return rc == H_SUCCESS ? 0 : -ENXIO;
+ return;
}
static int papr_scm_meta_get(struct papr_scm_priv *p,
@@ -436,14 +430,16 @@ static int papr_scm_probe(struct platform_device *pdev)
rc = drc_pmem_bind(p);
/* If phyp says drc memory still bound then force unbound and retry */
- if (rc == -EBUSY) {
+ if (rc == H_OVERLAP) {
dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
drc_pmem_unbind(p);
rc = drc_pmem_bind(p);
}
- if (rc)
+ if (rc != H_SUCCESS) {
+ rc = -ENXIO;
goto err;
+ }
/* setup the resource for the newly bound range */
p->res.start = p->bound_addr;
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error
2019-08-29 6:33 [PATCH 1/2] powerpc/nvdimm: Use HCALL error as the return value Aneesh Kumar K.V
@ 2019-08-29 6:33 ` Aneesh Kumar K.V
2019-08-29 7:59 ` Oliver O'Halloran
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2019-08-29 6:33 UTC (permalink / raw)
To: mpe; +Cc: Vaibhav Jain, Oliver O'Halloran, linuxppc-dev,
Aneesh Kumar K.V
Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
This really slows down operations like kexec where we get the H_OVERLAP
error because we don't go through a full hypervisor re init.
H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
drc index is already bound. Since we don't specify a logical memory
address for bind hcall, we can use the H_SCM_QUERY hcall to query
the already bound logical address.
Boot time difference with and without patch is:
[ 5.583617] IOMMU table initialized, virtual merging enabled
[ 5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
[ 301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
[ 340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
after fix
[ 5.101572] IOMMU table initialized, virtual merging enabled
[ 5.116984] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Querying SCM details
[ 5.117223] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Querying SCM details
[ 5.120530] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux•ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 220e595cb579..4b74cfe7b334 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -110,6 +110,27 @@ static void drc_pmem_unbind(struct papr_scm_priv *p)
return;
}
+static int drc_pmem_query(struct papr_scm_priv *p)
+{
+ unsigned long ret[PLPAR_HCALL_BUFSIZE];
+ int64_t rc;
+
+
+ rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
+ p->drc_index, 0);
+
+ if (rc) {
+ dev_err(&p->pdev->dev, "Failed to bind SCM");
+ return rc;
+ }
+
+ p->bound_addr = ret[0];
+ dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
+
+ return 0;
+}
+
+
static int papr_scm_meta_get(struct papr_scm_priv *p,
struct nd_cmd_get_config_data_hdr *hdr)
{
@@ -431,9 +452,8 @@ static int papr_scm_probe(struct platform_device *pdev)
/* If phyp says drc memory still bound then force unbound and retry */
if (rc == H_OVERLAP) {
- dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
- drc_pmem_unbind(p);
- rc = drc_pmem_bind(p);
+ dev_warn(&pdev->dev, "Querying SCM details\n");
+ rc = drc_pmem_query(p);
}
if (rc != H_SUCCESS) {
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error
2019-08-29 6:33 ` [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error Aneesh Kumar K.V
@ 2019-08-29 7:59 ` Oliver O'Halloran
2019-08-29 8:21 ` Aneesh Kumar K.V
0 siblings, 1 reply; 5+ messages in thread
From: Oliver O'Halloran @ 2019-08-29 7:59 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, Vaibhav Jain
On Thu, Aug 29, 2019 at 4:34 PM Aneesh Kumar K.V
<aneesh.kumar@linux•ibm.com> wrote:
>
> Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
> This really slows down operations like kexec where we get the H_OVERLAP
> error because we don't go through a full hypervisor re init.
Maybe we should be unbinding it on a kexec().
> H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
> drc index is already bound. Since we don't specify a logical memory
> address for bind hcall, we can use the H_SCM_QUERY hcall to query
> the already bound logical address.
This is a little sketchy since we might have crashed during the
initial bind. Checking if the last block is bound to where we expect
it to be might be a good idea. If it's not where we expect it to be,
then an unbind->bind cycle is the only sane thing to do.
> Boot time difference with and without patch is:
>
> [ 5.583617] IOMMU table initialized, virtual merging enabled
> [ 5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
> [ 301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
> [ 340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
Is the unbind significantly slower than a bind? Or is the region here
just massive?
> after fix
>
> [ 5.101572] IOMMU table initialized, virtual merging enabled
> [ 5.116984] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Querying SCM details
> [ 5.117223] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Querying SCM details
> [ 5.120530] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux•ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++++++++++++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 220e595cb579..4b74cfe7b334 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -110,6 +110,27 @@ static void drc_pmem_unbind(struct papr_scm_priv *p)
> return;
> }
>
> +static int drc_pmem_query(struct papr_scm_priv *p)
> +{
> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
> + int64_t rc;
> +
> +
> + rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
> + p->drc_index, 0);
> +
> + if (rc) {
> + dev_err(&p->pdev->dev, "Failed to bind SCM");
> + return rc;
> + }
> +
> + p->bound_addr = ret[0];
> + dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
> +
> + return 0;
> +}
> +
> +
> static int papr_scm_meta_get(struct papr_scm_priv *p,
> struct nd_cmd_get_config_data_hdr *hdr)
> {
> @@ -431,9 +452,8 @@ static int papr_scm_probe(struct platform_device *pdev)
>
> /* If phyp says drc memory still bound then force unbound and retry */
> if (rc == H_OVERLAP) {
> - dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
> - drc_pmem_unbind(p);
> - rc = drc_pmem_bind(p);
> + dev_warn(&pdev->dev, "Querying SCM details\n");
That's a pretty vague message. If we're going to treat leaving the
region bound over kexec() as normal then you might want to bump it
down to pr_info() or so.
> + rc = drc_pmem_query(p);
> }
>
> if (rc != H_SUCCESS) {
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error
2019-08-29 7:59 ` Oliver O'Halloran
@ 2019-08-29 8:21 ` Aneesh Kumar K.V
2019-08-29 8:54 ` Oliver O'Halloran
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2019-08-29 8:21 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev, Vaibhav Jain
On 8/29/19 1:29 PM, Oliver O'Halloran wrote:
> On Thu, Aug 29, 2019 at 4:34 PM Aneesh Kumar K.V
> <aneesh.kumar@linux•ibm.com> wrote:
>>
>> Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
>> This really slows down operations like kexec where we get the H_OVERLAP
>> error because we don't go through a full hypervisor re init.
>
> Maybe we should be unbinding it on a kexec().
>
shouldn't ?
>> H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
>> drc index is already bound. Since we don't specify a logical memory
>> address for bind hcall, we can use the H_SCM_QUERY hcall to query
>> the already bound logical address.
>
> This is a little sketchy since we might have crashed during the
> initial bind. Checking if the last block is bound to where we expect
> it to be might be a good idea. If it's not where we expect it to be,
> then an unbind->bind cycle is the only sane thing to do.
>
I would not have expected hypervisor to not mark the drc index bound if
we failed the previous BIND request.
I can query start block and last block logical address and check whether
the full blocks is indeed mapped.
>> Boot time difference with and without patch is:
>>
>> [ 5.583617] IOMMU table initialized, virtual merging enabled
>> [ 5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
>> [ 301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
>> [ 340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
>
> Is the unbind significantly slower than a bind? Or is the region here
> just massive?
>
on unbind. We go two regions one of 60G and other of 10G
>> after fix
>>
>> [ 5.101572] IOMMU table initialized, virtual merging enabled
>> [ 5.116984] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Querying SCM details
>> [ 5.117223] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Querying SCM details
>> [ 5.120530] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux•ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 26 ++++++++++++++++++++---
>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 220e595cb579..4b74cfe7b334 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -110,6 +110,27 @@ static void drc_pmem_unbind(struct papr_scm_priv *p)
>> return;
>> }
>>
>> +static int drc_pmem_query(struct papr_scm_priv *p)
>> +{
>> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> + int64_t rc;
>> +
>> +
>> + rc = plpar_hcall(H_SCM_QUERY_BLOCK_MEM_BINDING, ret,
>> + p->drc_index, 0);
>> +
>> + if (rc) {
>> + dev_err(&p->pdev->dev, "Failed to bind SCM");
>> + return rc;
>> + }
>> +
>> + p->bound_addr = ret[0];
>> + dev_dbg(&p->pdev->dev, "bound drc 0x%x to %pR\n", p->drc_index, &p->res);
>> +
>> + return 0;
>> +}
>> +
>> +
>> static int papr_scm_meta_get(struct papr_scm_priv *p,
>> struct nd_cmd_get_config_data_hdr *hdr)
>> {
>> @@ -431,9 +452,8 @@ static int papr_scm_probe(struct platform_device *pdev)
>>
>> /* If phyp says drc memory still bound then force unbound and retry */
>> if (rc == H_OVERLAP) {
>> - dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
>> - drc_pmem_unbind(p);
>> - rc = drc_pmem_bind(p);
>
>> + dev_warn(&pdev->dev, "Querying SCM details\n");
>
> That's a pretty vague message. If we're going to treat leaving the
> region bound over kexec() as normal then you might want to bump it
> down to pr_info() or so.
sure.
>
>> + rc = drc_pmem_query(p);
>> }
>>
>> if (rc != H_SUCCESS) {
>> --
>> 2.21.0
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error
2019-08-29 8:21 ` Aneesh Kumar K.V
@ 2019-08-29 8:54 ` Oliver O'Halloran
0 siblings, 0 replies; 5+ messages in thread
From: Oliver O'Halloran @ 2019-08-29 8:54 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, Vaibhav Jain
On Thu, Aug 29, 2019 at 6:21 PM Aneesh Kumar K.V
<aneesh.kumar@linux•ibm.com> wrote:
>
> On 8/29/19 1:29 PM, Oliver O'Halloran wrote:
> > On Thu, Aug 29, 2019 at 4:34 PM Aneesh Kumar K.V
> > <aneesh.kumar@linux•ibm.com> wrote:
> >>
> >> Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
> >> This really slows down operations like kexec where we get the H_OVERLAP
> >> error because we don't go through a full hypervisor re init.
> >
> > Maybe we should be unbinding it on a kexec().
> >
>
> shouldn't ?
I mean in the previous kernel. We don't have a shutdown() method
defined for the driver so I figured it was leaving the bind intact
across kexec, hence the patch. I was thinking that we should have the
previous kernel unbind it rather than letting the kernel deal with the
H_OVERLAP error. I suppose we'll have to do that in kdump case anyway.
> >> H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
> >> drc index is already bound. Since we don't specify a logical memory
> >> address for bind hcall, we can use the H_SCM_QUERY hcall to query
> >> the already bound logical address.
> >
> > This is a little sketchy since we might have crashed during the
> > initial bind. Checking if the last block is bound to where we expect
> > it to be might be a good idea. If it's not where we expect it to be,
> > then an unbind->bind cycle is the only sane thing to do.
> >
>
> I would not have expected hypervisor to not mark the drc index bound if
> we failed the previous BIND request.
I was thinking of the partial-bind case where the driver started
binding, but never exits the loop in drc_pmem_bind() due to a kernel
panic or whatever.
> I can query start block and last block logical address and check whether
> the full blocks is indeed mapped.
>
>
> >> Boot time difference with and without patch is:
> >>
> >> [ 5.583617] IOMMU table initialized, virtual merging enabled
> >> [ 5.603041] papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Retrying bind after unbinding
> >> [ 301.514221] papr_scm ibm,persistent-memory:ibm,pmemory@44108001: Retrying bind after unbinding
> >> [ 340.057238] hv-24x7: read 1530 catalog entries, created 537 event attrs (0 failures), 275 descs
> >
> > Is the unbind significantly slower than a bind? Or is the region here
> > just massive?
> >
>
> on unbind. We go two regions one of 60G and other of 10G
Assuming you mean "it's slow on unbind" then that sounds like a
hypervisor bug tbh.
Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-29 9:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-29 6:33 [PATCH 1/2] powerpc/nvdimm: Use HCALL error as the return value Aneesh Kumar K.V
2019-08-29 6:33 ` [PATCH 2/2] powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error Aneesh Kumar K.V
2019-08-29 7:59 ` Oliver O'Halloran
2019-08-29 8:21 ` Aneesh Kumar K.V
2019-08-29 8:54 ` Oliver O'Halloran
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox