From: Tyrel Datwyler <turtle.in.the.kernel@gmail.com>
To: Michael Ellerman <mpe@ellerman.id.au>,
Tyrel Datwyler <tyreld@linux.vnet.ibm.com>,
Sachin Sant <sachinp@linux.vnet.ibm.com>,
linuxppc-dev@ozlabs.org
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter.
Date: Wed, 12 Apr 2017 12:41:20 -0700 [thread overview]
Message-ID: <f288cb35-1dea-f7e2-c7a7-6cb9c0d062f3@gmail.com> (raw)
In-Reply-To: <87efwyv0j7.fsf@concordia.ellerman.id.au>
On 04/11/2017 07:10 PM, Michael Ellerman wrote:
> Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
>> On 04/11/2017 02:00 AM, Michael Ellerman wrote:
>>> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>>>> I started looking at it when Bharata submitted a patch trying to fix the
>>>> issue for CPUs, but got side tracked by other things. I suspect that
>>>> this underflow has actually been an issue for quite some time, and we
>>>> are just now becoming aware of it thanks to the recount_t patchset being
>>>> merged.
>>>
>>> Yes I agree. Which means it might be broken in existing distros.
>>
>> Definitely. I did some profiling last night, and I understand the
>> hotplug case. It turns out to be as I suggested in the original thread
>> about CPUs. When the devicetree code was worked to move the tree out of
>> proc and into sysfs the sysfs detach code added a of_node_put to remove
>> the original of_init reference. pSeries Being the sole original
>> *dynamic* device tree user we had always issued a of_node_put in our
>> dlpar specific detach function to achieve that end. So, this should be a
>> pretty straight forward trivial fix.
>
> Excellent, thanks.
>
>> However, for the case where devices are present at boot it appears we a
>> leaking a lot of references resulting in the device nodes never actually
>> being released/freed after a dlpar remove. In the CPU case after boot I
>> count 8 more references taken than the hotplug case, and corresponding
>> of_node_put's are not called at dlpar remove time either. That will take
>> some time to track them down, review and clean up.
I found our reference leak. In topology_init() we call register_cpu()
for each possible logical cpu id. For any logical cpu present a
reference to the device node of the cpu core is grabbed and added to
cpu->dev.of_node. Which matches what I'm seeing on a Power8 lpar, 8
extraneous references which is equal to the 8 hardware threads of a core.
>
> Yes that is a perennial problem unfortunately which we've never come up
> with a good solution for.
>
> The (old) patch below might help track some of them down. I remember
> having a script to process the output of the trace and find mismatches,
> but I can't find it right now - but I'm sure you can hack up something
> :)
Haha, this patch is almost identical to what I hacked up Monday to get
an idea of where the refcounts were at. Probably wouldn't hurt to try
and upstream it into the driver/of tree.
-Tyrel
>
> cheers
>
>
> diff --git a/arch/powerpc/include/asm/trace.h b/arch/powerpc/include/asm/trace.h
> index 32e36b16773f..ad32365082a0 100644
> --- a/arch/powerpc/include/asm/trace.h
> +++ b/arch/powerpc/include/asm/trace.h
> @@ -168,6 +168,44 @@ TRACE_EVENT(hash_fault,
> __entry->addr, __entry->access, __entry->trap)
> );
>
> +TRACE_EVENT(of_node_get,
> +
> + TP_PROTO(struct device_node *dn, int val),
> +
> + TP_ARGS(dn, val),
> +
> + TP_STRUCT__entry(
> + __field(struct device_node *, dn)
> + __field(int, val)
> + ),
> +
> + TP_fast_assign(
> + __entry->dn = dn;
> + __entry->val = val;
> + ),
> +
> + TP_printk("get %d -> %d %s", __entry->val - 1, __entry->val, __entry->dn->full_name)
> +);
> +
> +TRACE_EVENT(of_node_put,
> +
> + TP_PROTO(struct device_node *dn, int val),
> +
> + TP_ARGS(dn, val),
> +
> + TP_STRUCT__entry(
> + __field(struct device_node *, dn)
> + __field(int, val)
> + ),
> +
> + TP_fast_assign(
> + __entry->dn = dn;
> + __entry->val = val;
> + ),
> +
> + TP_printk("put %d -> %d %s", __entry->val + 1, __entry->val, __entry->dn->full_name)
> +);
> +
> #endif /* _TRACE_POWERPC_H */
>
> #undef TRACE_INCLUDE_PATH
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index c647bd1b6903..f5c3d761f3cd 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -14,6 +14,8 @@
>
> #include "of_private.h"
>
> +#include <asm/trace.h>
> +
> /**
> * of_node_get() - Increment refcount of a node
> * @node: Node to inc refcount, NULL is supported to simplify writing of
> @@ -23,8 +25,12 @@
> */
> struct device_node *of_node_get(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_get(&node->kobj);
> +
> + trace_of_node_get(node, atomic_read(&node->kobj.kref.refcount));
> + }
> +
> return node;
> }
> EXPORT_SYMBOL(of_node_get);
> @@ -36,8 +42,10 @@ EXPORT_SYMBOL(of_node_get);
> */
> void of_node_put(struct device_node *node)
> {
> - if (node)
> + if (node) {
> kobject_put(&node->kobj);
> + trace_of_node_put(node, atomic_read(&node->kobj.kref.refcount));
> + }
> }
> EXPORT_SYMBOL(of_node_put);
>
>
next prev parent reply other threads:[~2017-04-12 19:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-06 10:27 WARN @lib/refcount.c:128 during hot unplug of I/O adapter Sachin Sant
2017-04-06 20:44 ` Tyrel Datwyler
2017-04-07 4:04 ` Michael Ellerman
2017-04-10 17:53 ` Tyrel Datwyler
2017-04-11 9:00 ` Michael Ellerman
2017-04-11 17:14 ` Tyrel Datwyler
2017-04-12 2:10 ` Michael Ellerman
2017-04-12 19:41 ` Tyrel Datwyler [this message]
2017-04-07 4:38 ` Sachin Sant
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=f288cb35-1dea-f7e2-c7a7-6cb9c0d062f3@gmail.com \
--to=turtle.in.the.kernel@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=nfont@linux.vnet.ibm.com \
--cc=sachinp@linux.vnet.ibm.com \
--cc=tyreld@linux.vnet.ibm.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).