Re: [PATCH v2 2/2] net: add debugfs files for showing netns refcount tracking info

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