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

[Jeff Layton <jlayton@kernel.org>]

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>