netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir
@ 2025-04-18 14:24 Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output Jeff Layton
                   ` (7 more replies)
  0 siblings, 8 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton, Thomas Weißschuh

This version should be pretty close to merge-ready. The only real
difference is the use of NAME_MAX as the field width for on-stack
sprintf buffers.

I left the Reviewed-bys intact. Let me know if that's an issue and
we can drop them.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v4:
- Drop patch to widen ref_tracker_dir_.name, use NAME_MAX+1 (256) instead since this only affects dentry name
- Link to v3: https://lore.kernel.org/r/20250417-reftrack-dbgfs-v3-0-c3159428c8fb@kernel.org

Changes in v3:
- don't overwrite dir->name in ref_tracker_dir_debugfs
- define REF_TRACKER_NAMESZ and use it when setting name
- Link to v2: https://lore.kernel.org/r/20250415-reftrack-dbgfs-v2-0-b18c4abd122f@kernel.org

Changes in v2:
- Add patch to do %pK -> %p conversion in ref_tracker.c
- Pass in output function to pr_ostream() instead of if statement
- Widen ref_tracker_dir.name to 64 bytes to accomodate unique names
- Eliminate error handling with debugfs manipulation
- Incorporate pointer value into netdev name
- Link to v1: https://lore.kernel.org/r/20250414-reftrack-dbgfs-v1-0-f03585832203@kernel.org

---
Jeff Layton (7):
      ref_tracker: don't use %pK in pr_ostream() output
      ref_tracker: add a top level debugfs directory for ref_tracker
      ref_tracker: have callers pass output function to pr_ostream()
      ref_tracker: allow pr_ostream() to print directly to a seq_file
      ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir
      net: add ref_tracker_dir_debugfs() calls for netns refcount tracking
      net: register debugfs file for net_device refcnt tracker

 include/linux/ref_tracker.h |  13 ++++
 lib/ref_tracker.c           | 151 +++++++++++++++++++++++++++++++++++++++-----
 net/core/dev.c              |   6 +-
 net/core/net_namespace.c    |  34 +++++++++-
 4 files changed, 187 insertions(+), 17 deletions(-)
---
base-commit: 695caca9345a160ecd9645abab8e70cfe849e9ff
change-id: 20250413-reftrack-dbgfs-3767b303e2fa

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-23 23:46   ` Jakub Kicinski
  2025-04-18 14:24 ` [PATCH v4 2/7] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton, Thomas Weißschuh

As Thomas Weißschuh points out [1], it is now preferable to use %p
instead of hashed pointers with printk(), since raw pointers should no
longer be leaked into the kernel log. Change the ref_tracker
infrastructure to use %p instead of %pK in its formats.

[1]: https://lore.kernel.org/netdev/20250414-restricted-pointers-net-v1-0-12af0ce46cdd@linutronix.de/

Cc: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Reviewed-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index cf5609b1ca79361763abe5a3a98484a3ee591ff2..de71439e12a3bab6456910986fa611dfbdd97980 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -96,7 +96,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
 
 	stats = ref_tracker_get_stats(dir, display_limit);
 	if (IS_ERR(stats)) {
-		pr_ostream(s, "%s@%pK: couldn't get stats, error %pe\n",
+		pr_ostream(s, "%s@%p: couldn't get stats, error %pe\n",
 			   dir->name, dir, stats);
 		return;
 	}
@@ -107,13 +107,13 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
 		stack = stats->stacks[i].stack_handle;
 		if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4))
 			sbuf[0] = 0;
-		pr_ostream(s, "%s@%pK has %d/%d users at\n%s\n", dir->name, dir,
+		pr_ostream(s, "%s@%p has %d/%d users at\n%s\n", dir->name, dir,
 			   stats->stacks[i].count, stats->total, sbuf);
 		skipped -= stats->stacks[i].count;
 	}
 
 	if (skipped)
-		pr_ostream(s, "%s@%pK skipped reports about %d/%d users.\n",
+		pr_ostream(s, "%s@%p skipped reports about %d/%d users.\n",
 			   dir->name, dir, skipped, stats->total);
 
 	kfree(sbuf);

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 2/7] ref_tracker: add a top level debugfs directory for ref_tracker
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 3/7] ref_tracker: have callers pass output function to pr_ostream() Jeff Layton
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

Add a new "ref_tracker" directory in debugfs. Each individual refcount
tracker can register files under there to display info about
currently-held references.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index de71439e12a3bab6456910986fa611dfbdd97980..a66cde325920780cfe7529ea9d827d0ee943318d 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -12,6 +12,8 @@
 #define REF_TRACKER_STACK_ENTRIES 16
 #define STACK_BUF_SIZE 1024
 
+static struct dentry *ref_tracker_debug_dir = (struct dentry *)-ENOENT;
+
 struct ref_tracker {
 	struct list_head	head;   /* anchor into dir->list or dir->quarantine */
 	bool			dead;
@@ -273,3 +275,17 @@ int ref_tracker_free(struct ref_tracker_dir *dir,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ref_tracker_free);
+
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+
+static int __init ref_tracker_debugfs_init(void)
+{
+	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
+	if (IS_ERR(ref_tracker_debug_dir))
+		pr_warn("ref_tracker: unable to create debugfs ref_tracker directory: %pe\n",
+			ref_tracker_debug_dir);
+	return 0;
+}
+late_initcall(ref_tracker_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 3/7] ref_tracker: have callers pass output function to pr_ostream()
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 2/7] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 4/7] ref_tracker: allow pr_ostream() to print directly to a seq_file Jeff Layton
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

In a later patch, we'll be adding a 3rd mechanism for outputting
ref_tracker info via seq_file. Instead of a conditional, have the caller
set a pointer to an output function in struct ostream. As part of this,
the log prefix must be explicitly passed in, as it's too late for the
pr_fmt macro.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 53 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index a66cde325920780cfe7529ea9d827d0ee943318d..b6e0a87dd75eddef4d504419c0cf398ea65c19d8 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -64,22 +64,40 @@ ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit)
 	return stats;
 }
 
+#define __ostream_printf __printf(2, 3)
+
 struct ostream {
+	void __ostream_printf (*func)(struct ostream *stream, char *fmt, ...);
+	char *prefix;
 	char *buf;
 	int size, used;
 };
 
+static void __ostream_printf pr_ostream_log(struct ostream *stream, char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	vprintk(fmt, args);
+	va_end(args);
+}
+
+static void __ostream_printf pr_ostream_buf(struct ostream *stream, char *fmt, ...)
+{
+	int ret, len = stream->size - stream->used;
+	va_list args;
+
+	va_start(args, fmt);
+	ret = vsnprintf(stream->buf + stream->used, len, fmt, args);
+	va_end(args);
+	stream->used += min(ret, len);
+}
+
 #define pr_ostream(stream, fmt, args...) \
 ({ \
 	struct ostream *_s = (stream); \
 \
-	if (!_s->buf) { \
-		pr_err(fmt, ##args); \
-	} else { \
-		int ret, len = _s->size - _s->used; \
-		ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt), ##args); \
-		_s->used += min(ret, len); \
-	} \
+	_s->func(_s, fmt, ##args); \
 })
 
 static void
@@ -98,8 +116,8 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
 
 	stats = ref_tracker_get_stats(dir, display_limit);
 	if (IS_ERR(stats)) {
-		pr_ostream(s, "%s@%p: couldn't get stats, error %pe\n",
-			   dir->name, dir, stats);
+		pr_ostream(s, "%s%s@%p: couldn't get stats, error %pe\n",
+			   s->prefix, dir->name, dir, stats);
 		return;
 	}
 
@@ -109,14 +127,15 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
 		stack = stats->stacks[i].stack_handle;
 		if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4))
 			sbuf[0] = 0;
-		pr_ostream(s, "%s@%p has %d/%d users at\n%s\n", dir->name, dir,
-			   stats->stacks[i].count, stats->total, sbuf);
+		pr_ostream(s, "%s%s@%p has %d/%d users at\n%s\n", s->prefix,
+			   dir->name, dir, stats->stacks[i].count,
+			   stats->total, sbuf);
 		skipped -= stats->stacks[i].count;
 	}
 
 	if (skipped)
-		pr_ostream(s, "%s@%p skipped reports about %d/%d users.\n",
-			   dir->name, dir, skipped, stats->total);
+		pr_ostream(s, "%s%s@%p skipped reports about %d/%d users.\n",
+			   s->prefix, dir->name, dir, skipped, stats->total);
 
 	kfree(sbuf);
 
@@ -126,7 +145,8 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir,
 void ref_tracker_dir_print_locked(struct ref_tracker_dir *dir,
 				  unsigned int display_limit)
 {
-	struct ostream os = {};
+	struct ostream os = { .func = pr_ostream_log,
+			      .prefix = "ref_tracker: " };
 
 	__ref_tracker_dir_pr_ostream(dir, display_limit, &os);
 }
@@ -145,7 +165,10 @@ EXPORT_SYMBOL(ref_tracker_dir_print);
 
 int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size)
 {
-	struct ostream os = { .buf = buf, .size = size };
+	struct ostream os = { .func = pr_ostream_buf,
+			      .prefix = "ref_tracker: ",
+			      .buf = buf,
+			      .size = size };
 	unsigned long flags;
 
 	spin_lock_irqsave(&dir->lock, flags);

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 4/7] ref_tracker: allow pr_ostream() to print directly to a seq_file
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
                   ` (2 preceding siblings ...)
  2025-04-18 14:24 ` [PATCH v4 3/7] ref_tracker: have callers pass output function to pr_ostream() Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 5/7] ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir Jeff Layton
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

Allow pr_ostream to also output directly to a seq_file without an
intermediate buffer.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 lib/ref_tracker.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index b6e0a87dd75eddef4d504419c0cf398ea65c19d8..4857bcb6d4bf557a0089f51328e75e8209e959e6 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -8,6 +8,7 @@
 #include <linux/slab.h>
 #include <linux/stacktrace.h>
 #include <linux/stackdepot.h>
+#include <linux/seq_file.h>
 
 #define REF_TRACKER_STACK_ENTRIES 16
 #define STACK_BUF_SIZE 1024
@@ -70,6 +71,7 @@ struct ostream {
 	void __ostream_printf (*func)(struct ostream *stream, char *fmt, ...);
 	char *prefix;
 	char *buf;
+	struct seq_file *seq;
 	int size, used;
 };
 
@@ -93,6 +95,15 @@ static void __ostream_printf pr_ostream_buf(struct ostream *stream, char *fmt, .
 	stream->used += min(ret, len);
 }
 
+static void __ostream_printf pr_ostream_seq(struct ostream *stream, char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	seq_vprintf(stream->seq, fmt, args);
+	va_end(args);
+}
+
 #define pr_ostream(stream, fmt, args...) \
 ({ \
 	struct ostream *_s = (stream); \
@@ -302,6 +313,20 @@ EXPORT_SYMBOL_GPL(ref_tracker_free);
 #ifdef CONFIG_DEBUG_FS
 #include <linux/debugfs.h>
 
+static int ref_tracker_dir_seq_print(struct ref_tracker_dir *dir, struct seq_file *seq)
+{
+	struct ostream os = { .func = pr_ostream_seq,
+			      .prefix = "",
+			      .seq = seq };
+	unsigned long flags;
+
+	spin_lock_irqsave(&dir->lock, flags);
+	__ref_tracker_dir_pr_ostream(dir, 16, &os);
+	spin_unlock_irqrestore(&dir->lock, flags);
+
+	return os.used;
+}
+
 static int __init ref_tracker_debugfs_init(void)
 {
 	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 5/7] ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
                   ` (3 preceding siblings ...)
  2025-04-18 14:24 ` [PATCH v4 4/7] ref_tracker: allow pr_ostream() to print directly to a seq_file Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 6/7] net: add ref_tracker_dir_debugfs() calls for netns refcount tracking Jeff Layton
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

Currently, there is no convenient way to see the info that the
ref_tracking infrastructure collects. Add a new function that other
subsystems can optionally call to update the name field in the
ref_tracker_dir and register a corresponding seq_file for it in the
top-level ref_tracker directory.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/ref_tracker.h | 13 +++++++++++
 lib/ref_tracker.c           | 57 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h
index 8eac4f3d52547ccbaf9dcd09962ce80d26fbdff8..77a55a32c067216fa02ba349498f53bd289aee0c 100644
--- a/include/linux/ref_tracker.h
+++ b/include/linux/ref_tracker.h
@@ -5,6 +5,7 @@
 #include <linux/types.h>
 #include <linux/spinlock.h>
 #include <linux/stackdepot.h>
+#include <linux/seq_file.h>
 
 struct ref_tracker;
 
@@ -17,6 +18,9 @@ struct ref_tracker_dir {
 	bool			dead;
 	struct list_head	list; /* List of active trackers */
 	struct list_head	quarantine; /* List of dead trackers */
+#ifdef CONFIG_DEBUG_FS
+	struct dentry		*dentry;
+#endif
 	char			name[32];
 #endif
 };
@@ -34,10 +38,15 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
 	dir->dead = false;
 	refcount_set(&dir->untracked, 1);
 	refcount_set(&dir->no_tracker, 1);
+#ifdef CONFIG_DEBUG_FS
+	dir->dentry = NULL;
+#endif
 	strscpy(dir->name, name, sizeof(dir->name));
 	stack_depot_init();
 }
 
+void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name);
+
 void ref_tracker_dir_exit(struct ref_tracker_dir *dir);
 
 void ref_tracker_dir_print_locked(struct ref_tracker_dir *dir,
@@ -62,6 +71,10 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
 {
 }
 
+static inline void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
+{
+}
+
 static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
 {
 }
diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index 4857bcb6d4bf557a0089f51328e75e8209e959e6..d53105b959c122a85558cbe84551726f6429e08e 100644
--- a/lib/ref_tracker.c
+++ b/lib/ref_tracker.c
@@ -31,6 +31,14 @@ struct ref_tracker_dir_stats {
 	} stacks[];
 };
 
+#ifdef CONFIG_DEBUG_FS
+static void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir);
+#else
+static inline void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir)
+{
+}
+#endif
+
 static struct ref_tracker_dir_stats *
 ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit)
 {
@@ -197,6 +205,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir)
 	bool leak = false;
 
 	dir->dead = true;
+	ref_tracker_debugfs_remove(dir);
 	spin_lock_irqsave(&dir->lock, flags);
 	list_for_each_entry_safe(tracker, n, &dir->quarantine, head) {
 		list_del(&tracker->head);
@@ -327,6 +336,54 @@ static int ref_tracker_dir_seq_print(struct ref_tracker_dir *dir, struct seq_fil
 	return os.used;
 }
 
+static int ref_tracker_debugfs_show(struct seq_file *f, void *v)
+{
+	struct ref_tracker_dir *dir = f->private;
+
+	return ref_tracker_dir_seq_print(dir, f);
+}
+
+static int ref_tracker_debugfs_open(struct inode *inode, struct file *filp)
+{
+	struct ref_tracker_dir *dir = inode->i_private;
+
+	return single_open(filp, ref_tracker_debugfs_show, dir);
+}
+
+static const struct file_operations ref_tracker_debugfs_fops = {
+	.owner		= THIS_MODULE,
+	.open		= ref_tracker_debugfs_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+/**
+ * ref_tracker_dir_debugfs - create debugfs file for ref_tracker_dir
+ * @dir: ref_tracker_dir to finalize
+ * @name: updated name of the ref_tracker_dir
+ *
+ * In some cases, the name given to a ref_tracker_dir is based on incomplete information,
+ * and may not be unique. Call this to finalize the name of @dir, and create a debugfs
+ * file for it. If the name is not unique, a warning will be emitted but it is not fatal
+ * to the tracker.
+ */
+void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
+{
+	dir->dentry = debugfs_create_file(name, S_IFREG | 0400,
+					  ref_tracker_debug_dir, dir,
+					  &ref_tracker_debugfs_fops);
+	if (IS_ERR(dir->dentry))
+		pr_warn("ref_tracker: unable to create debugfs file for %s: %pe\n",
+			name, dir->dentry);
+}
+EXPORT_SYMBOL(ref_tracker_dir_debugfs);
+
+static void ref_tracker_debugfs_remove(struct ref_tracker_dir *dir)
+{
+	debugfs_remove(dir->dentry);
+}
+
 static int __init ref_tracker_debugfs_init(void)
 {
 	ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 6/7] net: add ref_tracker_dir_debugfs() calls for netns refcount tracking
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
                   ` (4 preceding siblings ...)
  2025-04-18 14:24 ` [PATCH v4 5/7] ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-18 14:24 ` [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker Jeff Layton
  2025-04-23 23:44 ` [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jakub Kicinski
  7 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

After assigning the inode number to the namespace, use it to create a
unique name for each netns refcount tracker and register the debugfs
files for them.

init_net is registered before the ref_tracker dir is created, so add a
late_initcall() to register its files.

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/core/net_namespace.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 4303f2a4926243e2c0ff0c0387383cd8e0658019..f35c51f1eddacf3e84cdd51d77dee9f1d6dc4f87 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -761,12 +761,44 @@ struct net *get_net_ns_by_pid(pid_t pid)
 }
 EXPORT_SYMBOL_GPL(get_net_ns_by_pid);
 
+#ifdef CONFIG_NET_NS_REFCNT_TRACKER
+static void net_ns_net_debugfs(struct net *net)
+{
+	char name[NAME_MAX + 1];
+	size_t len;
+
+	len = snprintf(name, sizeof(name), "netns-%u-refcnt", net->ns.inum);
+	if (len < sizeof(name))
+		ref_tracker_dir_debugfs(&net->refcnt_tracker, name);
+
+	len = snprintf(name, sizeof(name), "netns-%u-notrefcnt", net->ns.inum);
+	if (len < sizeof(name))
+		ref_tracker_dir_debugfs(&net->notrefcnt_tracker, name);
+}
+
+static int __init init_net_debugfs(void)
+{
+	net_ns_net_debugfs(&init_net);
+	return 0;
+}
+late_initcall(init_net_debugfs);
+#else
+static void net_ns_net_debugfs(struct net *net)
+{
+}
+#endif
+
 static __net_init int net_ns_net_init(struct net *net)
 {
+	int ret;
+
 #ifdef CONFIG_NET_NS
 	net->ns.ops = &netns_operations;
 #endif
-	return ns_alloc_inum(&net->ns);
+	ret = ns_alloc_inum(&net->ns);
+	if (!ret)
+		net_ns_net_debugfs(net);
+	return ret;
 }
 
 static __net_exit void net_ns_net_exit(struct net *net)

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
                   ` (5 preceding siblings ...)
  2025-04-18 14:24 ` [PATCH v4 6/7] net: add ref_tracker_dir_debugfs() calls for netns refcount tracking Jeff Layton
@ 2025-04-18 14:24 ` Jeff Layton
  2025-04-23 23:53   ` Jakub Kicinski
  2025-04-23 23:44 ` [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jakub Kicinski
  7 siblings, 1 reply; 22+ messages in thread
From: Jeff Layton @ 2025-04-18 14:24 UTC (permalink / raw)
  To: Andrew Morton, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman
  Cc: Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor, Andrew Lunn,
	linux-kernel, netdev, Jeff Layton

As a nearly-final step in register_netdevice(), finalize the name in the
refcount tracker, and register a debugfs file for it.

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 net/core/dev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2f7f5fd9ffec7c0fc219eb6ba57d57a55134186e..377a19240f2b774281088d91e7da3e4b6ec52e33 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10834,8 +10834,9 @@ static void netdev_free_phy_link_topology(struct net_device *dev)
  */
 int register_netdevice(struct net_device *dev)
 {
-	int ret;
 	struct net *net = dev_net(dev);
+	char name[NAME_MAX + 1];
+	int ret;
 
 	BUILD_BUG_ON(sizeof(netdev_features_t) * BITS_PER_BYTE <
 		     NETDEV_FEATURE_COUNT);
@@ -10994,6 +10995,9 @@ int register_netdevice(struct net_device *dev)
 	    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
 		rtmsg_ifinfo(RTM_NEWLINK, dev, ~0U, GFP_KERNEL, 0, NULL);
 
+	/* Register debugfs file for the refcount tracker */
+	if (snprintf(name, sizeof(name), "netdev-%s@%p", dev->name, dev) < sizeof(name))
+		ref_tracker_dir_debugfs(&dev->refcnt_tracker, name);
 out:
 	return ret;
 

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir
  2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
                   ` (6 preceding siblings ...)
  2025-04-18 14:24 ` [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker Jeff Layton
@ 2025-04-23 23:44 ` Jakub Kicinski
  2025-04-23 23:48   ` Jeff Layton
  7 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-23 23:44 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev, Thomas Weißschuh

On Fri, 18 Apr 2025 10:24:24 -0400 Jeff Layton wrote:
> This version should be pretty close to merge-ready. The only real
> difference is the use of NAME_MAX as the field width for on-stack
> sprintf buffers.

Merge via which tree?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output
  2025-04-18 14:24 ` [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output Jeff Layton
@ 2025-04-23 23:46   ` Jakub Kicinski
  2025-04-23 23:56     ` Jeff Layton
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-23 23:46 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev, Thomas Weißschuh

On Fri, 18 Apr 2025 10:24:25 -0400 Jeff Layton wrote:
> -		pr_ostream(s, "%s@%pK: couldn't get stats, error %pe\n",
> +		pr_ostream(s, "%s@%p: couldn't get stats, error %pe\n",

FTR I think this is counter productive. 

Reference tracking is a debug feature and you may want to find that
object with a debugger and inspect it. That's not the same as random
drivers printing kernel addresses into logs for not apparent reason.

Does kmemleak use the hashed address representation? It's a very
similar situation.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir
  2025-04-23 23:44 ` [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jakub Kicinski
@ 2025-04-23 23:48   ` Jeff Layton
  2025-04-23 23:56     ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff Layton @ 2025-04-23 23:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev, Thomas Weißschuh

On Wed, 2025-04-23 at 16:44 -0700, Jakub Kicinski wrote:
> On Fri, 18 Apr 2025 10:24:24 -0400 Jeff Layton wrote:
> > This version should be pretty close to merge-ready. The only real
> > difference is the use of NAME_MAX as the field width for on-stack
> > sprintf buffers.
> 
> Merge via which tree?

Good Q. get_maintainer.pl says Andrew owns the ref_tracker code:

$ ./scripts/get_maintainer.pl lib/ref_tracker.c
Andrew Morton <akpm@linux-foundation.org> (maintainer:LIBRARY CODE)
linux-kernel@vger.kernel.org (open list:LIBRARY CODE)
LIBRARY CODE status: Supported

...but I think he ends up owning anything without an explicit
maintainer.

Eric Dumazet wrote the ref_tracker originally though, and the new files
are only added for networking stuff. If you wanted to pick it up, I
doubt Andrew would mind.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-18 14:24 ` [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker Jeff Layton
@ 2025-04-23 23:53   ` Jakub Kicinski
  2025-04-24  0:04     ` Jeff Layton
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-23 23:53 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev

On Fri, 18 Apr 2025 10:24:31 -0400 Jeff Layton wrote:
> +	/* Register debugfs file for the refcount tracker */
> +	if (snprintf(name, sizeof(name), "netdev-%s@%p", dev->name, dev) < sizeof(name))
> +		ref_tracker_dir_debugfs(&dev->refcnt_tracker, name);

Names are not unique and IIUC debugfs is not namespaced.
How much naming the objects in a "user readable" fashion actually
matter? It'd be less churn to create some kind of "object class"
with a directory level named after what's already passed to
ref_tracker_dir_init() and then id the objects by the pointer value 
as sub-dirs of that?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output
  2025-04-23 23:46   ` Jakub Kicinski
@ 2025-04-23 23:56     ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-23 23:56 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev, Thomas Weißschuh

On Wed, 2025-04-23 at 16:46 -0700, Jakub Kicinski wrote:
> On Fri, 18 Apr 2025 10:24:25 -0400 Jeff Layton wrote:
> > -		pr_ostream(s, "%s@%pK: couldn't get stats, error %pe\n",
> > +		pr_ostream(s, "%s@%p: couldn't get stats, error %pe\n",
> 
> FTR I think this is counter productive. 
> 
> Reference tracking is a debug feature and you may want to find that
> object with a debugger and inspect it. That's not the same as random
> drivers printing kernel addresses into logs for not apparent reason.
>
> Does kmemleak use the hashed address representation? It's a very
> similar situation.

Looking at mm/kmemleak.c, I think it doesn't. It looks like it casts
the pointers to unsigned long and prints them with %08lx:

        warn_or_seq_printf(seq, "unreferenced object%s 0x%08lx (size %zu):\n",
                           __object_type_str(object),
                           object->pointer, object->size);

I'm not sure if that's any different from %px though. I can make either
change in the name formats if that's preferred.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir
  2025-04-23 23:48   ` Jeff Layton
@ 2025-04-23 23:56     ` Jakub Kicinski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-23 23:56 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev, Thomas Weißschuh

On Wed, 23 Apr 2025 19:48:29 -0400 Jeff Layton wrote:
> On Wed, 2025-04-23 at 16:44 -0700, Jakub Kicinski wrote:
> > On Fri, 18 Apr 2025 10:24:24 -0400 Jeff Layton wrote:  
> > > This version should be pretty close to merge-ready. The only real
> > > difference is the use of NAME_MAX as the field width for on-stack
> > > sprintf buffers.  
> > 
> > Merge via which tree?  
> 
> Good Q. get_maintainer.pl says Andrew owns the ref_tracker code:
> 
> $ ./scripts/get_maintainer.pl lib/ref_tracker.c
> Andrew Morton <akpm@linux-foundation.org> (maintainer:LIBRARY CODE)
> linux-kernel@vger.kernel.org (open list:LIBRARY CODE)
> LIBRARY CODE status: Supported
> 
> ...but I think he ends up owning anything without an explicit
> maintainer.
> 
> Eric Dumazet wrote the ref_tracker originally though, and the new files
> are only added for networking stuff. If you wanted to pick it up, I
> doubt Andrew would mind.

My moderate excitement about the churn in the core networking core
aside :) - also no strong preferences on the tree. Looks like the
patches don't apply to net next:

Applying: net: register debugfs file for net_device refcnt tracker
Using index info to reconstruct a base tree...
M	net/core/dev.c
Falling back to patching base and 3-way merge...
Auto-merging net/core/dev.c

so we could save a conflict by rebasing and routing them via the
networking trees?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-23 23:53   ` Jakub Kicinski
@ 2025-04-24  0:04     ` Jeff Layton
  2025-04-24  0:32       ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff Layton @ 2025-04-24  0:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev

On Wed, 2025-04-23 at 16:53 -0700, Jakub Kicinski wrote:
> On Fri, 18 Apr 2025 10:24:31 -0400 Jeff Layton wrote:
> > +	/* Register debugfs file for the refcount tracker */
> > +	if (snprintf(name, sizeof(name), "netdev-%s@%p", dev->name, dev) < sizeof(name))
> > +		ref_tracker_dir_debugfs(&dev->refcnt_tracker, name);
> 
> Names are not unique and IIUC debugfs is not namespaced.
> How much naming the objects in a "user readable" fashion actually
> matter? It'd be less churn to create some kind of "object class"
> with a directory level named after what's already passed to
> ref_tracker_dir_init() and then id the objects by the pointer value 
> as sub-dirs of that?

That sounds closer to what I had done originally. Andrew L. suggested
the flat directory that this version represents. I'm fine with whatever
hierarchy, but let's decide that before I respin again.

When I was tracking down net namespace leaks recently, it was very nice
to have the inode number of the leaked netns's in the filenames. I
would have probably had to grovel around with drgn to figure out the
address if that weren't embedded in the name. I think we probably ought
to leave it up to each subsystem how it names its files. The
discriminators between different types of objects can vary wildly.

One thing that might be simpler is to make ref_tracker_dir_debugfs() a
varargs function and allow passing in a format string and set of
arguments for it. That might make things simpler for the callers.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24  0:04     ` Jeff Layton
@ 2025-04-24  0:32       ` Jakub Kicinski
  2025-04-24 10:56         ` Jeff Layton
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-24  0:32 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev

On Wed, 23 Apr 2025 20:04:58 -0400 Jeff Layton wrote:
> On Wed, 2025-04-23 at 16:53 -0700, Jakub Kicinski wrote:
> > Names are not unique and IIUC debugfs is not namespaced.
> > How much naming the objects in a "user readable" fashion actually
> > matter? It'd be less churn to create some kind of "object class"
> > with a directory level named after what's already passed to
> > ref_tracker_dir_init() and then id the objects by the pointer value 
> > as sub-dirs of that?  
> 
> That sounds closer to what I had done originally. Andrew L. suggested
> the flat directory that this version represents. I'm fine with whatever
> hierarchy, but let's decide that before I respin again.

Sorry about that :(

> When I was tracking down net namespace leaks recently, it was very nice
> to have the inode number of the leaked netns's in the filenames. I
> would have probably had to grovel around with drgn to figure out the
> address if that weren't embedded in the name. I think we probably ought
> to leave it up to each subsystem how it names its files. The
> discriminators between different types of objects can vary wildly.
> 
> One thing that might be simpler is to make ref_tracker_dir_debugfs() a
> varargs function and allow passing in a format string and set of
> arguments for it. That might make things simpler for the callers.

Yes, cutting out the formatting in the callers would definitely 
be a win. Maybe that'd make the whole thing sufficiently palatable :)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24  0:32       ` Jakub Kicinski
@ 2025-04-24 10:56         ` Jeff Layton
  2025-04-24 12:10           ` Andrew Lunn
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff Layton @ 2025-04-24 10:56 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	Andrew Lunn, linux-kernel, netdev

On Wed, 2025-04-23 at 17:32 -0700, Jakub Kicinski wrote:
> On Wed, 23 Apr 2025 20:04:58 -0400 Jeff Layton wrote:
> > On Wed, 2025-04-23 at 16:53 -0700, Jakub Kicinski wrote:
> > > Names are not unique and IIUC debugfs is not namespaced.

Correct, debugfs is not namespaced.

I meant to say earlier that I'm open to suggestions on how to make the
netdev names unique. Low-level netdev stuff is not my area of
expertise. We can drop this patch if doing so is problematic.

> > > How much naming the objects in a "user readable" fashion actually
> > > matter? It'd be less churn to create some kind of "object class"
> > > with a directory level named after what's already passed to
> > > ref_tracker_dir_init() and then id the objects by the pointer value 
> > > as sub-dirs of that?  
> > 
> > That sounds closer to what I had done originally. Andrew L. suggested
> > the flat directory that this version represents. I'm fine with whatever
> > hierarchy, but let's decide that before I respin again.
> 
> Sorry about that :(
> 

No worries...but we do need to decide what this directory hierarchy
should look like.

Andrew's point earlier was that this is just debugfs, so a flat
"ref_tracker" directory full of files is fine. I tend to agree with
him; NAME_MAX is 255, so we have plenty of room to make uniquely-named
files.

We could build a dir hierarchy though. Something like:

- ref_tracker
    + netdev
    + netns
    
...and then each object class subdir would hold uniquely-named files.
Each subsys would need to create the directory and then the files
within it, but we could add helpers for that.

Note that this is debugfs, so we can always change that later as it's
not counted as part of ABI.

Which way should we do it?
 
> > When I was tracking down net namespace leaks recently, it was very nice
> > to have the inode number of the leaked netns's in the filenames. I
> > would have probably had to grovel around with drgn to figure out the
> > address if that weren't embedded in the name. I think we probably ought
> > to leave it up to each subsystem how it names its files. The
> > discriminators between different types of objects can vary wildly.
> > 
> > One thing that might be simpler is to make ref_tracker_dir_debugfs() a
> > varargs function and allow passing in a format string and set of
> > arguments for it. That might make things simpler for the callers.
> 
> Yes, cutting out the formatting in the callers would definitely 
> be a win. Maybe that'd make the whole thing sufficiently palatable :)

Ok, I'll plan to change that. That does make the callers a lot cleaner.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24 10:56         ` Jeff Layton
@ 2025-04-24 12:10           ` Andrew Lunn
  2025-04-24 22:52             ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Andrew Lunn @ 2025-04-24 12:10 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Jakub Kicinski, Andrew Morton, David S. Miller, Eric Dumazet,
	Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Qasim Ijaz,
	Nathan Chancellor, linux-kernel, netdev

On Thu, Apr 24, 2025 at 06:56:06AM -0400, Jeff Layton wrote:
> On Wed, 2025-04-23 at 17:32 -0700, Jakub Kicinski wrote:
> > On Wed, 23 Apr 2025 20:04:58 -0400 Jeff Layton wrote:
> > > On Wed, 2025-04-23 at 16:53 -0700, Jakub Kicinski wrote:
> > > > Names are not unique and IIUC debugfs is not namespaced.
> 
> Correct, debugfs is not namespaced.
> 
> I meant to say earlier that I'm open to suggestions on how to make the
> netdev names unique. Low-level netdev stuff is not my area of
> expertise. We can drop this patch if doing so is problematic.
> 
> > > > How much naming the objects in a "user readable" fashion actually
> > > > matter? It'd be less churn to create some kind of "object class"
> > > > with a directory level named after what's already passed to
> > > > ref_tracker_dir_init() and then id the objects by the pointer value 
> > > > as sub-dirs of that?  
> > > 
> > > That sounds closer to what I had done originally. Andrew L. suggested
> > > the flat directory that this version represents. I'm fine with whatever
> > > hierarchy, but let's decide that before I respin again.
> > 
> > Sorry about that :(
> > 
> 
> No worries...but we do need to decide what this directory hierarchy
> should look like.
> 
> Andrew's point earlier was that this is just debugfs, so a flat
> "ref_tracker" directory full of files is fine. I tend to agree with
> him; NAME_MAX is 255, so we have plenty of room to make uniquely-named
> files.
> 
> We could build a dir hierarchy though. Something like:
> 
> - ref_tracker
>     + netdev
>     + netns

How do you make that generic? How due the GPU users of reftracker fit
in? And whatever the next users are? A flat directory keeps it
simple. Anybody capable of actually using this has to have a level of
intelligence sufficient for glob(3).

However, a varargs format function does make sense, since looking at
the current users, many of them will need it.

	Andrew

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24 12:10           ` Andrew Lunn
@ 2025-04-24 22:52             ` Jakub Kicinski
  2025-04-24 23:07               ` Eric Dumazet
  2025-04-25 12:46               ` Jeff Layton
  0 siblings, 2 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-04-24 22:52 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Jeff Layton, Andrew Morton, David S. Miller, Eric Dumazet,
	Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Qasim Ijaz,
	Nathan Chancellor, linux-kernel, netdev

On Thu, 24 Apr 2025 14:10:03 +0200 Andrew Lunn wrote:
> > > > > How much naming the objects in a "user readable" fashion actually
> > > > > matter? It'd be less churn to create some kind of "object class"
> > > > > with a directory level named after what's already passed to
> > > > > ref_tracker_dir_init() and then id the objects by the pointer value 
> > > > > as sub-dirs of that?    
> > > > 
> > > > That sounds closer to what I had done originally. Andrew L. suggested
> > > > the flat directory that this version represents. I'm fine with whatever
> > > > hierarchy, but let's decide that before I respin again.  
> > > 
> > > Sorry about that :(
> > >   
> > 
> > No worries...but we do need to decide what this directory hierarchy
> > should look like.
> > 
> > Andrew's point earlier was that this is just debugfs, so a flat
> > "ref_tracker" directory full of files is fine. I tend to agree with
> > him; NAME_MAX is 255, so we have plenty of room to make uniquely-named
> > files.
> > 
> > We could build a dir hierarchy though. Something like:
> > 
> > - ref_tracker
> >     + netdev
> >     + netns  
> 
> How do you make that generic? How due the GPU users of reftracker fit
> in? And whatever the next users are? A flat directory keeps it
> simple. Anybody capable of actually using this has to have a level of
> intelligence sufficient for glob(3).
> 
> However, a varargs format function does make sense, since looking at
> the current users, many of them will need it.

No preference on my side about the hierarchy TBH. I just defaulted to
thinking about it in terms of a hierarchy class/id rather than class-id
but shouldn't matter.

The main point I was trying to make was about using a synthetic ID -
like the pointer value. For the netdevs this patchset waits until 
the very end of the registration process to add the debugfs dir
because (I'm guessing) the name isn't assigned when we alloc 
the device (and therefore when we call ref_tracker_dir_init()).

Using synthetic ID lets us register the debugfs from
ref_tracker_dir_init().

In fact using "registered name" can be misleading. In modern setups
where devices are renamed based on the system topology, after
registration.

The Ethernet interface on my laptop is called enp0s13f0u1u1,
not eth0. It is renamed by systemd right _after_ registration.

[45224.911324] r8152 2-1.1:1.0 eth0: v1.12.13
[45225.220032] r8152 2-1.1:1.0 enp0s13f0u1u1: renamed from eth0

so in (most?) modern systems the name we carefully printed
into the debugfs name will in fact not match the current device name.
What more we don't try to keep the IDs monotonically increasing. 
If I plug in another Ethernet card it will also be called eth0 when
it's registered, again. You can experiment by adding dummy devices:

# ip link add type dummy
# ip link
...
2: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff

# ip link set dev dummy0 name other-name
[  206.747381][  T670] other-name: renamed from dummy0
# ip link
...
2: other-name: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff

# ip link add type dummy
# ip link
...
2: other-name: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff
3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff


The second device is called dummy0, again, because that name was
"free". So with the current code we delay the registration of debugfs
just to provide a name which is in fact misleading :S

But with all that said, I guess you still want the "meaningful" ID for
the netns, and that one is in fact stable :S

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24 22:52             ` Jakub Kicinski
@ 2025-04-24 23:07               ` Eric Dumazet
  2025-04-25 12:40                 ` Jeff Layton
  2025-04-25 12:46               ` Jeff Layton
  1 sibling, 1 reply; 22+ messages in thread
From: Eric Dumazet @ 2025-04-24 23:07 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andrew Lunn, Jeff Layton, Andrew Morton, David S. Miller,
	Paolo Abeni, Simon Horman, Kuniyuki Iwashima, Qasim Ijaz,
	Nathan Chancellor, linux-kernel, netdev

On Thu, Apr 24, 2025 at 3:52 PM Jakub Kicinski <kuba@kernel.org> wrote:

> But with all that said, I guess you still want the "meaningful" ID for
> the netns, and that one is in fact stable :S

We could add a stable id for net devices, and use it for this purpose

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0321fd952f70887735ac789d72f72948a3879832..339d09167eaf7f58fc877fa470c94175237ce534
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2544,6 +2544,8 @@ struct net_device {

        struct hwtstamp_provider __rcu  *hwprov;

+       u64                     permanent_id;
+
        u8                      priv[] ____cacheline_aligned
                                       __counted_by(priv_len);
 } ____cacheline_aligned;
diff --git a/net/core/dev.c b/net/core/dev.c
index d1a8cad0c99c47996e8bda44bf220266a5e51102..9d2d45e0246fab99ad3e7523885224bd114fa686
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10954,6 +10954,9 @@ int register_netdevice(struct net_device *dev)
 {
        int ret;
        struct net *net = dev_net(dev);
+       static atomic64_t permanent_id;
+
+       dev->permanent_id = atomic64_inc_return(&permanent_id);

        BUILD_BUG_ON(sizeof(netdev_features_t) * BITS_PER_BYTE <
                     NETDEV_FEATURE_COUNT);

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24 23:07               ` Eric Dumazet
@ 2025-04-25 12:40                 ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-25 12:40 UTC (permalink / raw)
  To: Eric Dumazet, Jakub Kicinski
  Cc: Andrew Lunn, Andrew Morton, David S. Miller, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	linux-kernel, netdev

On Thu, 2025-04-24 at 16:07 -0700, Eric Dumazet wrote:
> On Thu, Apr 24, 2025 at 3:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > But with all that said, I guess you still want the "meaningful" ID for
> > the netns, and that one is in fact stable :S
> 
> We could add a stable id for net devices, and use it for this purpose
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0321fd952f70887735ac789d72f72948a3879832..339d09167eaf7f58fc877fa470c94175237ce534
> 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2544,6 +2544,8 @@ struct net_device {
> 
>         struct hwtstamp_provider __rcu  *hwprov;
> 
> +       u64                     permanent_id;
> +
>         u8                      priv[] ____cacheline_aligned
>                                        __counted_by(priv_len);
>  } ____cacheline_aligned;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d1a8cad0c99c47996e8bda44bf220266a5e51102..9d2d45e0246fab99ad3e7523885224bd114fa686
> 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10954,6 +10954,9 @@ int register_netdevice(struct net_device *dev)
>  {
>         int ret;
>         struct net *net = dev_net(dev);
> +       static atomic64_t permanent_id;
> +
> +       dev->permanent_id = atomic64_inc_return(&permanent_id);
> 
>         BUILD_BUG_ON(sizeof(netdev_features_t) * BITS_PER_BYTE <
>                      NETDEV_FEATURE_COUNT);

That would give us uniqueness, but the name doesn't mean anything and
it wouldn't be trivial to track down which ref_tracker/netdev-* file
refers to which device.

If we're going to go that route, we're probably better off just
embedding the address in the name using a "netdev@%px" format.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker
  2025-04-24 22:52             ` Jakub Kicinski
  2025-04-24 23:07               ` Eric Dumazet
@ 2025-04-25 12:46               ` Jeff Layton
  1 sibling, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2025-04-25 12:46 UTC (permalink / raw)
  To: Jakub Kicinski, Andrew Lunn
  Cc: Andrew Morton, David S. Miller, Eric Dumazet, Paolo Abeni,
	Simon Horman, Kuniyuki Iwashima, Qasim Ijaz, Nathan Chancellor,
	linux-kernel, netdev

On Thu, 2025-04-24 at 15:52 -0700, Jakub Kicinski wrote:
> On Thu, 24 Apr 2025 14:10:03 +0200 Andrew Lunn wrote:
> > > > > > How much naming the objects in a "user readable" fashion actually
> > > > > > matter? It'd be less churn to create some kind of "object class"
> > > > > > with a directory level named after what's already passed to
> > > > > > ref_tracker_dir_init() and then id the objects by the pointer value 
> > > > > > as sub-dirs of that?    
> > > > > 
> > > > > That sounds closer to what I had done originally. Andrew L. suggested
> > > > > the flat directory that this version represents. I'm fine with whatever
> > > > > hierarchy, but let's decide that before I respin again.  
> > > > 
> > > > Sorry about that :(
> > > >   
> > > 
> > > No worries...but we do need to decide what this directory hierarchy
> > > should look like.
> > > 
> > > Andrew's point earlier was that this is just debugfs, so a flat
> > > "ref_tracker" directory full of files is fine. I tend to agree with
> > > him; NAME_MAX is 255, so we have plenty of room to make uniquely-named
> > > files.
> > > 
> > > We could build a dir hierarchy though. Something like:
> > > 
> > > - ref_tracker
> > >     + netdev
> > >     + netns  
> > 
> > How do you make that generic? How due the GPU users of reftracker fit
> > in? And whatever the next users are? A flat directory keeps it
> > simple. Anybody capable of actually using this has to have a level of
> > intelligence sufficient for glob(3).
> > 
> > However, a varargs format function does make sense, since looking at
> > the current users, many of them will need it.
> 
> No preference on my side about the hierarchy TBH. I just defaulted to
> thinking about it in terms of a hierarchy class/id rather than class-id
> but shouldn't matter.
> 
> The main point I was trying to make was about using a synthetic ID -
> like the pointer value. For the netdevs this patchset waits until 
> the very end of the registration process to add the debugfs dir
> because (I'm guessing) the name isn't assigned when we alloc 
> the device (and therefore when we call ref_tracker_dir_init()).
> 
> Using synthetic ID lets us register the debugfs from
> ref_tracker_dir_init().
> 

Yes, exactly. dev->name doesn't get populated until later, so we have
to delay creating the debugfs file if we want the name to be
meaningful. Ditto for the netns and its inode number.

We certainly could move to "classname@%px" format. If we go that route,
I'd suggest that we convert the ref_tracker_dir.name field to a const
char * pointer, and have the ref_tracker_dir_init() callers pass in a
pointer to a static classname string. No need to keep copies in that
case. The i915 ref_trackers would need to be changed, but hopefully
that's not a problem.

With that change we could register the debugfs files in
ref_tracker_dir_init() instead of having an extra call. The caveat
there is that some of these objects get created very early (e.g.
init_net), before debugfs comes online, so we'll miss creating debugfs
files for those objects. Maybe that's no big deal.

> In fact using "registered name" can be misleading. In modern setups
> where devices are renamed based on the system topology, after
> registration.
> 
> The Ethernet interface on my laptop is called enp0s13f0u1u1,
> not eth0. It is renamed by systemd right _after_ registration.
> 
> [45224.911324] r8152 2-1.1:1.0 eth0: v1.12.13
> [45225.220032] r8152 2-1.1:1.0 enp0s13f0u1u1: renamed from eth0
> 
> so in (most?) modern systems the name we carefully printed
> into the debugfs name will in fact not match the current device name.
> What more we don't try to keep the IDs monotonically increasing. 
> If I plug in another Ethernet card it will also be called eth0 when
> it's registered, again. You can experiment by adding dummy devices:
> 
> # ip link add type dummy
> # ip link
> ...
> 2: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff
> 
> # ip link set dev dummy0 name other-name
> [  206.747381][  T670] other-name: renamed from dummy0
> # ip link
> ...
> 2: other-name: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff
> 
> # ip link add type dummy
> # ip link
> ...
> 2: other-name: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff
> 3: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether b2:51:ee:5b:4b:83 brd ff:ff:ff:ff:ff:ff
> 
> 
> The second device is called dummy0, again, because that name was
> "free". So with the current code we delay the registration of debugfs
> just to provide a name which is in fact misleading :S
> 

Lovely.

> But with all that said, I guess you still want the "meaningful" ID for
> the netns, and that one is in fact stable :S

I would prefer that, but if that's not feasible then I can live without
meaningful names. We'd just have to determine some way to get the
address of a particular netns or netdev, which is an extra step when
debugging, but one we can do (e.g., with drgn).
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2025-04-25 12:46 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-18 14:24 [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jeff Layton
2025-04-18 14:24 ` [PATCH v4 1/7] ref_tracker: don't use %pK in pr_ostream() output Jeff Layton
2025-04-23 23:46   ` Jakub Kicinski
2025-04-23 23:56     ` Jeff Layton
2025-04-18 14:24 ` [PATCH v4 2/7] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
2025-04-18 14:24 ` [PATCH v4 3/7] ref_tracker: have callers pass output function to pr_ostream() Jeff Layton
2025-04-18 14:24 ` [PATCH v4 4/7] ref_tracker: allow pr_ostream() to print directly to a seq_file Jeff Layton
2025-04-18 14:24 ` [PATCH v4 5/7] ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir Jeff Layton
2025-04-18 14:24 ` [PATCH v4 6/7] net: add ref_tracker_dir_debugfs() calls for netns refcount tracking Jeff Layton
2025-04-18 14:24 ` [PATCH v4 7/7] net: register debugfs file for net_device refcnt tracker Jeff Layton
2025-04-23 23:53   ` Jakub Kicinski
2025-04-24  0:04     ` Jeff Layton
2025-04-24  0:32       ` Jakub Kicinski
2025-04-24 10:56         ` Jeff Layton
2025-04-24 12:10           ` Andrew Lunn
2025-04-24 22:52             ` Jakub Kicinski
2025-04-24 23:07               ` Eric Dumazet
2025-04-25 12:40                 ` Jeff Layton
2025-04-25 12:46               ` Jeff Layton
2025-04-23 23:44 ` [PATCH v4 0/7] ref_tracker: add ability to register a debugfs file for a ref_tracker_dir Jakub Kicinski
2025-04-23 23:48   ` Jeff Layton
2025-04-23 23:56     ` Jakub Kicinski

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