[PATCH RFC 0/2] Simplify detection of resource leaks

[PATCH RFC 0/2] Simplify detection of resource leaks

[Florian Bezdeka]

Hi all,

over the last few days we had to search for and identify several
resource leaks in one of our internal applications. Here are some patches 
that were written to help us tracking the leaks down to the root cause.

In our case /proc/xenomai/registry/usage reported that all available
slots are in use but we had now clue why slots are exhausted at all.

Patch 1 adds /proc/xenomai/registry/dump. That allowed us to get a
overview of registry usage. We easily identified that the application
was leaking message queues after looking at the output.

Patch 2 adds trace events. These allowed us to identify the thread that
was creating such queues. The inballance of registry_enter and
registry_{unlink,remove} got visible.

I would like to learn if:
 - these patches are correct (especially the first one)
 - these patches are in general interest

I have backports to -stable available as well. I could send them out
easily if -stable is of interest as well.

Best regards,
Florian

Florian Bezdeka (2):
  cobalt/registry: Add a new virtual /proc file for dumping registry
  cobalt/registry: Adding trace events for
    registry_{enter,unlink,remove}

 kernel/cobalt/registry.c          | 40 +++++++++++++++++++++++++++++++
 kernel/cobalt/trace/cobalt-core.h | 33 +++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

-- 
2.30.2

[PATCH RFC 1/2] cobalt/registry: Add a new virtual /proc file for dumping registry

[Florian Bezdeka]

Introducing /proc/xenomai/registry/dump which could be accessed by
something like "cat" to dump the current registry. Like the "usage"
file it is available when CONFIG_XENO_OPT_VFILE is enabled.

The main purpose of this file is providing some additional help when
trying to find resource leaks triggered by Xenomai based applications.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 kernel/cobalt/registry.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/kernel/cobalt/registry.c b/kernel/cobalt/registry.c
index 816102f7a..8befa795c 100644
--- a/kernel/cobalt/registry.c
+++ b/kernel/cobalt/registry.c
@@ -80,6 +80,23 @@ static int usage_vfile_show(struct xnvfile_regular_iterator *it, void *data)
 	return 0;
 }
 
+static int dump_vfile_show(struct xnvfile_regular_iterator *it, void *data)
+{
+	struct xnobject *elem;
+	struct hlist_node *next;
+	int n;
+
+	xnvfile_printf(it, "nr_object_entries: %d\n", nr_object_entries);
+	xnvfile_printf(it, "obj_idx[0]: \treserved\n");
+
+	for (n = 0; n < nr_object_entries; n++)
+		hlist_for_each_entry_safe(elem, next, &object_index[n], hlink)
+			xnvfile_printf(it, "obj_idx[%d]:\tobj=%p, key=%s\n", n,
+				       elem->objaddr, elem->key ?: "(anon)");
+
+	return 0;
+}
+
 static struct xnvfile_regular_ops usage_vfile_ops = {
 	.show = usage_vfile_show,
 };
@@ -88,6 +105,14 @@ static struct xnvfile_regular usage_vfile = {
 	.ops = &usage_vfile_ops,
 };
 
+static struct xnvfile_regular_ops dump_vfile_ops = {
+	.show = dump_vfile_show,
+};
+
+static struct xnvfile_regular dump_vfile = {
+	.ops = &dump_vfile_ops,
+};
+
 #endif /* CONFIG_XENO_OPT_VFILE */
 
 unsigned xnregistry_hash_size(void)
@@ -124,8 +149,16 @@ int xnregistry_init(void)
 		return ret;
 	}
 
