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

[Jeff Layton]

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