Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732872 > unrolled thread

Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Rob Herring <robh@kernel.org> - 2017-09-15 15:00 +0200
    Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Abdul Haleem <abdhalee@linux.vnet.ibm.com> - 2017-09-19 15:40 +0200
    Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Michael Ellerman <mpe@ellerman.id.au> - 2017-09-20 13:40 +0200
      Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-09-20 19:30 +0200
        Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Michael Ellerman <mpe@ellerman.id.au> - 2017-09-21 12:00 +0200
          Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-09-21 20:50 +0200
            Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus Michael Ellerman <mpe@ellerman.id.au> - 2017-09-22 14:00 +0200

#1732872 — Re: [mainline][DLPAR][Oops] OF: ERROR: Bad of_node_put() on /cpus

On Fri, Sep 15, 2017 at 6:04 AM, abdul <abdhalee@linux.vnet.ibm.com> wrote:
> Hi,
>
> Mainline kernel panics during DLPAR CPU add/remove operation.
>
> Machine Type: Power8 PowerVM LPAR
> kernel 4.13.0

Did 4.12 work or when was it last working? I'm not seeing anything
recent in the DT code that looks suspicious.

Rob

[toc] | [next] | [standalone]

#1734911

On Fri, 2017-09-15 at 07:52 -0500, Rob Herring wrote:
> On Fri, Sep 15, 2017 at 6:04 AM, abdul <abdhalee@linux.vnet.ibm.com> wrote:
> > Hi,
> >
> > Mainline kernel panics during DLPAR CPU add/remove operation.
> >
> > Machine Type: Power8 PowerVM LPAR
> > kernel 4.13.0
> 
> Did 4.12 work or when was it last working? I'm not seeing anything
> recent in the DT code that looks suspicious.

The issue was not seen with 4.12.0

-- 
Regard's

Abdul Haleem
IBM Linux Technology Centre

[toc] | [prev] | [next] | [standalone]

#1735730

Rob Herring <robh@kernel.org> writes:

> On Fri, Sep 15, 2017 at 6:04 AM, abdul <abdhalee@linux.vnet.ibm.com> wrote:
>> Hi,
>>
>> Mainline kernel panics during DLPAR CPU add/remove operation.
>>
>> Machine Type: Power8 PowerVM LPAR
>> kernel 4.13.0
>
> Did 4.12 work or when was it last working? I'm not seeing anything
> recent in the DT code that looks suspicious.

I'm pretty sure it's:

int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
	int rc;

	dn->parent = parent;

	rc = of_attach_node(dn);
	if (rc) {
		printk(KERN_ERR "Failed to add device node %pOF\n", dn);
		return rc;
	}

	of_node_put(dn->parent);
HERE    ^^^^^^^^^^

	return 0;
}


Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
dependency on full path"), we re-looked up the parent, and got another
reference on it. That meant the put before the return there was correct.
But now it's not because the caller has a reference to parent but it's
not ours to drop.

Testing a fix, will report back.

cheers

[toc] | [prev] | [next] | [standalone]

#1735960

On 09/20/2017 04:39 AM, Michael Ellerman wrote:
> Rob Herring <robh@kernel.org> writes:
> 
>> On Fri, Sep 15, 2017 at 6:04 AM, abdul <abdhalee@linux.vnet.ibm.com> wrote:
>>> Hi,
>>>
>>> Mainline kernel panics during DLPAR CPU add/remove operation.
>>>
>>> Machine Type: Power8 PowerVM LPAR
>>> kernel 4.13.0
>>
>> Did 4.12 work or when was it last working? I'm not seeing anything
>> recent in the DT code that looks suspicious.
> 
> I'm pretty sure it's:
> 
> int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
> {
> 	int rc;
> 
> 	dn->parent = parent;
> 
> 	rc = of_attach_node(dn);
> 	if (rc) {
> 		printk(KERN_ERR "Failed to add device node %pOF\n", dn);
> 		return rc;
> 	}
> 
> 	of_node_put(dn->parent);
> HERE    ^^^^^^^^^^
> 
> 	return 0;
> }
> 
> 
> Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
> dependency on full path"), we re-looked up the parent, and got another
> reference on it. That meant the put before the return there was correct.
> But now it's not because the caller has a reference to parent but it's
> not ours to drop.
> 
> Testing a fix, will report back.

So, that patch slipped past me. Not only is the parent reference not ours to drop, but
when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
"parent" reference until after dlpar_attach_node() returns.

-Tyrel

> 
> cheers
> 

[toc] | [prev] | [next] | [standalone]

