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

Groups > linux.kernel > #1620540 > unrolled thread

Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter.

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: WARN @lib/refcount.c:128 during hot unplug of I/O adapter. Tyrel Datwyler <tyreld@linux.vnet.ibm.com> - 2017-04-10 20:00 +0200
    Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter. Michael Ellerman <mpe@ellerman.id.au> - 2017-04-11 11:10 +0200
      Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter. Tyrel Datwyler <turtle.in.the.kernel@gmail.com> - 2017-04-11 19:20 +0200
        Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter. Michael Ellerman <mpe@ellerman.id.au> - 2017-04-12 04:20 +0200
          Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter. Tyrel Datwyler <turtle.in.the.kernel@gmail.com> - 2017-04-12 21:50 +0200

#1620540 — Re: WARN @lib/refcount.c:128 during hot unplug of I/O adapter.

On 04/06/2017 09:04 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> 
>> On 04/06/2017 03:27 AM, Sachin Sant wrote:
>>> On a POWER8 LPAR running 4.11.0-rc5, a hot unplug operation on
>>> any I/O adapter results in the following warning
>>>
>>> This problem has been in the code for some time now. I had first seen this in
>>> -next tree.
>>>

<snip>

>>> Have attached the dmesg log from the system. Let me know if any additional
>>> information is required to help debug this problem.
>>
>> I remember you mentioning this when the issue was brought up for CPUs. I
>> assume the case is the same here where the issue is only seen with
>> adapters that were hot-added after boot (ie. hot-remove of adapter
>> present at boot doesn't trip the warning)?
> 
> So who's fixing this?

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. I'll look into it again this week.

-Tyrel

> 
> cheers
> 

[toc] | [next] | [standalone]

#1621046

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

> On 04/06/2017 09:04 PM, Michael Ellerman wrote:
>> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>> 
>>> On 04/06/2017 03:27 AM, Sachin Sant wrote:
>>>> On a POWER8 LPAR running 4.11.0-rc5, a hot unplug operation on
>>>> any I/O adapter results in the following warning
>>>>
>>>> This problem has been in the code for some time now. I had first seen this in
>>>> -next tree.
>>>>
>
> <snip>
>
>>>> Have attached the dmesg log from the system. Let me know if any additional
>>>> information is required to help debug this problem.
>>>
>>> I remember you mentioning this when the issue was brought up for CPUs. I
>>> assume the case is the same here where the issue is only seen with
>>> adapters that were hot-added after boot (ie. hot-remove of adapter
>>> present at boot doesn't trip the warning)?
>> 
>> So who's fixing this?
>
> 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.

> I'll look into it again this week.

Thanks.

cheers

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

#1621485

On 04/11/2017 02:00 AM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
> 
>> On 04/06/2017 09:04 PM, Michael Ellerman wrote:
>>> Tyrel Datwyler <tyreld@linux.vnet.ibm.com> writes:
>>>
>>>> On 04/06/2017 03:27 AM, Sachin Sant wrote:
>>>>> On a POWER8 LPAR running 4.11.0-rc5, a hot unplug operation on
>>>>> any I/O adapter results in the following warning
>>>>>
>>>>> This problem has been in the code for some time now. I had first seen this in
>>>>> -next tree.
>>>>>
>>
>> <snip>
>>
>>>>> Have attached the dmesg log from the system. Let me know if any additional
>>>>> information is required to help debug this problem.
>>>>
>>>> I remember you mentioning this when the issue was brought up for CPUs. I
>>>> assume the case is the same here where the issue is only seen with
>>>> adapters that were hot-added after boot (ie. hot-remove of adapter
>>>> present at boot doesn't trip the warning)?
>>>
>>> So who's fixing this?
>>
>> 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.

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.

-Tyrel

> 
>> I'll look into it again this week.
> 
> Thanks.
> 
> cheers
> 

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

#1621747

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.

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
:)

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);
 

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

#1622502

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);
>  
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web