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

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-04-15 at 08:54 -0400, Jeff Layton wrote:
> On Tue, 2025-04-15 at 06:23 -0400, Jeff Layton wrote:
> > 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.
> > 
> > > 
> 
> I took a crack at this and it's trickier than it looks. We have to pass
> a variadic function pointer (which is fine), but that means that they
> can't use handy macros like pr_err. We have to call functions that can
> take a va_list (which is also fine).
> 
> The part I'm having trouble with is incorporating the pr_fmt(). I've
> attached the patch I have on top of the current series. It doesn't
> compile for me. If I remove the pr_fmt() calls and just pass in the
> format string, it does compile.
> 
> What am I doing wrong here?
> 
> -------------------------8<---------------------------------
> 
> [PATCH] ref_tracker: have callers pass output function to pr_ostream
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  lib/ref_tracker.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
> index 4cc49cc21f5b..e131fdc51838 100644
> --- a/lib/ref_tracker.c
> +++ b/lib/ref_tracker.c
> @@ -77,21 +77,41 @@ struct ostream {
>  	char *buf;
>  	struct seq_file *seq;
>  	int size, used;
> +	void (*func)(struct ostream *stream, char *fmt, ...);
>  };
>  
> +#define ref_tracker_log(fmt, args) vprintk_emit(0, LOGLEVEL_ERR, NULL,
> pr_fmt(fmt), args)
> +
> +static void pr_ostream_log(struct ostream *stream, char *fmt, ...)
> +{
> +	va_list args;
> +
> +	ref_tracker_log(fmt, args);
> +}
> +

Oh, duh, I figured it out -- the problem is that the compiler can't
implicitly concatenate a string literal and a variable (of course).

I think we can just pass a prefix string in struct ostream that we can
add into the pr_ostream calls. I'll plan to send an updated set after a
bit more testing.
-- 
Jeff Layton <jlayton@kernel.org>