+	ret = xnvfile_init_regular("dump", &dump_vfile, &registry_vfroot);
+	if (ret) {
+		xnvfile_destroy_regular(&usage_vfile);
+		xnvfile_destroy_dir(&registry_vfroot);
+		return ret;
+	}
+
 	proc_virq = pipeline_create_inband_sirq(registry_proc_schedule);
 	if (proc_virq < 0) {
+		xnvfile_destroy_regular(&dump_vfile);
 		xnvfile_destroy_regular(&usage_vfile);
 		xnvfile_destroy_dir(&registry_vfroot);
 		return proc_virq;
@@ -149,6 +182,7 @@ int xnregistry_init(void)
 
 	if (object_index == NULL) {
 #ifdef CONFIG_XENO_OPT_VFILE
+		xnvfile_destroy_regular(&dump_vfile);
 		xnvfile_destroy_regular(&usage_vfile);
 		xnvfile_destroy_dir(&registry_vfroot);
 		pipeline_delete_inband_sirq(proc_virq);
@@ -199,6 +233,7 @@ void xnregistry_cleanup(void)
 #ifdef CONFIG_XENO_OPT_VFILE
 	pipeline_delete_inband_sirq(proc_virq);
 	flush_scheduled_work();
+	xnvfile_destroy_regular(&dump_vfile);
 	xnvfile_destroy_regular(&usage_vfile);
 	xnvfile_destroy_dir(&registry_vfroot);
 #endif /* CONFIG_XENO_OPT_VFILE */
-- 
2.30.2

[PATCH RFC 2/2] cobalt/registry: Adding trace events for registry_{enter, unlink, remove}

[Florian Bezdeka]

Adding the following trace events to make registry-usage traceable:
  - registry_enter   (object gets added to the registry)
  - registry_unlink  (object is made anonymous)
  - registry_remove  (object is removed from the registry)

These trace events are helpful if there is a need to track down
resource leaks triggered by an Xenomai based application.

Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
---
 kernel/cobalt/registry.c          |  5 +++++
 kernel/cobalt/trace/cobalt-core.h | 33 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/kernel/cobalt/registry.c b/kernel/cobalt/registry.c
index 8befa795c..c20c4cc1a 100644
--- a/kernel/cobalt/registry.c
+++ b/kernel/cobalt/registry.c
@@ -685,6 +685,7 @@ int xnregistry_enter(const char *key, void *objaddr,
 	nr_active_objects++;
 	object->objaddr = objaddr;
 	object->cstamp = ++next_object_stamp;
+	trace_cobalt_registry_enter(key, objaddr);
 #ifdef CONFIG_XENO_OPT_VFILE
 	object->pnode = NULL;
 #endif
@@ -866,6 +867,8 @@ int xnregistry_remove(xnhandle_t handle)
 		goto unlock_and_exit;
 	}
 
+	trace_cobalt_registry_remove(object->key, object->objaddr);
+
 	objaddr = object->objaddr;
 	object->objaddr = NULL;
 	object->cstamp = 0;
@@ -932,6 +935,8 @@ int xnregistry_unlink(const char *key)
 		goto unlock_and_exit;
 	}
 
+	trace_cobalt_registry_unlink(object->key, object->objaddr);
+
 	ret = registry_hash_remove(object);
 	if (ret < 0)
 		goto unlock_and_exit;
diff --git a/kernel/cobalt/trace/cobalt-core.h b/kernel/cobalt/trace/cobalt-core.h
index 88ed5700d..d98787c5a 100644
--- a/kernel/cobalt/trace/cobalt-core.h
+++ b/kernel/cobalt/trace/cobalt-core.h
@@ -26,6 +26,7 @@
 #include <linux/tracepoint.h>
 #include <linux/math64.h>
 #include <cobalt/kernel/timer.h>
+#include <cobalt/kernel/registry.h>
 #include <cobalt/uapi/kernel/types.h>
 
 struct xnsched;
@@ -149,6 +150,23 @@ DECLARE_EVENT_CLASS(timer_event,
 	TP_printk("timer=%p", __entry->timer)
 );
 
+DECLARE_EVENT_CLASS(registry_event,
+	TP_PROTO(const char *key, void *addr),
+	TP_ARGS(key, addr),
+
+	TP_STRUCT__entry(
+		__string(key, key ?: "(anon)")
+		__field(void *, addr)
+	),
+
+	TP_fast_assign(
+		__assign_str(key, key ?: "(anon)");
+		__entry->addr = addr;
+	),
+
+	TP_printk("key=%s, addr=%p", __get_str(key), __entry->addr)
+);
+
 TRACE_EVENT(cobalt_schedule,
 	TP_PROTO(struct xnsched *sched),
 	TP_ARGS(sched),
@@ -778,6 +796,21 @@ DEFINE_EVENT(synch_post_event, cobalt_synch_forget,
 	TP_ARGS(synch)
 );
 
+DEFINE_EVENT(registry_event, cobalt_registry_enter,
+	TP_PROTO(const char *key, void *addr),
+	TP_ARGS(key, addr)
+);
+
+DEFINE_EVENT(registry_event, cobalt_registry_remove,
+	TP_PROTO(const char *key, void *addr),
+	TP_ARGS(key, addr)
+);
+
+DEFINE_EVENT(registry_event, cobalt_registry_unlink,
+	TP_PROTO(const char *key, void *addr),
+	TP_ARGS(key, addr)
+);
+
 TRACE_EVENT(cobalt_tick_shot,
 	TP_PROTO(s64 delta),
 	TP_ARGS(delta),
-- 
2.30.2

Re: [PATCH RFC 1/2] cobalt/registry: Add a new virtual /proc file for dumping registry

[Jan Kiszka]

On 16.08.21 18:12, Florian Bezdeka wrote:
> Introducing /proc/xenomai/registry/dump which could be accessed by
> something like "cat" to dump the current registry. Like the "usage"
> file it is available when CONFIG_XENO_OPT_VFILE is enabled.
> 
> The main purpose of this file is providing some additional help when
> trying to find resource leaks triggered by Xenomai based applications.
> 
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  kernel/cobalt/registry.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/kernel/cobalt/registry.c b/kernel/cobalt/registry.c
> index 816102f7a..8befa795c 100644
> --- a/kernel/cobalt/registry.c
> +++ b/kernel/cobalt/registry.c
> @@ -80,6 +80,23 @@ static int usage_vfile_show(struct xnvfile_regular_iterator *it, void *data)
>  	return 0;
>  }
>  
> +static int dump_vfile_show(struct xnvfile_regular_iterator *it, void *data)
> +{
> +	struct xnobject *elem;
> +	struct hlist_node *next;
> +	int n;
> +
> +	xnvfile_printf(it, "nr_object_entries: %d\n", nr_object_entries);
> +	xnvfile_printf(it, "obj_idx[0]: \treserved\n");
> +
> +	for (n = 0; n < nr_object_entries; n++)
> +		hlist_for_each_entry_safe(elem, next, &object_index[n], hlink)
> +			xnvfile_printf(it, "obj_idx[%d]:\tobj=%p, key=%s\n", n,
> +				       elem->objaddr, elem->key ?: "(anon)");
> +
> +	return 0;
> +}
> +
>  static struct xnvfile_regular_ops usage_vfile_ops = {
>  	.show = usage_vfile_show,
>  };
> @@ -88,6 +105,14 @@ static struct xnvfile_regular usage_vfile = {
>  	.ops = &usage_vfile_ops,
>  };
>  
> +static struct xnvfile_regular_ops dump_vfile_ops = {
> +	.show = dump_vfile_show,
> +};
> +
> +static struct xnvfile_regular dump_vfile = {
> +	.ops = &dump_vfile_ops,
> +};
> +
>  #endif /* CONFIG_XENO_OPT_VFILE */
>  
>  unsigned xnregistry_hash_size(void)
> @@ -124,8 +149,16 @@ int xnregistry_init(void)
>  		return ret;
>  	}
>  
> +	ret = xnvfile_init_regular("dump", &dump_vfile, &registry_vfroot);
> +	if (ret) {
> +		xnvfile_destroy_regular(&usage_vfile);
> +		xnvfile_destroy_dir(&registry_vfroot);
> +		return ret;
> +	}
> +
>  	proc_virq = pipeline_create_inband_sirq(registry_proc_schedule);
>  	if (proc_virq < 0) {
> +		xnvfile_destroy_regular(&dump_vfile);
>  		xnvfile_destroy_regular(&usage_vfile);
>  		xnvfile_destroy_dir(&registry_vfroot);
>  		return proc_virq;
> @@ -149,6 +182,7 @@ int xnregistry_init(void)
>  
>  	if (object_index == NULL) {
>  #ifdef CONFIG_XENO_OPT_VFILE
> +		xnvfile_destroy_regular(&dump_vfile);
>  		xnvfile_destroy_regular(&usage_vfile);
>  		xnvfile_destroy_dir(&registry_vfroot);
>  		pipeline_delete_inband_sirq(proc_virq);
> @@ -199,6 +233,7 @@ void xnregistry_cleanup(void)
>  #ifdef CONFIG_XENO_OPT_VFILE
>  	pipeline_delete_inband_sirq(proc_virq);
>  	flush_scheduled_work();
> +	xnvfile_destroy_regular(&dump_vfile);
>  	xnvfile_destroy_regular(&usage_vfile);
>  	xnvfile_destroy_dir(&registry_vfroot);
>  #endif /* CONFIG_XENO_OPT_VFILE */
> 

We need something like this, I'm just wondering if we shouldn't make the
missing information available in a similar way like the already
published one (drivers/ipc), also to avoid duplicating things this way.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH RFC 2/2] cobalt/registry: Adding trace events for registry_{enter,unlink,remove}

[Jan Kiszka]

On 16.08.21 18:12, Florian Bezdeka wrote:
> Adding the following trace events to make registry-usage traceable:
>   - registry_enter   (object gets added to the registry)
>   - registry_unlink  (object is made anonymous)
>   - registry_remove  (object is removed from the registry)
> 
> These trace events are helpful if there is a need to track down
> resource leaks triggered by an Xenomai based application.
> 
> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
> ---
>  kernel/cobalt/registry.c          |  5 +++++
>  kernel/cobalt/trace/cobalt-core.h | 33 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/kernel/cobalt/registry.c b/kernel/cobalt/registry.c
> index 8befa795c..c20c4cc1a 100644
> --- a/kernel/cobalt/registry.c
> +++ b/kernel/cobalt/registry.c
> @@ -685,6 +685,7 @@ int xnregistry_enter(const char *key, void *objaddr,
>  	nr_active_objects++;
>  	object->objaddr = objaddr;
>  	object->cstamp = ++next_object_stamp;
> +	trace_cobalt_registry_enter(key, objaddr);
>  #ifdef CONFIG_XENO_OPT_VFILE
>  	object->pnode = NULL;
>  #endif
> @@ -866,6 +867,8 @@ int xnregistry_remove(xnhandle_t handle)
>  		goto unlock_and_exit;
>  	}
>  
> +	trace_cobalt_registry_remove(object->key, object->objaddr);
> +
>  	objaddr = object->objaddr;
>  	object->objaddr = NULL;
>  	object->cstamp = 0;
> @@ -932,6 +935,8 @@ int xnregistry_unlink(const char *key)
>  		goto unlock_and_exit;
>  	}
>  
> +	trace_cobalt_registry_unlink(object->key, object->objaddr);
> +
>  	ret = registry_hash_remove(object);
>  	if (ret < 0)
>  		goto unlock_and_exit;
> diff --git a/kernel/cobalt/trace/cobalt-core.h b/kernel/cobalt/trace/cobalt-core.h
> index 88ed5700d..d98787c5a 100644
> --- a/kernel/cobalt/trace/cobalt-core.h
> +++ b/kernel/cobalt/trace/cobalt-core.h
> @@ -26,6 +26,7 @@
>  #include <linux/tracepoint.h>
>  #include <linux/math64.h>
>  #include <cobalt/kernel/timer.h>
> +#include <cobalt/kernel/registry.h>
>  #include <cobalt/uapi/kernel/types.h>
>  
>  struct xnsched;
> @@ -149,6 +150,23 @@ DECLARE_EVENT_CLASS(timer_event,
>  	TP_printk("timer=%p", __entry->timer)
>  );
>  
> +DECLARE_EVENT_CLASS(registry_event,
> +	TP_PROTO(const char *key, void *addr),
> +	TP_ARGS(key, addr),
> +
> +	TP_STRUCT__entry(
> +		__string(key, key ?: "(anon)")
> +		__field(void *, addr)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(key, key ?: "(anon)");
> +		__entry->addr = addr;
> +	),
> +
> +	TP_printk("key=%s, addr=%p", __get_str(key), __entry->addr)
> +);
> +
>  TRACE_EVENT(cobalt_schedule,
>  	TP_PROTO(struct xnsched *sched),
>  	TP_ARGS(sched),
> @@ -778,6 +796,21 @@ DEFINE_EVENT(synch_post_event, cobalt_synch_forget,
>  	TP_ARGS(synch)
>  );
>  
> +DEFINE_EVENT(registry_event, cobalt_registry_enter,
> +	TP_PROTO(const char *key, void *addr),
> +	TP_ARGS(key, addr)
> +);
> +
> +DEFINE_EVENT(registry_event, cobalt_registry_remove,
> +	TP_PROTO(const char *key, void *addr),
> +	TP_ARGS(key, addr)
> +);
> +
> +DEFINE_EVENT(registry_event, cobalt_registry_unlink,
> +	TP_PROTO(const char *key, void *addr),
> +	TP_ARGS(key, addr)
> +);
> +
>  TRACE_EVENT(cobalt_tick_shot,
>  	TP_PROTO(s64 delta),
>  	TP_ARGS(delta),
> 

Thanks, applied to next.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH RFC 2/2] cobalt/registry: Adding trace events for registry_{enter,unlink,remove}

[Jan Kiszka]

On 17.08.21 08:09, Jan Kiszka via Xenomai wrote:
> On 16.08.21 18:12, Florian Bezdeka wrote:
>> Adding the following trace events to make registry-usage traceable:
>>   - registry_enter   (object gets added to the registry)
>>   - registry_unlink  (object is made anonymous)
>>   - registry_remove  (object is removed from the registry)
>>
>> These trace events are helpful if there is a need to track down
>> resource leaks triggered by an Xenomai based application.
>>
>> Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
>> ---
>>  kernel/cobalt/registry.c          |  5 +++++
>>  kernel/cobalt/trace/cobalt-core.h | 33 +++++++++++++++++++++++++++++++
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/kernel/cobalt/registry.c b/kernel/cobalt/registry.c
>> index 8befa795c..c20c4cc1a 100644
>> --- a/kernel/cobalt/registry.c
>> +++ b/kernel/cobalt/registry.c
>> @@ -685,6 +685,7 @@ int xnregistry_enter(const char *key, void *objaddr,
>>  	nr_active_objects++;
>>  	object->objaddr = objaddr;
>>  	object->cstamp = ++next_object_stamp;
>> +	trace_cobalt_registry_enter(key, objaddr);
>>  #ifdef CONFIG_XENO_OPT_VFILE
>>  	object->pnode = NULL;
>>  #endif
>> @@ -866,6 +867,8 @@ int xnregistry_remove(xnhandle_t handle)
>>  		goto unlock_and_exit;
>>  	}
>>  
>> +	trace_cobalt_registry_remove(object->key, object->objaddr);
>> +
>>  	objaddr = object->objaddr;
>>  	object->objaddr = NULL;
>>  	object->cstamp = 0;
>> @@ -932,6 +935,8 @@ int xnregistry_unlink(const char *key)
>>  		goto unlock_and_exit;
>>  	}
>>  
>> +	trace_cobalt_registry_unlink(object->key, object->objaddr);
>> +
>>  	ret = registry_hash_remove(object);
>>  	if (ret < 0)
>>  		goto unlock_and_exit;
>> diff --git a/kernel/cobalt/trace/cobalt-core.h b/kernel/cobalt/trace/cobalt-core.h
>> index 88ed5700d..d98787c5a 100644
>> --- a/kernel/cobalt/trace/cobalt-core.h
>> +++ b/kernel/cobalt/trace/cobalt-core.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/tracepoint.h>
>>  #include <linux/math64.h>
>>  #include <cobalt/kernel/timer.h>
>> +#include <cobalt/kernel/registry.h>
>>  #include <cobalt/uapi/kernel/types.h>
>>  
>>  struct xnsched;
>> @@ -149,6 +150,23 @@ DECLARE_EVENT_CLASS(timer_event,
>>  	TP_printk("timer=%p", __entry->timer)
>>  );
>>  
>> +DECLARE_EVENT_CLASS(registry_event,
>> +	TP_PROTO(const char *key, void *addr),
>> +	TP_ARGS(key, addr),
>> +
>> +	TP_STRUCT__entry(
>> +		__string(key, key ?: "(anon)")
>> +		__field(void *, addr)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__assign_str(key, key ?: "(anon)");
>> +		__entry->addr = addr;
>> +	),
>> +
>> +	TP_printk("key=%s, addr=%p", __get_str(key), __entry->addr)
>> +);
>> +
>>  TRACE_EVENT(cobalt_schedule,
>>  	TP_PROTO(struct xnsched *sched),
>>  	TP_ARGS(sched),
>> @@ -778,6 +796,21 @@ DEFINE_EVENT(synch_post_event, cobalt_synch_forget,
>>  	TP_ARGS(synch)
>>  );
>>  
>> +DEFINE_EVENT(registry_event, cobalt_registry_enter,
>> +	TP_PROTO(const char *key, void *addr),
>> +	TP_ARGS(key, addr)
>> +);
>> +
>> +DEFINE_EVENT(registry_event, cobalt_registry_remove,
>> +	TP_PROTO(const char *key, void *addr),
>> +	TP_ARGS(key, addr)
>> +);
>> +
>> +DEFINE_EVENT(registry_event, cobalt_registry_unlink,
>> +	TP_PROTO(const char *key, void *addr),
>> +	TP_ARGS(key, addr)
>> +);
>> +
>>  TRACE_EVENT(cobalt_tick_shot,
>>  	TP_PROTO(s64 delta),
>>  	TP_ARGS(delta),
>>
> 
> Thanks, applied to next.
> 
> Jan
> 

Fixed up a missing include in registry.c.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux