Re: [PATCH v3 net-next 01/23] lib: add reference counting tracking infrastructure

From: Andrzej Hajda <andrzej.hajda@intel.com>
To: Eric Dumazet <eric.dumazet@gmail.com>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Cc: netdev <netdev@vger.kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Thomas Gleixner <tglx@linutronix.de>, Greg KH <greg@kroah.com>,
	Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH v3 net-next 01/23] lib: add reference counting tracking infrastructure
Date: Wed, 8 Dec 2021 15:09:17 +0100	[thread overview]
Message-ID: <3d80c863-8f1d-5e94-44c8-cc1193cca06a@intel.com> (raw)
In-Reply-To: <20211205042217.982127-2-eric.dumazet@gmail.com>

Hi Eric,

I've spotted this patchset thanks to LWN, anyway it was merged very 
quickly, I think it missed more broader review.

As the patch touches kernel lib I have added few people who could be 
interested.

On 05.12.2021 05:21, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> It can be hard to track where references are taken and released.
>
> In networking, we have annoying issues at device or netns dismantles,
> and we had various proposals to ease root causing them.
>
> This patch adds new infrastructure pairing refcount increases
> and decreases. This will self document code, because programmers
> will have to associate increments/decrements.
>
> This is controled by CONFIG_REF_TRACKER which can be selected
> by users of this feature.
>
> This adds both cpu and memory costs, and thus should probably be
> used with care.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> ---

Life is surprising, I was working on my own framework, solving the same 
issue, with intention to publish it in few days :)

My approach was little different:

1. Instead of creating separate framework I have extended debug_objects.

2. There were no additional fields in refcounted object and trackers - 
infrastructure of debug_objects was reused - debug_objects tracked both 
pointers of refcounted object and its users.

Have you considered using debug_object? it seems to be good place to put 
it there, I am not sure about performance differences.

One more thing about design - as I understand CONFIG_REF_TRACKER turns 
on all trackers in whole kernel, have you considered possibility/helpers 
to enable/disable tracking per class of objects?

>   include/linux/ref_tracker.h |  73 +++++++++++++++++++
>   lib/Kconfig                 |   5 ++
>   lib/Makefile                |   2 +
>   lib/ref_tracker.c           | 140 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 220 insertions(+)
>   create mode 100644 include/linux/ref_tracker.h
>   create mode 100644 lib/ref_tracker.c
>
> diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..c11c9db5825cf933acf529c83db441a818135f29
> --- /dev/null
> +++ b/include/linux/ref_tracker.h
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#ifndef _LINUX_REF_TRACKER_H
> +#define _LINUX_REF_TRACKER_H
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +
> +struct ref_tracker;

With sth similar to:

#ifdef CONFIG_REF_TRACKER

typedef struct ref_tracker *ref_tracker_p;
#else
typedef struct {} ref_tracker_p;
#endif

you can eliminate unused field in user's structures.

Beside this it looks OK to me.

Regards

Andrzej