#1736509

Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> On 09/20/2017 04:39 AM, Michael Ellerman wrote:
>> Rob Herring <robh@kernel.org> writes:
>>> On Fri, Sep 15, 2017 at 6:04 AM, abdul <abdhalee@linux.vnet.ibm.com> wrote:
>>>>
>>>> Mainline kernel panics during DLPAR CPU add/remove operation.
>>>>
>>>> Machine Type: Power8 PowerVM LPAR
>>>> kernel 4.13.0
>>>
>>> Did 4.12 work or when was it last working? I'm not seeing anything
>>> recent in the DT code that looks suspicious.
>> 
>> I'm pretty sure it's:
>> 
>> int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
>> {
>> 	int rc;
>> 
>> 	dn->parent = parent;
>> 
>> 	rc = of_attach_node(dn);
>> 	if (rc) {
>> 		printk(KERN_ERR "Failed to add device node %pOF\n", dn);
>> 		return rc;
>> 	}
>> 
>> 	of_node_put(dn->parent);
>> HERE    ^^^^^^^^^^
>> 
>> 	return 0;
>> }
>> 
>> 
>> Prior to 215ee763f8cb ("powerpc: pseries: remove dlpar_attach_node
>> dependency on full path"), we re-looked up the parent, and got another
>> reference on it. That meant the put before the return there was correct.
>> But now it's not because the caller has a reference to parent but it's
>> not ours to drop.
>> 
>> Testing a fix, will report back.
>
> So, that patch slipped past me. Not only is the parent reference not ours to drop, but
> when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
> the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
> dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
> "parent" reference until after dlpar_attach_node() returns.

Yep. I wrote the same patch :)

Rob asked me to test it, which I did, but /cpus starts out with an
elevated ref count, so you have to do ~30 (on my system) DLPAR removes
to hit the bug, which I didn't do.

I've updated my test script to do roughly $(nproc) x 10 DLPAR removes,
which is hopefully sufficient to catch these bugs in future.

cheers

[toc] | [prev] | [next] | [standalone]

#1737005

On 09/21/2017 02:57 AM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>> On 09/20/2017 04:39 AM, Michael Ellerman wrote:
>>> Rob Herring <robh@kernel.org> writes:

<snip>

>>>
>>> Testing a fix, will report back.
>>
>> So, that patch slipped past me. Not only is the parent reference not ours to drop, but
>> when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
>> the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
>> dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
>> "parent" reference until after dlpar_attach_node() returns.
> 
> Yep. I wrote the same patch :)
> 
> Rob asked me to test it, which I did, but /cpus starts out with an
> elevated ref count, so you have to do ~30 (on my system) DLPAR removes
> to hit the bug, which I didn't do.

Yeah, there are a lot of things that grab references to /cpus. So, I had a good idea that
I needed to loop a few times adding and removing multiple cpus to trigger the issue. Its
also obvious when using those OF trace points I wrote a while back that refcount for /cpus
is dropping off uncharacteristically in response to symmetrical adds/removes of cpus. I
saw your note about getting that patchset resubmitted. I'll try and get that queued back
up soon.

-Tyrel

> 
> I've updated my test script to do roughly $(nproc) x 10 DLPAR removes,
> which is hopefully sufficient to catch these bugs in future.
> 
> cheers
> 

[toc] | [prev] | [next] | [standalone]

#1737432

Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:

> On 09/21/2017 02:57 AM, Michael Ellerman wrote:
>> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>>> On 09/20/2017 04:39 AM, Michael Ellerman wrote:
>>>> Rob Herring <robh@kernel.org> writes:
>
> <snip>
>
>>>>
>>>> Testing a fix, will report back.
>>>
>>> So, that patch slipped past me. Not only is the parent reference not ours to drop, but
>>> when I went and looked at dlpar_cpu_add() I also noticed that of_node_put() was done on
>>> the parent prior to the call to dlpar_attach_node(). With the addition of "parent" to the
>>> dlpar_attach_node() parameter list dlpar_cpu_add() needs to be fixed up to hold the
>>> "parent" reference until after dlpar_attach_node() returns.
>> 
>> Yep. I wrote the same patch :)
>> 
>> Rob asked me to test it, which I did, but /cpus starts out with an
>> elevated ref count, so you have to do ~30 (on my system) DLPAR removes
>> to hit the bug, which I didn't do.
>
> Yeah, there are a lot of things that grab references to /cpus. So, I had a good idea that
> I needed to loop a few times adding and removing multiple cpus to trigger the issue. Its
> also obvious when using those OF trace points I wrote a while back that refcount for /cpus
> is dropping off uncharacteristically in response to symmetrical adds/removes of cpus. I
> saw your note about getting that patchset resubmitted. I'll try and get that queued back
> up soon.

Thanks, it'd be great to get it in. I applied it from the list and used
it for testing this.

cheers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web