Message-ID: <b2183fb18102b99685b69d57cb099995364c68c8.camel@kernel.org>
Date: Thu, 10 Apr 2025 10:41:13 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>,  Jakub Kicinski	 <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman	 <horms@...nel.org>, Andrew Morton
 <akpm@...ux-foundation.org>, 	netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] net: add debugfs files for showing netns
 refcount tracking info

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@...nel.org>

Powered by blists - more mailing lists