> +
> +struct ref_tracker_dir {
> +#ifdef CONFIG_REF_TRACKER
> +	spinlock_t		lock;
> +	unsigned int		quarantine_avail;
> +	refcount_t		untracked;
> +	struct list_head	list; /* List of active trackers */
> +	struct list_head	quarantine; /* List of dead trackers */
> +#endif
> +};
> +
> +#ifdef CONFIG_REF_TRACKER
> +static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
> +					unsigned int quarantine_count)
> +{
> +	INIT_LIST_HEAD(&dir->list);
> +	INIT_LIST_HEAD(&dir->quarantine);
> +	spin_lock_init(&dir->lock);
> +	dir->quarantine_avail = quarantine_count;
> +	refcount_set(&dir->untracked, 1);
> +}
> +
> +void ref_tracker_dir_exit(struct ref_tracker_dir *dir);
> +
> +void ref_tracker_dir_print(struct ref_tracker_dir *dir,
> +			   unsigned int display_limit);
> +
> +int ref_tracker_alloc(struct ref_tracker_dir *dir,
> +		      struct ref_tracker **trackerp, gfp_t gfp);
> +
> +int ref_tracker_free(struct ref_tracker_dir *dir,
> +		     struct ref_tracker **trackerp);
> +
> +#else /* CONFIG_REF_TRACKER */
> +
> +static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
> +					unsigned int quarantine_count)
> +{
> +}
> +
> +static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
> +{
> +}
> +
> +static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir,
> +					 unsigned int display_limit)
> +{
> +}
> +
> +static inline int ref_tracker_alloc(struct ref_tracker_dir *dir,
> +				    struct ref_tracker **trackerp,
> +				    gfp_t gfp)
> +{
> +	return 0;
> +}
> +
> +static inline int ref_tracker_free(struct ref_tracker_dir *dir,
> +				   struct ref_tracker **trackerp)
> +{
> +	return 0;
> +}
> +
> +#endif
> +
> +#endif /* _LINUX_REF_TRACKER_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 5e7165e6a346c9bec878b78c8c8c3d175fc98dfd..655b0e43f260bfca63240794191e3f1890b2e801 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -680,6 +680,11 @@ config STACK_HASH_ORDER
>   	 Select the hash size as a power of 2 for the stackdepot hash table.
>   	 Choose a lower value to reduce the memory impact.
>   
> +config REF_TRACKER
> +	bool
> +	depends on STACKTRACE_SUPPORT
> +	select STACKDEPOT
> +
>   config SBITMAP
>   	bool
>   
> diff --git a/lib/Makefile b/lib/Makefile
> index 364c23f1557816f73aebd8304c01224a4846ac6c..c1fd9243ddb9cc1ac5252d7eb8009f9290782c4a 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -270,6 +270,8 @@ obj-$(CONFIG_STACKDEPOT) += stackdepot.o
>   KASAN_SANITIZE_stackdepot.o := n
>   KCOV_INSTRUMENT_stackdepot.o := n
>   
> +obj-$(CONFIG_REF_TRACKER) += ref_tracker.o
> +
>   libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
>   	       fdt_empty_tree.o fdt_addresses.o
>   $(foreach file, $(libfdt_files), \
> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..0ae2e66dcf0fdb976f4cb99e747c9448b37f22cc
> --- /dev/null
> +++ b/lib/ref_tracker.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/export.h>
> +#include <linux/ref_tracker.h>
> +#include <linux/slab.h>
> +#include <linux/stacktrace.h>
> +#include <linux/stackdepot.h>
> +
> +#define REF_TRACKER_STACK_ENTRIES 16
> +
> +struct ref_tracker {
> +	struct list_head	head;   /* anchor into dir->list or dir->quarantine */
> +	bool			dead;
> +	depot_stack_handle_t	alloc_stack_handle;
> +	depot_stack_handle_t	free_stack_handle;
> +};
> +
> +void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
> +{
> +	struct ref_tracker *tracker, *n;
> +	unsigned long flags;
> +	bool leak = false;
> +
> +	spin_lock_irqsave(&dir->lock, flags);
> +	list_for_each_entry_safe(tracker, n, &dir->quarantine, head) {
> +		list_del(&tracker->head);
> +		kfree(tracker);
> +		dir->quarantine_avail++;
> +	}
> +	list_for_each_entry_safe(tracker, n, &dir->list, head) {
> +		pr_err("leaked reference.\n");
> +		if (tracker->alloc_stack_handle)
> +			stack_depot_print(tracker->alloc_stack_handle);
> +		leak = true;
> +		list_del(&tracker->head);
> +		kfree(tracker);
> +	}
> +	spin_unlock_irqrestore(&dir->lock, flags);
> +	WARN_ON_ONCE(leak);
> +	WARN_ON_ONCE(refcount_read(&dir->untracked) != 1);
> +}
> +EXPORT_SYMBOL(ref_tracker_dir_exit);
> +
> +void ref_tracker_dir_print(struct ref_tracker_dir *dir,
> +			   unsigned int display_limit)
> +{
> +	struct ref_tracker *tracker;
> +	unsigned long flags;
> +	unsigned int i = 0;
> +
> +	spin_lock_irqsave(&dir->lock, flags);
> +	list_for_each_entry(tracker, &dir->list, head) {
> +		if (i < display_limit) {
> +			pr_err("leaked reference.\n");
> +			if (tracker->alloc_stack_handle)
> +				stack_depot_print(tracker->alloc_stack_handle);
> +			i++;
> +		} else {
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&dir->lock, flags);
> +}
> +EXPORT_SYMBOL(ref_tracker_dir_print);
> +
> +int ref_tracker_alloc(struct ref_tracker_dir *dir,
> +		      struct ref_tracker **trackerp,
> +		      gfp_t gfp)
> +{
> +	unsigned long entries[REF_TRACKER_STACK_ENTRIES];
> +	struct ref_tracker *tracker;
> +	unsigned int nr_entries;
> +	unsigned long flags;
> +
> +	*trackerp = tracker = kzalloc(sizeof(*tracker), gfp | __GFP_NOFAIL);
> +	if (unlikely(!tracker)) {
> +		pr_err_once("memory allocation failure, unreliable refcount tracker.\n");
> +		refcount_inc(&dir->untracked);
> +		return -ENOMEM;
> +	}
> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1);
> +	nr_entries = filter_irq_stacks(entries, nr_entries);
> +	tracker->alloc_stack_handle = stack_depot_save(entries, nr_entries, gfp);
> +
> +	spin_lock_irqsave(&dir->lock, flags);
> +	list_add(&tracker->head, &dir->list);
> +	spin_unlock_irqrestore(&dir->lock, flags);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ref_tracker_alloc);
> +
> +int ref_tracker_free(struct ref_tracker_dir *dir,
> +		     struct ref_tracker **trackerp)
> +{
> +	unsigned long entries[REF_TRACKER_STACK_ENTRIES];
> +	struct ref_tracker *tracker = *trackerp;
> +	depot_stack_handle_t stack_handle;
> +	unsigned int nr_entries;
> +	unsigned long flags;
> +
> +	if (!tracker) {
> +		refcount_dec(&dir->untracked);
> +		return -EEXIST;
> +	}
> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1);
> +	nr_entries = filter_irq_stacks(entries, nr_entries);
> +	stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC);
> +
> +	spin_lock_irqsave(&dir->lock, flags);
> +	if (tracker->dead) {
> +		pr_err("reference already released.\n");
> +		if (tracker->alloc_stack_handle) {
> +			pr_err("allocated in:\n");
> +			stack_depot_print(tracker->alloc_stack_handle);
> +		}
> +		if (tracker->free_stack_handle) {
> +			pr_err("freed in:\n");
> +			stack_depot_print(tracker->free_stack_handle);
> +		}
> +		spin_unlock_irqrestore(&dir->lock, flags);
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}
> +	tracker->dead = true;
> +
> +	tracker->free_stack_handle = stack_handle;
> +
> +	list_move_tail(&tracker->head, &dir->quarantine);
> +	if (!dir->quarantine_avail) {
> +		tracker = list_first_entry(&dir->quarantine, struct ref_tracker, head);
> +		list_del(&tracker->head);
> +	} else {
> +		dir->quarantine_avail--;
> +		tracker = NULL;
> +	}
> +	spin_unlock_irqrestore(&dir->lock, flags);
> +
> +	kfree(tracker);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ref_tracker_free);