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 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);
+}
+
+#define ref_tracker_vsnprintf(buf, size, fmt, args) vsnprintf(buf,
size, pr_fmt(fmt), args)
+
+static void pr_ostream_buf(struct ostream *stream, char *fmt, ...)
+{
+	int ret, len = stream->size - stream->used;
+	va_list args;
+
+	ret = ref_tracker_vsnprintf(stream->buf + stream->used, len,
fmt, args);
+	stream->used += min(ret, len);
+}
+
+static void pr_ostream_seq(struct ostream *stream, char *fmt, ...)
+{
+	va_list args;
+
+	seq_vprintf(stream->seq, fmt, args);
+}
+
 #define pr_ostream(stream, fmt, args...) \
 ({ \
 	struct ostream *_s = (stream); \
 \
-	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; \
-		ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt),
##args); \
-		_s->used += min(ret, len); \
-	} \
+	_s->func(_s, fmt, ##args); \
 })
 
 static void
@@ -138,7 +158,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir
*dir,
 void ref_tracker_dir_print_locked(struct ref_tracker_dir *dir,
 				  unsigned int display_limit)
 {
-	struct ostream os = {};
+	struct ostream os = { .func = pr_ostream_log };
 
 	__ref_tracker_dir_pr_ostream(dir, display_limit, &os);
 }
@@ -157,7 +177,7 @@ EXPORT_SYMBOL(ref_tracker_dir_print);
 
 int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf,
size_t size)
 {
-	struct ostream os = { .buf = buf, .size = size };
+	struct ostream os = { .buf = buf, .size = size, .func =
pr_ostream_buf };
 	unsigned long flags;
 
 	spin_lock_irqsave(&dir->lock, flags);
@@ -294,7 +314,7 @@ EXPORT_SYMBOL_GPL(ref_tracker_free);
 
 static int ref_tracker_dir_seq_print(struct ref_tracker_dir *dir,
struct seq_file *seq)
 {
-	struct ostream os = { .seq = seq };
+	struct ostream os = { .seq = seq, .func = pr_ostream_seq };
 	unsigned long flags;
 
 	spin_lock_irqsave(&dir->lock, flags);
-- 
2.49.0