Re: [PATCH 2/4] ref_tracker: add ability to register a file in debugfs for a ref_tracker_dir

[Jeff Layton]

On Tue, 2025-04-15 at 01:08 +0200, Andrew Lunn wrote:
> On Mon, Apr 14, 2025 at 10:45:47AM -0400, Jeff Layton wrote:
> > Currently, there is no convenient way to see the info that the
> > ref_tracking infrastructure collects. Add a new function that other
> > subsystems can optionally call to update the name field in the
> > ref_tracker_dir and register a corresponding seq_file for it in the
> > top-level ref_tracker directory.
> > 
> > Also, alter the pr_ostream infrastructure to allow the caller to specify
> > a seq_file to which the output should go instead of printing to an
> > arbitrary buffer or the kernel's ring buffer.
> 
> When i see an Also, or And, or a list in a commit message, i always
> think, should this be multiple patches?
> 

Sure. I actually had this part in a separate patch earlier, but I don't
usually like adding functions with no callers and this patch was pretty
small. I can break it up though.

> >  struct ostream {
> >  	char *buf;
> > +	struct seq_file *seq;
> >  	int size, used;
> >  };
> >  
> > @@ -73,7 +83,9 @@ struct ostream {
> >  ({ \
> >  	struct ostream *_s = (stream); \
> >  \
> > -	if (!_s->buf) { \
> > +	if (_s->seq) { \
> > +		seq_printf(_s->seq, fmt, ##args); \
> > +	} else if (!_s->buf) { \
> >  		pr_err(fmt, ##args); \
> >  	} else { \
> >  		int ret, len = _s->size - _s->used; \
> 
> The pr_ostream() macro is getting pretty convoluted. It currently
> supports two user cases:
> 
> struct ostream os = {}; which means just use pr_err().
> 
> And os.buf points to an allocated buffer and the output should be
> dumped there.
> 
> You are about to add a third.
> 
> Is it about time this got split up into three helper functions, and
> you pass one to __ref_tracker_dir_pr_ostream()? Your choice.
> 

Maybe? It doesn't seem worth it for this, but I'll take a look.

> > +/**
> > + * ref_tracker_dir_debugfs - create debugfs file for ref_tracker_dir
> > + * @dir: ref_tracker_dir to finalize
> > + * @name: updated name of the ref_tracker_dir
> > + *
> > + * In some cases, the name given to a ref_tracker_dir is based on incomplete information,
> > + * and may not be unique. Call this to finalize the name of @dir, and create a debugfs
> > + * file for it.
> 
> Maybe extend the documentation with a comment that is name is not
> unique within debugfs directory, a warning will be emitted but it is
> not fatal to the tracker.
> 

Ok.

> > + */
> > +void ref_tracker_dir_debugfs(struct ref_tracker_dir *dir, const char *name)
> > +{
> > +	strscpy(dir->name, name, sizeof(dir->name));
> 
> I don't know about this. Should we really overwrite the name passed
> earlier? Would it be better to treat the name here only as the debugfs
> filename?
> 

I think it's safe. The only thing that currently uses ->name is
pr_ostream(), AFAICT.

> > +	if (ref_tracker_debug_dir) {
> 
> Not needed
> 

If we call debugfs_create_file() and pass in NULL as the parent, it
will try to create the entry at the root of debugfs. We can get rid of
this though if we initialize ref_tracker_debug_dir to an ERR_PTR value.
I'll see about doing that.

> > +		dir->dentry = debugfs_create_file(dir->name, S_IFREG | 0400,
> > +						  ref_tracker_debug_dir, dir,
> > +						  &ref_tracker_debugfs_fops);
> > +		if (IS_ERR(dir->dentry)) {
> > +			pr_warn("ref_tracker: unable to create debugfs file for %s: %pe\n",
> > +				dir->name, dir->dentry);
> > +			dir->dentry = NULL;
> 
> this last statement should also be unneeded.
> 

Only if ref_tracker_debug_dir is initialized to an ERR_PTR.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>