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

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

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>

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>

> 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

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>

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

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>

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

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>

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>

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
>