* [PATCH v2 0/2] net: add debugfs files for showing netns refcount tracking info
@ 2025-04-08 13:36 Jeff Layton
2025-04-08 13:36 ` [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
2025-04-08 13:36 ` [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
0 siblings, 2 replies; 16+ messages in thread
From: Jeff Layton @ 2025-04-08 13:36 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton
Cc: Andrew Lunn, netdev, linux-kernel, Jeff Layton
Recently, I had a need to track down some long-held netns references,
and discovered CONFIG_NET_NS_REFCNT_TRACKER. The main thing that seemed
to be missing from it though is a simple way to view the currently held
references on the netns. This adds files in debugfs for this.
Eric, I didn't incorporate your proposed patch to limit how long this
code spends in ref_tracker_dir_snprint(), as it didn't apply properly.
If you send that as a formal patch, or point me at a branch to base this
on, I can rebase this series on top.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- add top-level ref_tracker directory in debugfs, and move net_ns directory under it
- Link to v1: https://lore.kernel.org/r/20250324-netns-debugfs-v1-1-c75e9d5a6266@kernel.org
---
Jeff Layton (2):
ref_tracker: add a top level debugfs directory for ref_tracker
net: add debugfs files for showing netns refcount tracking info
include/linux/ref_tracker.h | 3 +
lib/ref_tracker.c | 15 +++++
net/core/net_namespace.c | 151 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 169 insertions(+)
---
base-commit: 695caca9345a160ecd9645abab8e70cfe849e9ff
change-id: 20250324-netns-debugfs-df213b2ab9ce
Best regards,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker
2025-04-08 13:36 [PATCH v2 0/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
@ 2025-04-08 13:36 ` Jeff Layton
2025-04-10 4:27 ` Kuniyuki Iwashima
2025-04-10 12:05 ` Andrew Lunn
2025-04-08 13:36 ` [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
1 sibling, 2 replies; 16+ messages in thread
From: Jeff Layton @ 2025-04-08 13:36 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton
Cc: Andrew Lunn, netdev, linux-kernel, Jeff Layton
Add a new "ref_tracker" directory in debugfs. Each individual refcount
tracker can add a directory or files under there to display info about
currently-held references.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
include/linux/ref_tracker.h | 3 +++
lib/ref_tracker.c | 15 +++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h
index 8eac4f3d52547ccbaf9dcd09962ce80d26fbdff8..16fb6ec0cc7adc24457cfab13ee3994d85c15b39 100644
--- a/include/linux/ref_tracker.h
+++ b/include/linux/ref_tracker.h
@@ -22,6 +22,9 @@ struct ref_tracker_dir {
};
#ifdef CONFIG_REF_TRACKER
+#ifdef CONFIG_DEBUG_FS
+extern struct dentry *ref_tracker_debug_dir;
+#endif /* CONFIG_DEBUG_FS */
static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir,
unsigned int quarantine_count,
diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
index cf5609b1ca79361763abe5a3a98484a3ee591ff2..136723eab6b17ae07132c659fd1d8b0690d8c2d9 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
+struct dentry *ref_tracker_debug_dir;
+
struct ref_tracker {
struct list_head head; /* anchor into dir->list or dir->quarantine */
bool dead;
@@ -273,3 +275,16 @@ 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_debug_init(void)
+{
+ ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
+ if (IS_ERR(ref_tracker_debug_dir))
+ return PTR_ERR(ref_tracker_debug_dir);
+ return 0;
+}
+late_initcall(ref_tracker_debug_init);
+#endif /* CONFIG_DEBUG_FS */
--
2.49.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-08 13:36 [PATCH v2 0/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
2025-04-08 13:36 ` [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
@ 2025-04-08 13:36 ` Jeff Layton
2025-04-10 4:24 ` Kuniyuki Iwashima
2025-04-10 12:36 ` Andrew Lunn
1 sibling, 2 replies; 16+ messages in thread
From: Jeff Layton @ 2025-04-08 13:36 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton
Cc: Andrew Lunn, netdev, linux-kernel, Jeff Layton
CONFIG_NET_NS_REFCNT_TRACKER currently has no convenient way to display
its tracking info. Add a new net_ns directory under the debugfs
ref_tracker directory. Create a directory in there for every netns, with
refcnt and notrefcnt files that show the currently tracked active and
passive references.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
net/core/net_namespace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 151 insertions(+)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 4303f2a4926243e2c0ff0c0387383cd8e0658019..7e9dc487f46d656ee4ae3d6d18d35bb2aba2b176 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -1512,3 +1512,154 @@ const struct proc_ns_operations netns_operations = {
.owner = netns_owner,
};
#endif
+
+#ifdef CONFIG_DEBUG_FS
+#ifdef CONFIG_NET_NS_REFCNT_TRACKER
+
+#include <linux/debugfs.h>
+
+static struct dentry *ns_ref_tracker_dir;
+static unsigned int ns_debug_net_id;
+
+struct ns_debug_net {
+ struct dentry *netdir;
+ struct dentry *refcnt;
+ struct dentry *notrefcnt;
+};
+
+#define MAX_NS_DEBUG_BUFSIZE (32 * PAGE_SIZE)
+
+static int
+ns_debug_tracker_show(struct seq_file *f, void *v)
+{
+ struct ref_tracker_dir *tracker = f->private;
+ int len, bufsize = PAGE_SIZE;
+ char *buf;
+
+ for (;;) {
+ buf = kvmalloc(bufsize, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ len = ref_tracker_dir_snprint(tracker, buf, bufsize);
+ if (len < bufsize)
+ break;
+
+ kvfree(buf);
+ bufsize *= 2;
+ if (bufsize > MAX_NS_DEBUG_BUFSIZE)
+ return -ENOBUFS;
+ }
+ seq_write(f, buf, len);
+ kvfree(buf);
+ return 0;
+}
+
+static int
+ns_debug_ref_open(struct inode *inode, struct file *filp)
+{
+ int ret;
+ struct net *net = inode->i_private;
+
+ ret = single_open(filp, ns_debug_tracker_show, &net->refcnt_tracker);
+ if (!ret)
+ net_passive_inc(net);
+ return ret;
+}
+
+static int
+ns_debug_notref_open(struct inode *inode, struct file *filp)
+{
+ int ret;
+ struct net *net = inode->i_private;
+
+ ret = single_open(filp, ns_debug_tracker_show, &net->notrefcnt_tracker);
+ if (!ret)
+ net_passive_inc(net);
+ return ret;
+}
+
+static int
+ns_debug_ref_release(struct inode *inode, struct file *filp)
+{
+ struct net *net = inode->i_private;
+
+ net_passive_dec(net);
+ return single_release(inode, filp);
+}
+
+static const struct file_operations ns_debug_ref_fops = {
+ .owner = THIS_MODULE,
+ .open = ns_debug_ref_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = ns_debug_ref_release,
+};
+
+static const struct file_operations ns_debug_notref_fops = {
+ .owner = THIS_MODULE,
+ .open = ns_debug_notref_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = ns_debug_ref_release,
+};
+
+static int
+ns_debug_init_net(struct net *net)
+{
+ struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
+ char name[11]; /* 10 decimal digits + NULL term */
+ int len;
+
+ len = snprintf(name, sizeof(name), "%u", net->ns.inum);
+ if (len >= sizeof(name))
+ return -EOVERFLOW;
+
+ dnet->netdir = debugfs_create_dir(name, ns_ref_tracker_dir);
+ if (IS_ERR(dnet->netdir))
+ return PTR_ERR(dnet->netdir);
+
+ dnet->refcnt = debugfs_create_file("refcnt", S_IFREG | 0400, dnet->netdir,
+ net, &ns_debug_ref_fops);
+ if (IS_ERR(dnet->refcnt)) {
+ debugfs_remove(dnet->netdir);
+ return PTR_ERR(dnet->refcnt);
+ }
+
+ dnet->notrefcnt = debugfs_create_file("notrefcnt", S_IFREG | 0400, dnet->netdir,
+ net, &ns_debug_notref_fops);
+ if (IS_ERR(dnet->notrefcnt)) {
+ debugfs_remove_recursive(dnet->netdir);
+ return PTR_ERR(dnet->notrefcnt);
+ }
+
+ return 0;
+}
+
+static void
+ns_debug_exit_net(struct net *net)
+{
+ struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
+
+ debugfs_remove_recursive(dnet->netdir);
+}
+
+static struct pernet_operations ns_debug_net_ops = {
+ .init = ns_debug_init_net,
+ .exit = ns_debug_exit_net,
+ .id = &ns_debug_net_id,
+ .size = sizeof(struct ns_debug_net),
+};
+
+static int __init ns_debug_init(void)
+{
+ ns_ref_tracker_dir = debugfs_create_dir("net_ns", ref_tracker_debug_dir);
+ if (IS_ERR(ns_ref_tracker_dir))
+ return PTR_ERR(ns_ref_tracker_dir);
+
+ register_pernet_subsys(&ns_debug_net_ops);
+ return 0;
+}
+late_initcall(ns_debug_init);
+#endif /* CONFIG_NET_NS_REFCNT_TRACKER */
+#endif /* CONFIG_DEBUG_FS */
--
2.49.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-08 13:36 ` [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
@ 2025-04-10 4:24 ` Kuniyuki Iwashima
2025-04-10 12:36 ` Andrew Lunn
1 sibling, 0 replies; 16+ messages in thread
From: Kuniyuki Iwashima @ 2025-04-10 4:24 UTC (permalink / raw)
To: jlayton
Cc: akpm, andrew, davem, edumazet, horms, kuba, linux-kernel, netdev,
pabeni, kuniyu
From: Jeff Layton <jlayton@kernel.org>
Date: Tue, 08 Apr 2025 09:36:38 -0400
> CONFIG_NET_NS_REFCNT_TRACKER currently has no convenient way to display
> its tracking info. Add a new net_ns directory under the debugfs
> ref_tracker directory. Create a directory in there for every netns, with
> refcnt and notrefcnt files that show the currently tracked active and
> passive references.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> net/core/net_namespace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 151 insertions(+)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 4303f2a4926243e2c0ff0c0387383cd8e0658019..7e9dc487f46d656ee4ae3d6d18d35bb2aba2b176 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -1512,3 +1512,154 @@ const struct proc_ns_operations netns_operations = {
> .owner = netns_owner,
> };
> #endif
> +
> +#ifdef CONFIG_DEBUG_FS
> +#ifdef CONFIG_NET_NS_REFCNT_TRACKER
> +
> +#include <linux/debugfs.h>
> +
> +static struct dentry *ns_ref_tracker_dir;
> +static unsigned int ns_debug_net_id;
> +
> +struct ns_debug_net {
> + struct dentry *netdir;
> + struct dentry *refcnt;
> + struct dentry *notrefcnt;
> +};
> +
> +#define MAX_NS_DEBUG_BUFSIZE (32 * PAGE_SIZE)
> +
> +static int
> +ns_debug_tracker_show(struct seq_file *f, void *v)
I think there is no clear rule about where to break, but could you
remove \n after int so that it will match with other functions in
this file ?
Same for other new functions, looks like none of them go over 80 columns.
> +{
> + struct ref_tracker_dir *tracker = f->private;
> + int len, bufsize = PAGE_SIZE;
> + char *buf;
> +
> + for (;;) {
> + buf = kvmalloc(bufsize, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + len = ref_tracker_dir_snprint(tracker, buf, bufsize);
> + if (len < bufsize)
> + break;
> +
> + kvfree(buf);
> + bufsize *= 2;
> + if (bufsize > MAX_NS_DEBUG_BUFSIZE)
> + return -ENOBUFS;
> + }
> + seq_write(f, buf, len);
> + kvfree(buf);
> + return 0;
> +}
> +
> +static int
> +ns_debug_ref_open(struct inode *inode, struct file *filp)
> +{
> + int ret;
> + struct net *net = inode->i_private;
nit: Please sort in the reverse xmas order.
https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
> +
> + ret = single_open(filp, ns_debug_tracker_show, &net->refcnt_tracker);
> + if (!ret)
> + net_passive_inc(net);
> + return ret;
> +}
> +
> +static int
> +ns_debug_notref_open(struct inode *inode, struct file *filp)
> +{
> + int ret;
> + struct net *net = inode->i_private;
Same here.
> +
> + ret = single_open(filp, ns_debug_tracker_show, &net->notrefcnt_tracker);
> + if (!ret)
> + net_passive_inc(net);
> + return ret;
> +}
> +
> +static int
> +ns_debug_ref_release(struct inode *inode, struct file *filp)
> +{
> + struct net *net = inode->i_private;
> +
> + net_passive_dec(net);
> + return single_release(inode, filp);
> +}
> +
> +static const struct file_operations ns_debug_ref_fops = {
> + .owner = THIS_MODULE,
> + .open = ns_debug_ref_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = ns_debug_ref_release,
> +};
> +
> +static const struct file_operations ns_debug_notref_fops = {
> + .owner = THIS_MODULE,
> + .open = ns_debug_notref_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = ns_debug_ref_release,
> +};
> +
> +static int
> +ns_debug_init_net(struct net *net)
> +{
> + struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
> + char name[11]; /* 10 decimal digits + NULL term */
> + int len;
> +
> + len = snprintf(name, sizeof(name), "%u", net->ns.inum);
> + if (len >= sizeof(name))
> + return -EOVERFLOW;
> +
> + dnet->netdir = debugfs_create_dir(name, ns_ref_tracker_dir);
> + if (IS_ERR(dnet->netdir))
> + return PTR_ERR(dnet->netdir);
> +
> + dnet->refcnt = debugfs_create_file("refcnt", S_IFREG | 0400, dnet->netdir,
> + net, &ns_debug_ref_fops);
> + if (IS_ERR(dnet->refcnt)) {
> + debugfs_remove(dnet->netdir);
> + return PTR_ERR(dnet->refcnt);
> + }
> +
> + dnet->notrefcnt = debugfs_create_file("notrefcnt", S_IFREG | 0400, dnet->netdir,
> + net, &ns_debug_notref_fops);
> + if (IS_ERR(dnet->notrefcnt)) {
> + debugfs_remove_recursive(dnet->netdir);
> + return PTR_ERR(dnet->notrefcnt);
> + }
> +
> + return 0;
> +}
> +
> +static void
> +ns_debug_exit_net(struct net *net)
> +{
> + struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
> +
> + debugfs_remove_recursive(dnet->netdir);
> +}
> +
> +static struct pernet_operations ns_debug_net_ops = {
> + .init = ns_debug_init_net,
> + .exit = ns_debug_exit_net,
> + .id = &ns_debug_net_id,
> + .size = sizeof(struct ns_debug_net),
> +};
> +
> +static int __init ns_debug_init(void)
> +{
> + ns_ref_tracker_dir = debugfs_create_dir("net_ns", ref_tracker_debug_dir);
> + if (IS_ERR(ns_ref_tracker_dir))
> + return PTR_ERR(ns_ref_tracker_dir);
> +
> + register_pernet_subsys(&ns_debug_net_ops);
> + return 0;
register_pernet_subsys() could fail, so
return register_pernet_subsys(&ns_debug_net_ops);
> +}
> +late_initcall(ns_debug_init);
> +#endif /* CONFIG_NET_NS_REFCNT_TRACKER */
> +#endif /* CONFIG_DEBUG_FS */
>
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker
2025-04-08 13:36 ` [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
@ 2025-04-10 4:27 ` Kuniyuki Iwashima
2025-04-10 12:05 ` Andrew Lunn
1 sibling, 0 replies; 16+ messages in thread
From: Kuniyuki Iwashima @ 2025-04-10 4:27 UTC (permalink / raw)
To: jlayton
Cc: akpm, andrew, davem, edumazet, horms, kuba, linux-kernel, netdev,
pabeni, Kuniyuki Iwashima
From: Jeff Layton <jlayton@kernel.org>
Date: Tue, 08 Apr 2025 09:36:37 -0400
> Add a new "ref_tracker" directory in debugfs. Each individual refcount
> tracker can add a directory or files under there to display info about
> currently-held references.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker
2025-04-08 13:36 ` [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
2025-04-10 4:27 ` Kuniyuki Iwashima
@ 2025-04-10 12:05 ` Andrew Lunn
1 sibling, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2025-04-10 12:05 UTC (permalink / raw)
To: Jeff Layton
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
> +static int __init ref_tracker_debug_init(void)
> +{
> + ref_tracker_debug_dir = debugfs_create_dir("ref_tracker", NULL);
> + if (IS_ERR(ref_tracker_debug_dir))
> + return PTR_ERR(ref_tracker_debug_dir);
> + return 0;
With debugfs you are not supposed to check the return codes. It is
debug, it does not matter if it fails, same way it does not matter if
it is not even part of the kernel. If it does fail, operations which
add to the directory as safe, they will not opps because of a previous
failure.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-08 13:36 ` [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
2025-04-10 4:24 ` Kuniyuki Iwashima
@ 2025-04-10 12:36 ` Andrew Lunn
2025-04-10 13:08 ` Jeff Layton
1 sibling, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2025-04-10 12:36 UTC (permalink / raw)
To: Jeff Layton
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Tue, Apr 08, 2025 at 09:36:38AM -0400, Jeff Layton wrote:
> CONFIG_NET_NS_REFCNT_TRACKER currently has no convenient way to display
> its tracking info. Add a new net_ns directory under the debugfs
> ref_tracker directory. Create a directory in there for every netns, with
> refcnt and notrefcnt files that show the currently tracked active and
> passive references.
I think most if not all of this should be moved into the tracker
sources, there is very little which is netdev specific.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> net/core/net_namespace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 151 insertions(+)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 4303f2a4926243e2c0ff0c0387383cd8e0658019..7e9dc487f46d656ee4ae3d6d18d35bb2aba2b176 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -1512,3 +1512,154 @@ const struct proc_ns_operations netns_operations = {
> .owner = netns_owner,
> };
> #endif
> +
> +#ifdef CONFIG_DEBUG_FS
> +#ifdef CONFIG_NET_NS_REFCNT_TRACKER
> +
> +#include <linux/debugfs.h>
> +
> +static struct dentry *ns_ref_tracker_dir;
> +static unsigned int ns_debug_net_id;
> +
> +struct ns_debug_net {
> + struct dentry *netdir;
> + struct dentry *refcnt;
> + struct dentry *notrefcnt;
> +};
> +
> +#define MAX_NS_DEBUG_BUFSIZE (32 * PAGE_SIZE)
> +
> +static int
> +ns_debug_tracker_show(struct seq_file *f, void *v)
> +{
> + struct ref_tracker_dir *tracker = f->private;
> + int len, bufsize = PAGE_SIZE;
> + char *buf;
> +
> + for (;;) {
> + buf = kvmalloc(bufsize, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + len = ref_tracker_dir_snprint(tracker, buf, bufsize);
> + if (len < bufsize)
> + break;
> +
> + kvfree(buf);
> + bufsize *= 2;
> + if (bufsize > MAX_NS_DEBUG_BUFSIZE)
> + return -ENOBUFS;
Maybe consider storing bufsize between calls to dump the tracker? I
guess you then have about the correct size for most calls, and from
looking at len, you can decide to downsize it if needed.
> +static int
> +ns_debug_init_net(struct net *net)
> +{
> + struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
> + char name[11]; /* 10 decimal digits + NULL term */
> + int len;
> +
> + len = snprintf(name, sizeof(name), "%u", net->ns.inum);
> + if (len >= sizeof(name))
> + return -EOVERFLOW;
> +
> + dnet->netdir = debugfs_create_dir(name, ns_ref_tracker_dir);
> + if (IS_ERR(dnet->netdir))
> + return PTR_ERR(dnet->netdir);
As i pointed out before, the tracker already has a name. Is that name
useless? Not specific enough? Rather than having two names, maybe
change the name to make it useful. Once it has a usable name, you
should be able to push more code into the core.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-10 12:36 ` Andrew Lunn
@ 2025-04-10 13:08 ` Jeff Layton
2025-04-10 13:23 ` Jeff Layton
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2025-04-10 13:08 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Thu, 2025-04-10 at 14:36 +0200, Andrew Lunn wrote:
> On Tue, Apr 08, 2025 at 09:36:38AM -0400, Jeff Layton wrote:
> > CONFIG_NET_NS_REFCNT_TRACKER currently has no convenient way to display
> > its tracking info. Add a new net_ns directory under the debugfs
> > ref_tracker directory. Create a directory in there for every netns, with
> > refcnt and notrefcnt files that show the currently tracked active and
> > passive references.
>
> I think most if not all of this should be moved into the tracker
> sources, there is very little which is netdev specific.
>
Fair enough. I can move most of this into helpers in ref_tracker.c.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > net/core/net_namespace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 151 insertions(+)
> >
> > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> > index 4303f2a4926243e2c0ff0c0387383cd8e0658019..7e9dc487f46d656ee4ae3d6d18d35bb2aba2b176 100644
> > --- a/net/core/net_namespace.c
> > +++ b/net/core/net_namespace.c
> > @@ -1512,3 +1512,154 @@ const struct proc_ns_operations netns_operations = {
> > .owner = netns_owner,
> > };
> > #endif
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +#ifdef CONFIG_NET_NS_REFCNT_TRACKER
> > +
> > +#include <linux/debugfs.h>
> > +
> > +static struct dentry *ns_ref_tracker_dir;
> > +static unsigned int ns_debug_net_id;
> > +
> > +struct ns_debug_net {
> > + struct dentry *netdir;
> > + struct dentry *refcnt;
> > + struct dentry *notrefcnt;
> > +};
> > +
> > +#define MAX_NS_DEBUG_BUFSIZE (32 * PAGE_SIZE)
> > +
> > +static int
> > +ns_debug_tracker_show(struct seq_file *f, void *v)
> > +{
> > + struct ref_tracker_dir *tracker = f->private;
> > + int len, bufsize = PAGE_SIZE;
> > + char *buf;
> > +
> > + for (;;) {
> > + buf = kvmalloc(bufsize, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + len = ref_tracker_dir_snprint(tracker, buf, bufsize);
> > + if (len < bufsize)
> > + break;
> > +
> > + kvfree(buf);
> > + bufsize *= 2;
> > + if (bufsize > MAX_NS_DEBUG_BUFSIZE)
> > + return -ENOBUFS;
>
> Maybe consider storing bufsize between calls to dump the tracker? I
> guess you then have about the correct size for most calls, and from
> looking at len, you can decide to downsize it if needed.
>
Eric had a proposed change to make ref_tracker_dir_snprint() not sit
with hard IRQs disabled for so long. That involved passing back a
needed size, so I might rather integrate this into that change.
> > +static int
> > +ns_debug_init_net(struct net *net)
> > +{
> > + struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
> > + char name[11]; /* 10 decimal digits + NULL term */
> > + int len;
> > +
> > + len = snprintf(name, sizeof(name), "%u", net->ns.inum);
> > + if (len >= sizeof(name))
> > + return -EOVERFLOW;
> > +
> > + dnet->netdir = debugfs_create_dir(name, ns_ref_tracker_dir);
> > + if (IS_ERR(dnet->netdir))
> > + return PTR_ERR(dnet->netdir);
>
> As i pointed out before, the tracker already has a name. Is that name
> useless? Not specific enough? Rather than having two names, maybe
> change the name to make it useful. Once it has a usable name, you
> should be able to push more code into the core.
>
I don't understand which name you mean.
This patch creates a ref_tracker/net_ns dir and then creates
directories under that that have names that match the net namespace
inode numbers as displayed in /proc/<pid>/ns/net symlink. Those
directories hold two files "refcnt" and "notrefcnt" that display the
different trackers.
It's not clear to me what you'd like to see changed in that scheme.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-10 13:08 ` Jeff Layton
@ 2025-04-10 13:23 ` Jeff Layton
2025-04-10 14:12 ` Andrew Lunn
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2025-04-10 13:23 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Thu, 2025-04-10 at 09:08 -0400, Jeff Layton wrote:
> On Thu, 2025-04-10 at 14:36 +0200, Andrew Lunn wrote:
> > On Tue, Apr 08, 2025 at 09:36:38AM -0400, Jeff Layton wrote:
> > > CONFIG_NET_NS_REFCNT_TRACKER currently has no convenient way to display
> > > its tracking info. Add a new net_ns directory under the debugfs
> > > ref_tracker directory. Create a directory in there for every netns, with
> > > refcnt and notrefcnt files that show the currently tracked active and
> > > passive references.
> >
> > I think most if not all of this should be moved into the tracker
> > sources, there is very little which is netdev specific.
> >
>
> Fair enough. I can move most of this into helpers in ref_tracker.c.
>
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > net/core/net_namespace.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 151 insertions(+)
> > >
> > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> > > index 4303f2a4926243e2c0ff0c0387383cd8e0658019..7e9dc487f46d656ee4ae3d6d18d35bb2aba2b176 100644
> > > --- a/net/core/net_namespace.c
> > > +++ b/net/core/net_namespace.c
> > > @@ -1512,3 +1512,154 @@ const struct proc_ns_operations netns_operations = {
> > > .owner = netns_owner,
> > > };
> > > #endif
> > > +
> > > +#ifdef CONFIG_DEBUG_FS
> > > +#ifdef CONFIG_NET_NS_REFCNT_TRACKER
> > > +
> > > +#include <linux/debugfs.h>
> > > +
> > > +static struct dentry *ns_ref_tracker_dir;
> > > +static unsigned int ns_debug_net_id;
> > > +
> > > +struct ns_debug_net {
> > > + struct dentry *netdir;
> > > + struct dentry *refcnt;
> > > + struct dentry *notrefcnt;
> > > +};
> > > +
> > > +#define MAX_NS_DEBUG_BUFSIZE (32 * PAGE_SIZE)
> > > +
> > > +static int
> > > +ns_debug_tracker_show(struct seq_file *f, void *v)
> > > +{
> > > + struct ref_tracker_dir *tracker = f->private;
> > > + int len, bufsize = PAGE_SIZE;
> > > + char *buf;
> > > +
> > > + for (;;) {
> > > + buf = kvmalloc(bufsize, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + len = ref_tracker_dir_snprint(tracker, buf, bufsize);
> > > + if (len < bufsize)
> > > + break;
> > > +
> > > + kvfree(buf);
> > > + bufsize *= 2;
> > > + if (bufsize > MAX_NS_DEBUG_BUFSIZE)
> > > + return -ENOBUFS;
> >
> > Maybe consider storing bufsize between calls to dump the tracker? I
> > guess you then have about the correct size for most calls, and from
> > looking at len, you can decide to downsize it if needed.
> >
>
> Eric had a proposed change to make ref_tracker_dir_snprint() not sit
> with hard IRQs disabled for so long. That involved passing back a
> needed size, so I might rather integrate this into that change.
>
> > > +static int
> > > +ns_debug_init_net(struct net *net)
> > > +{
> > > + struct ns_debug_net *dnet = net_generic(net, ns_debug_net_id);
> > > + char name[11]; /* 10 decimal digits + NULL term */
> > > + int len;
> > > +
> > > + len = snprintf(name, sizeof(name), "%u", net->ns.inum);
> > > + if (len >= sizeof(name))
> > > + return -EOVERFLOW;
> > > +
> > > + dnet->netdir = debugfs_create_dir(name, ns_ref_tracker_dir);
> > > + if (IS_ERR(dnet->netdir))
> > > + return PTR_ERR(dnet->netdir);
> >
> > As i pointed out before, the tracker already has a name. Is that name
> > useless? Not specific enough? Rather than having two names, maybe
> > change the name to make it useful. Once it has a usable name, you
> > should be able to push more code into the core.
> >
>
> I don't understand which name you mean.
>
> This patch creates a ref_tracker/net_ns dir and then creates
> directories under that that have names that match the net namespace
> inode numbers as displayed in /proc/<pid>/ns/net symlink. Those
> directories hold two files "refcnt" and "notrefcnt" that display the
> different trackers.
>
> It's not clear to me what you'd like to see changed in that scheme.
Oh, ok. I guess you mean these names?
ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
Two problems there:
1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
2/ they aren't named in a per-net namespace way
I guess we could do something like these nested dirs:
debug
ref_tracker
net_refcnt
net_notrefcnt
...and then create files under the net_* directories that match the
netns ID's. That's what I was trying to ask when I asked about the
directory structure in the last set. How do you want the directory
structure laid out?
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-10 13:23 ` Jeff Layton
@ 2025-04-10 14:12 ` Andrew Lunn
2025-04-10 14:41 ` Jeff Layton
2025-04-13 11:40 ` Jeff Layton
0 siblings, 2 replies; 16+ messages in thread
From: Andrew Lunn @ 2025-04-10 14:12 UTC (permalink / raw)
To: Jeff Layton
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
> Oh, ok. I guess you mean these names?
>
> ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
>
> Two problems there:
>
> 1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
> 2/ they aren't named in a per-net namespace way
So the first question is, are the names ABI? Are they exposed to
userspace anywhere? Can we change them?
If we can change them, space to _ is a simple change. Another option
is what hwmon does, hwmon_sanitize_name() which turns a name into
something which is legal in a filesystem. If all of this code can be
pushed into the core tracker, so all trackers appear in debugfs, such
a sanitiser is the way i would go.
And if we can change the name, putting the netns into the name would
also work. There is then no need for the directory, if they have
unique names.
Looking at other users of ref_tracker_dir_init():
~/linux$ grep -r ref_tracker_dir_init
lib/test_ref_tracker.c: ref_tracker_dir_init(&ref_dir, 100, "selftest");
Can only be loaded once, so is unique.
drivers/gpu/drm/i915/intel_wakeref.c: ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
Looks like it is unique for one GPU, but if you have multiple GPUs
they are not unique.
drivers/gpu/drm/i915/intel_runtime_pm.c: ref_tracker_dir_init(&rpm->debug, INTEL_REFTRACK_DEAD_COUNT, dev_name(rpm->kdev));
At a guess kdev is unique.
drivers/gpu/drm/display/drm_dp_tunnel.c: ref_tracker_dir_init(&mgr->ref_tracker, 16, "dptun");
Probably not unique.
net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
Not unique across name spaces, but ...
So could the tracker core check if the debugfs file already exists,
emit a warning if it does, and keep going? I think debugfs_lookup()
will tell you if a file already exists, or debugfs_create_file() will
return -EEXIST, which is probably safer, no race condition.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-10 14:12 ` Andrew Lunn
@ 2025-04-10 14:41 ` Jeff Layton
2025-04-13 11:40 ` Jeff Layton
1 sibling, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2025-04-10 14:41 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Thu, 2025-04-10 at 16:12 +0200, Andrew Lunn wrote:
> > Oh, ok. I guess you mean these names?
> >
> > ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> >
> > Two problems there:
> >
> > 1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
> > 2/ they aren't named in a per-net namespace way
>
> So the first question is, are the names ABI? Are they exposed to
> userspace anywhere? Can we change them?
>
> If we can change them, space to _ is a simple change. Another option
> is what hwmon does, hwmon_sanitize_name() which turns a name into
> something which is legal in a filesystem. If all of this code can be
> pushed into the core tracker, so all trackers appear in debugfs, such
> a sanitiser is the way i would go.
>
> And if we can change the name, putting the netns into the name would
> also work. There is then no need for the directory, if they have
> unique names.
AFAICT, they only show up today in some of the pr_ostream() error
messages. I'm assuming that means that they aren't part of ABI.
If we can change those names, then we could just make the debugfs file
registrations happen as part of ref_tracker_dir_init() under a flat
hierarchy like you suggest. If that's the case I can move the whole
thing into ref_tracker.c.
I'd probably shoot to just ensure that the names unique in some fashion
rather than try to run them through a sanitizer.
Eric any thoughts, re: ABI?
> Looking at other users of ref_tracker_dir_init():
>
> ~/linux$ grep -r ref_tracker_dir_init
> lib/test_ref_tracker.c: ref_tracker_dir_init(&ref_dir, 100, "selftest");
>
> Can only be loaded once, so is unique.
>
> drivers/gpu/drm/i915/intel_wakeref.c: ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
>
> Looks like it is unique for one GPU, but if you have multiple GPUs
> they are not unique.
>
> drivers/gpu/drm/i915/intel_runtime_pm.c: ref_tracker_dir_init(&rpm->debug, INTEL_REFTRACK_DEAD_COUNT, dev_name(rpm->kdev));
>
> At a guess kdev is unique.
>
> drivers/gpu/drm/display/drm_dp_tunnel.c: ref_tracker_dir_init(&mgr->ref_tracker, 16, "dptun");
>
> Probably not unique.
>
> net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
>
> Not unique across name spaces, but ...
>
> So could the tracker core check if the debugfs file already exists,
> emit a warning if it does, and keep going? I think debugfs_lookup()
> will tell you if a file already exists, or debugfs_create_file() will
> return -EEXIST, which is probably safer, no race condition.
Yeah, that would work. If we get back an EEXIST, we can throw a
pr_notice or something too. That probably means the name wasn't
sufficiently unique.
I'll work on a v3 that moves things in that direction.
Thanks for the review!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-10 14:12 ` Andrew Lunn
2025-04-10 14:41 ` Jeff Layton
@ 2025-04-13 11:40 ` Jeff Layton
2025-04-13 19:32 ` Andrew Lunn
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2025-04-13 11:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Thu, 2025-04-10 at 16:12 +0200, Andrew Lunn wrote:
> > Oh, ok. I guess you mean these names?
> >
> > ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> >
> > Two problems there:
> >
> > 1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
> > 2/ they aren't named in a per-net namespace way
>
> So the first question is, are the names ABI? Are they exposed to
> userspace anywhere? Can we change them?
>
> If we can change them, space to _ is a simple change. Another option
> is what hwmon does, hwmon_sanitize_name() which turns a name into
> something which is legal in a filesystem. If all of this code can be
> pushed into the core tracker, so all trackers appear in debugfs, such
> a sanitiser is the way i would go.
>
> And if we can change the name, putting the netns into the name would
> also work. There is then no need for the directory, if they have
> unique names.
>
> Looking at other users of ref_tracker_dir_init():
>
> ~/linux$ grep -r ref_tracker_dir_init
> lib/test_ref_tracker.c: ref_tracker_dir_init(&ref_dir, 100, "selftest");
>
> Can only be loaded once, so is unique.
>
> drivers/gpu/drm/i915/intel_wakeref.c: ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
>
> Looks like it is unique for one GPU, but if you have multiple GPUs
> they are not unique.
>
We'll need some input from the i915 folks then.
> drivers/gpu/drm/i915/intel_runtime_pm.c: ref_tracker_dir_init(&rpm->debug, INTEL_REFTRACK_DEAD_COUNT, dev_name(rpm->kdev));
>
> At a guess kdev is unique.
>
Yeah, looks like it.
> drivers/gpu/drm/display/drm_dp_tunnel.c: ref_tracker_dir_init(&mgr->ref_tracker, 16, "dptun");
>
> Probably not unique.
>
Looking more here, perhaps we can incorporate mgr->dev->unique into the
name? I don't know much about dp drivers. Can multiple mgrs point to
the same device?
> net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
>
> Not unique across name spaces, but ...
That's easily fixable. net->ns.inum is unique.
>
> So could the tracker core check if the debugfs file already exists,
> emit a warning if it does, and keep going? I think debugfs_lookup()
> will tell you if a file already exists, or debugfs_create_file() will
> return -EEXIST, which is probably safer, no race condition.
>
Yeah, that should be possible. I think too that we can eliminate the
buffer management in this codepath by allowing pr_ostream() to output
directly to a seq_file. I'll look into that as well.
Thanks for the input!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-13 11:40 ` Jeff Layton
@ 2025-04-13 19:32 ` Andrew Lunn
2025-04-14 12:20 ` Jeff Layton
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2025-04-13 19:32 UTC (permalink / raw)
To: Jeff Layton
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Sun, Apr 13, 2025 at 07:40:59AM -0400, Jeff Layton wrote:
> On Thu, 2025-04-10 at 16:12 +0200, Andrew Lunn wrote:
> > > Oh, ok. I guess you mean these names?
> > >
> > > ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > > ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> > >
> > > Two problems there:
> > >
> > > 1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
> > > 2/ they aren't named in a per-net namespace way
> >
> > So the first question is, are the names ABI? Are they exposed to
> > userspace anywhere? Can we change them?
> >
> > If we can change them, space to _ is a simple change. Another option
> > is what hwmon does, hwmon_sanitize_name() which turns a name into
> > something which is legal in a filesystem. If all of this code can be
> > pushed into the core tracker, so all trackers appear in debugfs, such
> > a sanitiser is the way i would go.
> >
> > And if we can change the name, putting the netns into the name would
> > also work. There is then no need for the directory, if they have
> > unique names.
> >
> > Looking at other users of ref_tracker_dir_init():
> >
> > ~/linux$ grep -r ref_tracker_dir_init
> > lib/test_ref_tracker.c: ref_tracker_dir_init(&ref_dir, 100, "selftest");
> >
> > Can only be loaded once, so is unique.
> >
> > drivers/gpu/drm/i915/intel_wakeref.c: ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
> >
> > Looks like it is unique for one GPU, but if you have multiple GPUs
> > they are not unique.
> >
>
> We'll need some input from the i915 folks then.
That is why i think it would be better to add a warning, give the i915
folks a heads up, and leave them to fix it how they want. We want the
warning anyway to cover new refcount instances being added.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-13 19:32 ` Andrew Lunn
@ 2025-04-14 12:20 ` Jeff Layton
2025-04-14 12:46 ` Andrew Lunn
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2025-04-14 12:20 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Sun, 2025-04-13 at 21:32 +0200, Andrew Lunn wrote:
> On Sun, Apr 13, 2025 at 07:40:59AM -0400, Jeff Layton wrote:
> > On Thu, 2025-04-10 at 16:12 +0200, Andrew Lunn wrote:
> > > > Oh, ok. I guess you mean these names?
> > > >
> > > > ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > > > ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> > > >
> > > > Two problems there:
> > > >
> > > > 1/ they have an embedded space in the name which is just painful. Maybe we can replace those with underscores?
> > > > 2/ they aren't named in a per-net namespace way
> > >
> > > So the first question is, are the names ABI? Are they exposed to
> > > userspace anywhere? Can we change them?
> > >
> > > If we can change them, space to _ is a simple change. Another option
> > > is what hwmon does, hwmon_sanitize_name() which turns a name into
> > > something which is legal in a filesystem. If all of this code can be
> > > pushed into the core tracker, so all trackers appear in debugfs, such
> > > a sanitiser is the way i would go.
> > >
> > > And if we can change the name, putting the netns into the name would
> > > also work. There is then no need for the directory, if they have
> > > unique names.
> > >
> > > Looking at other users of ref_tracker_dir_init():
> > >
> > > ~/linux$ grep -r ref_tracker_dir_init
> > > lib/test_ref_tracker.c: ref_tracker_dir_init(&ref_dir, 100, "selftest");
> > >
> > > Can only be loaded once, so is unique.
> > >
> > > drivers/gpu/drm/i915/intel_wakeref.c: ref_tracker_dir_init(&wf->debug, INTEL_REFTRACK_DEAD_COUNT, name);
> > >
> > > Looks like it is unique for one GPU, but if you have multiple GPUs
> > > they are not unique.
> > >
> >
> > We'll need some input from the i915 folks then.
>
> That is why i think it would be better to add a warning, give the i915
> folks a heads up, and leave them to fix it how they want. We want the
> warning anyway to cover new refcount instances being added.
>
There will definitely be a pr_warn() if creation fails. I was hoping to
send a suggested change alongside the patchset, but I may have to just
leave it up to them.
I threw together a draft patchset that auto-registers a debugfs file
for every call to ref_tracker_dir_init(). The problem I've hit now
though is that at least in the networking cases, the kernel calls
ref_tracker_dir_init() very early in the process of creating a new
objects, so we're not getting good names here:
The "name" in alloc_netdev_mqs is actually a format string, so we end
up with a name like "eth%d" here:
net/core/dev.c: ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);
At the point that we call these (preinit), net->ns.inum hasn't been
assigned yet, so we can't incorporate it properly into the dentry name:
net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
We could do the ref_tracker_dir_init() calls later, but we may end up
missing some references that way (or end up crashing because the "dir"
isn't initialized.
My current thinking is to add a new ref_tracker_dir_finalize() function
that we could use to finalize the name and register the debugfs files
after we have the requisite info. It's an extra manual step, but I like
that better than moving around the ref_tracker_dir_init() calls.
Thoughts?
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-14 12:20 ` Jeff Layton
@ 2025-04-14 12:46 ` Andrew Lunn
2025-04-14 12:48 ` Jeff Layton
0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2025-04-14 12:46 UTC (permalink / raw)
To: Jeff Layton
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
> > > We'll need some input from the i915 folks then.
> >
> > That is why i think it would be better to add a warning, give the i915
> > folks a heads up, and leave them to fix it how they want. We want the
> > warning anyway to cover new refcount instances being added.
> >
>
> There will definitely be a pr_warn() if creation fails. I was hoping to
> send a suggested change alongside the patchset, but I may have to just
> leave it up to them.
>
> I threw together a draft patchset that auto-registers a debugfs file
> for every call to ref_tracker_dir_init(). The problem I've hit now
> though is that at least in the networking cases, the kernel calls
> ref_tracker_dir_init() very early in the process of creating a new
> objects, so we're not getting good names here:
>
> The "name" in alloc_netdev_mqs is actually a format string, so we end
> up with a name like "eth%d" here:
>
> net/core/dev.c: ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);
Oh, yes, never thought about that...
It also seems like it might be a common problem, the subsystem defined
name has not been given yet, although the core dev_name(dev) should
work. But in netdev drivers, alloc_netdev_mqs() does not have access
to that.
> At the point that we call these (preinit), net->ns.inum hasn't been
> assigned yet, so we can't incorporate it properly into the dentry name:
>
> net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
>
> We could do the ref_tracker_dir_init() calls later, but we may end up
> missing some references that way (or end up crashing because the "dir"
> isn't initialized.
>
> My current thinking is to add a new ref_tracker_dir_finalize() function
> that we could use to finalize the name and register the debugfs files
> after we have the requisite info. It's an extra manual step, but I like
> that better than moving around the ref_tracker_dir_init() calls.
Agree. Although, bike shedding, i don't really like the _finalize in
ref_tracker_dir_finalize(). Ideally this should be optional and
populate debugfs. _finalize does not sound particularly optional. So
maybe ref_tracker_dir_debugfs()?
This also has the advantage of we need to worry less about GPU
drivers. They see no change in behaviour, but we can give them a heads
up the new call exists if they want to use it to populate debugfs.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info
2025-04-14 12:46 ` Andrew Lunn
@ 2025-04-14 12:48 ` Jeff Layton
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2025-04-14 12:48 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Morton, netdev, linux-kernel
On Mon, 2025-04-14 at 14:46 +0200, Andrew Lunn wrote:
> > > > We'll need some input from the i915 folks then.
> > >
> > > That is why i think it would be better to add a warning, give the i915
> > > folks a heads up, and leave them to fix it how they want. We want the
> > > warning anyway to cover new refcount instances being added.
> > >
> >
> > There will definitely be a pr_warn() if creation fails. I was hoping to
> > send a suggested change alongside the patchset, but I may have to just
> > leave it up to them.
> >
> > I threw together a draft patchset that auto-registers a debugfs file
> > for every call to ref_tracker_dir_init(). The problem I've hit now
> > though is that at least in the networking cases, the kernel calls
> > ref_tracker_dir_init() very early in the process of creating a new
> > objects, so we're not getting good names here:
> >
> > The "name" in alloc_netdev_mqs is actually a format string, so we end
> > up with a name like "eth%d" here:
> >
> > net/core/dev.c: ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);
>
>
>
>
> Oh, yes, never thought about that...
>
> It also seems like it might be a common problem, the subsystem defined
> name has not been given yet, although the core dev_name(dev) should
> work. But in netdev drivers, alloc_netdev_mqs() does not have access
> to that.
>
> > At the point that we call these (preinit), net->ns.inum hasn't been
> > assigned yet, so we can't incorporate it properly into the dentry name:
> >
> > net/core/net_namespace.c: ref_tracker_dir_init(&net->refcnt_tracker, 128, "net refcnt");
> > net/core/net_namespace.c: ref_tracker_dir_init(&net->notrefcnt_tracker, 128, "net notrefcnt");
> >
> > We could do the ref_tracker_dir_init() calls later, but we may end up
> > missing some references that way (or end up crashing because the "dir"
> > isn't initialized.
> >
> > My current thinking is to add a new ref_tracker_dir_finalize() function
> > that we could use to finalize the name and register the debugfs files
> > after we have the requisite info. It's an extra manual step, but I like
> > that better than moving around the ref_tracker_dir_init() calls.
>
> Agree. Although, bike shedding, i don't really like the _finalize in
> ref_tracker_dir_finalize(). Ideally this should be optional and
> populate debugfs. _finalize does not sound particularly optional. So
> maybe ref_tracker_dir_debugfs()?
>
I'm terrible at naming, so we can go with that. That last step will
definitely be optional.
> This also has the advantage of we need to worry less about GPU
> drivers. They see no change in behaviour, but we can give them a heads
> up the new call exists if they want to use it to populate debugfs.
>
+1
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2025-04-14 12:48 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-08 13:36 [PATCH v2 0/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
2025-04-08 13:36 ` [PATCH v2 1/2] ref_tracker: add a top level debugfs directory for ref_tracker Jeff Layton
2025-04-10 4:27 ` Kuniyuki Iwashima
2025-04-10 12:05 ` Andrew Lunn
2025-04-08 13:36 ` [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info Jeff Layton
2025-04-10 4:24 ` Kuniyuki Iwashima
2025-04-10 12:36 ` Andrew Lunn
2025-04-10 13:08 ` Jeff Layton
2025-04-10 13:23 ` Jeff Layton
2025-04-10 14:12 ` Andrew Lunn
2025-04-10 14:41 ` Jeff Layton
2025-04-13 11:40 ` Jeff Layton
2025-04-13 19:32 ` Andrew Lunn
2025-04-14 12:20 ` Jeff Layton
2025-04-14 12:46 ` Andrew Lunn
2025-04-14 12:48 ` Jeff Layton
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).