From: Jason Xing <kernelxing@tencent.com>

In this version, only support dumping the counter for buffer full and
implement the framework of how it works. Users MUST pass a valid @buf
with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
to acquire which information indicated by @flags to dump.

RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
choose to dump all the values.

Users can use this buffer to do whatever they expect in their own kernel
module, say, print to console/dmesg or write them into the relay buffer.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 include/linux/relay.h | 10 ++++++++++
 kernel/relay.c        | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index 022cf11e5a92..7a442c4cbead 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -31,6 +31,15 @@
 /*
  * Relay buffer error statistics dump
  */
+enum {
+	RELAY_DUMP_BUF_FULL = (1 << 0),
+
+	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
+	RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
+};
+
+#define RELAY_DUMP_BUF_MAX_LEN 32
+
 struct rchan_buf_error_stats
 {
 	unsigned int full;		/* counter for buffer full */
@@ -170,6 +179,7 @@ extern int relay_late_setup_files(struct rchan *chan,
 				  struct dentry *parent);
 extern void relay_close(struct rchan *chan);
 extern void relay_flush(struct rchan *chan);
+extern void relay_dump(struct rchan *chan, char *buf, int len, int flags);
 extern void relay_subbufs_consumed(struct rchan *chan,
 				   unsigned int cpu,
 				   size_t consumed);
diff --git a/kernel/relay.c b/kernel/relay.c
index b5db4aa60da1..0e675a77285c 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -810,6 +810,41 @@ void relay_flush(struct rchan *chan)
 }
 EXPORT_SYMBOL_GPL(relay_flush);
 
+/**
+ *	relay_dump - dump statistics of the specified channel buffer
+ *	@chan: the channel
+ *	@buf: buf to store statistics
+ *	@len: len of buf to check
+ *	@flags: select particular information to dump
+ */
+void relay_dump(struct rchan *chan, char *buf, int len, int flags)
+{
+	unsigned int i, full_counter = 0;
+	struct rchan_buf *rbuf;
+	int offset = 0;
+
+	if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
+		return;
+
+	if (len < RELAY_DUMP_BUF_MAX_LEN)
+		return;
+
+	if (chan->is_global) {
+		rbuf = *per_cpu_ptr(chan->buf, 0);
+		full_counter = rbuf->stats.full;
+	} else {
+		for_each_possible_cpu(i) {
+			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
+				full_counter += rbuf->stats.full;
+	}
+
+	if (flags & RELAY_DUMP_BUF_FULL)
+		offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
+
+	snprintf(buf + offset, 1, "\n");
+}
+EXPORT_SYMBOL_GPL(relay_dump);
+
 /**
  *	relay_file_open - open file op for relay files
  *	@inode: the inode
-- 
2.43.5

Hi Jason,

On Mon, 12 May 2025 10:49:32 +0800
Jason Xing <kerneljasonxing@gmail.com> wrote:

> From: Jason Xing <kernelxing@tencent.com>
> 
> In this version, only support dumping the counter for buffer full and
> implement the framework of how it works. Users MUST pass a valid @buf
> with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> to acquire which information indicated by @flags to dump.
> 
> RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> choose to dump all the values.
> 
> Users can use this buffer to do whatever they expect in their own kernel
> module, say, print to console/dmesg or write them into the relay buffer.
> 
> Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  include/linux/relay.h | 10 ++++++++++
>  kernel/relay.c        | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index 022cf11e5a92..7a442c4cbead 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -31,6 +31,15 @@
>  /*
>   * Relay buffer error statistics dump
>   */
> +enum {
> +	RELAY_DUMP_BUF_FULL = (1 << 0),
> +
> +	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> +	RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
> +};
> +
> +#define RELAY_DUMP_BUF_MAX_LEN 32
> +
>  struct rchan_buf_error_stats
>  {
>  	unsigned int full;		/* counter for buffer full */
> @@ -170,6 +179,7 @@ extern int relay_late_setup_files(struct rchan *chan,
>  				  struct dentry *parent);
>  extern void relay_close(struct rchan *chan);
>  extern void relay_flush(struct rchan *chan);
> +extern void relay_dump(struct rchan *chan, char *buf, int len, int flags);
>  extern void relay_subbufs_consumed(struct rchan *chan,
>  				   unsigned int cpu,
>  				   size_t consumed);
> diff --git a/kernel/relay.c b/kernel/relay.c
> index b5db4aa60da1..0e675a77285c 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -810,6 +810,41 @@ void relay_flush(struct rchan *chan)
>  }
>  EXPORT_SYMBOL_GPL(relay_flush);
>  
> +/**
> + *	relay_dump - dump statistics of the specified channel buffer
> + *	@chan: the channel
> + *	@buf: buf to store statistics
> + *	@len: len of buf to check
> + *	@flags: select particular information to dump
> + */
> +void relay_dump(struct rchan *chan, char *buf, int len, int flags)
> +{
> +	unsigned int i, full_counter = 0;
> +	struct rchan_buf *rbuf;
> +	int offset = 0;
> +
> +	if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> +		return;
> +
> +	if (len < RELAY_DUMP_BUF_MAX_LEN)
> +		return;
> +
> +	if (chan->is_global) {
> +		rbuf = *per_cpu_ptr(chan->buf, 0);
> +		full_counter = rbuf->stats.full;
> +	} else {
> +		for_each_possible_cpu(i) {
> +			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> +				full_counter += rbuf->stats.full;
> +	}
> +
> +	if (flags & RELAY_DUMP_BUF_FULL)
> +		offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> +
> +	snprintf(buf + offset, 1, "\n");

Is there any reason to return the value as string?
If it returns a digit value and the caller makes it a string,
it could be more flexible for other use cases.

Thank you,

> +}
> +EXPORT_SYMBOL_GPL(relay_dump);
> +
>  /**
>   *	relay_file_open - open file op for relay files
>   *	@inode: the inode
> -- 
> 2.43.5
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Hi Masami,

On Tue, May 13, 2025 at 5:58 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Jason,
>
> On Mon, 12 May 2025 10:49:32 +0800
> Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > In this version, only support dumping the counter for buffer full and
> > implement the framework of how it works. Users MUST pass a valid @buf
> > with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> > to acquire which information indicated by @flags to dump.
> >
> > RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> > choose to dump all the values.
> >
> > Users can use this buffer to do whatever they expect in their own kernel
> > module, say, print to console/dmesg or write them into the relay buffer.
> >
> > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  include/linux/relay.h | 10 ++++++++++
> >  kernel/relay.c        | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index 022cf11e5a92..7a442c4cbead 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -31,6 +31,15 @@
> >  /*
> >   * Relay buffer error statistics dump
> >   */
> > +enum {
> > +     RELAY_DUMP_BUF_FULL = (1 << 0),
> > +
> > +     RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> > +     RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
> > +};
> > +
> > +#define RELAY_DUMP_BUF_MAX_LEN 32
> > +
> >  struct rchan_buf_error_stats
> >  {
> >       unsigned int full;              /* counter for buffer full */
> > @@ -170,6 +179,7 @@ extern int relay_late_setup_files(struct rchan *chan,
> >                                 struct dentry *parent);
> >  extern void relay_close(struct rchan *chan);
> >  extern void relay_flush(struct rchan *chan);
> > +extern void relay_dump(struct rchan *chan, char *buf, int len, int flags);
> >  extern void relay_subbufs_consumed(struct rchan *chan,
> >                                  unsigned int cpu,
> >                                  size_t consumed);
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index b5db4aa60da1..0e675a77285c 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -810,6 +810,41 @@ void relay_flush(struct rchan *chan)
> >  }
> >  EXPORT_SYMBOL_GPL(relay_flush);
> >
> > +/**
> > + *   relay_dump - dump statistics of the specified channel buffer
> > + *   @chan: the channel
> > + *   @buf: buf to store statistics
> > + *   @len: len of buf to check
> > + *   @flags: select particular information to dump
> > + */
> > +void relay_dump(struct rchan *chan, char *buf, int len, int flags)
> > +{
> > +     unsigned int i, full_counter = 0;
> > +     struct rchan_buf *rbuf;
> > +     int offset = 0;
> > +
> > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > +             return;
> > +
> > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > +             return;
> > +
> > +     if (chan->is_global) {
> > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > +             full_counter = rbuf->stats.full;
> > +     } else {
> > +             for_each_possible_cpu(i) {
> > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > +                             full_counter += rbuf->stats.full;
> > +     }
> > +
> > +     if (flags & RELAY_DUMP_BUF_FULL)
> > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> > +
> > +     snprintf(buf + offset, 1, "\n");
>
> Is there any reason to return the value as string?
> If it returns a digit value and the caller makes it a string,
> it could be more flexible for other use cases.

Thanks for the feedback.

I will remove the above one as you pointed out in the next revision.
And it seems unnecessary to add '\0' at the end of the buffer like
"*buf = '\0';"?

While at it, I'm thinking if I can change the return value of
relay_dump() to "how many bytes do we actually write into the buffer"?
Does it sound better?

Thanks,
Jason

>
> Thank you,
>
> > +}
> > +EXPORT_SYMBOL_GPL(relay_dump);
> > +
> >  /**
> >   *   relay_file_open - open file op for relay files
> >   *   @inode: the inode
> > --
> > 2.43.5
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Tue, 13 May 2025 18:32:29 +0800
Jason Xing <kerneljasonxing@gmail.com> wrote:

> Hi Masami,
> 
> On Tue, May 13, 2025 at 5:58 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Jason,
> >
> > On Mon, 12 May 2025 10:49:32 +0800
> > Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > In this version, only support dumping the counter for buffer full and
> > > implement the framework of how it works. Users MUST pass a valid @buf
> > > with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> > > to acquire which information indicated by @flags to dump.
> > >
> > > RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> > > choose to dump all the values.
> > >
> > > Users can use this buffer to do whatever they expect in their own kernel
> > > module, say, print to console/dmesg or write them into the relay buffer.
> > >
> > > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  include/linux/relay.h | 10 ++++++++++
> > >  kernel/relay.c        | 35 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 45 insertions(+)
> > >
> > > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > > index 022cf11e5a92..7a442c4cbead 100644
> > > --- a/include/linux/relay.h
> > > +++ b/include/linux/relay.h
> > > @@ -31,6 +31,15 @@
> > >  /*
> > >   * Relay buffer error statistics dump
> > >   */
> > > +enum {
> > > +     RELAY_DUMP_BUF_FULL = (1 << 0),
> > > +
> > > +     RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> > > +     RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
> > > +};
> > > +
> > > +#define RELAY_DUMP_BUF_MAX_LEN 32
> > > +
> > >  struct rchan_buf_error_stats
> > >  {
> > >       unsigned int full;              /* counter for buffer full */
> > > @@ -170,6 +179,7 @@ extern int relay_late_setup_files(struct rchan *chan,
> > >                                 struct dentry *parent);
> > >  extern void relay_close(struct rchan *chan);
> > >  extern void relay_flush(struct rchan *chan);
> > > +extern void relay_dump(struct rchan *chan, char *buf, int len, int flags);
> > >  extern void relay_subbufs_consumed(struct rchan *chan,
> > >                                  unsigned int cpu,
> > >                                  size_t consumed);
> > > diff --git a/kernel/relay.c b/kernel/relay.c
> > > index b5db4aa60da1..0e675a77285c 100644
> > > --- a/kernel/relay.c
> > > +++ b/kernel/relay.c
> > > @@ -810,6 +810,41 @@ void relay_flush(struct rchan *chan)
> > >  }
> > >  EXPORT_SYMBOL_GPL(relay_flush);
> > >
> > > +/**
> > > + *   relay_dump - dump statistics of the specified channel buffer
> > > + *   @chan: the channel
> > > + *   @buf: buf to store statistics
> > > + *   @len: len of buf to check
> > > + *   @flags: select particular information to dump
> > > + */
> > > +void relay_dump(struct rchan *chan, char *buf, int len, int flags)
> > > +{
> > > +     unsigned int i, full_counter = 0;
> > > +     struct rchan_buf *rbuf;
> > > +     int offset = 0;
> > > +
> > > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > > +             return;
> > > +
> > > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > > +             return;
> > > +
> > > +     if (chan->is_global) {
> > > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > > +             full_counter = rbuf->stats.full;
> > > +     } else {
> > > +             for_each_possible_cpu(i) {
> > > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > > +                             full_counter += rbuf->stats.full;
> > > +     }
> > > +
> > > +     if (flags & RELAY_DUMP_BUF_FULL)
> > > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> > > +
> > > +     snprintf(buf + offset, 1, "\n");
> >
> > Is there any reason to return the value as string?
> > If it returns a digit value and the caller makes it a string,
> > it could be more flexible for other use cases.
> 
> Thanks for the feedback.
> 
> I will remove the above one as you pointed out in the next revision.
> And it seems unnecessary to add '\0' at the end of the buffer like
> "*buf = '\0';"?

Sorry, I think you missed my point. I meant it should be

/* Return the number of fullfilled buffer in the channel */
size_t relay_full(struct rchan *chan);

And keep the snprintf() in the blk_dropped_read() because that function
is responsible for formatting the output.

static ssize_t blk_dropped_read(struct file *filp, char __user *buffer,
 				size_t count, loff_t *ppos)
{
	struct blk_trace *bt = filp->private_data;
	char buf[16];
 
	snprintf(buf, sizeof(buf), "%zu\n", relay_full(bt->rchan));
 
 	return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
}

But the question is that what blk_subbuf_start_callback() counts
is really equal to what the relay_full() counts, because it seems
relay_full() just returns the current status of the channel, but
bt->dropped is the accumlated number of dropping events.

This means that if the client (reader) reads the subbufs the
number of full subbufs will be decreased, but the number of
dropped event does not change.

Can you recheck it?

Thank you,

> 
> While at it, I'm thinking if I can change the return value of
> relay_dump() to "how many bytes do we actually write into the buffer"?
> Does it sound better?
> 
> Thanks,
> Jason
> 
> >
> > Thank you,
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(relay_dump);
> > > +
> > >  /**
> > >   *   relay_file_open - open file op for relay files
> > >   *   @inode: the inode
> > > --
> > > 2.43.5
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Tue, May 13, 2025 at 9:22 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 13 May 2025 18:32:29 +0800
> Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > Hi Masami,
> >
> > On Tue, May 13, 2025 at 5:58 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > Hi Jason,
> > >
> > > On Mon, 12 May 2025 10:49:32 +0800
> > > Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > In this version, only support dumping the counter for buffer full and
> > > > implement the framework of how it works. Users MUST pass a valid @buf
> > > > with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> > > > to acquire which information indicated by @flags to dump.
> > > >
> > > > RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> > > > choose to dump all the values.
> > > >
> > > > Users can use this buffer to do whatever they expect in their own kernel
> > > > module, say, print to console/dmesg or write them into the relay buffer.
> > > >
> > > > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > >  include/linux/relay.h | 10 ++++++++++
> > > >  kernel/relay.c        | 35 +++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 45 insertions(+)
> > > >
> > > > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > > > index 022cf11e5a92..7a442c4cbead 100644
> > > > --- a/include/linux/relay.h
> > > > +++ b/include/linux/relay.h
> > > > @@ -31,6 +31,15 @@
> > > >  /*
> > > >   * Relay buffer error statistics dump
> > > >   */
> > > > +enum {
> > > > +     RELAY_DUMP_BUF_FULL = (1 << 0),
> > > > +
> > > > +     RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> > > > +     RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
> > > > +};
> > > > +
> > > > +#define RELAY_DUMP_BUF_MAX_LEN 32
> > > > +
> > > >  struct rchan_buf_error_stats
> > > >  {
> > > >       unsigned int full;              /* counter for buffer full */
> > > > @@ -170,6 +179,7 @@ extern int relay_late_setup_files(struct rchan *chan,
> > > >                                 struct dentry *parent);
> > > >  extern void relay_close(struct rchan *chan);
> > > >  extern void relay_flush(struct rchan *chan);
> > > > +extern void relay_dump(struct rchan *chan, char *buf, int len, int flags);
> > > >  extern void relay_subbufs_consumed(struct rchan *chan,
> > > >                                  unsigned int cpu,
> > > >                                  size_t consumed);
> > > > diff --git a/kernel/relay.c b/kernel/relay.c
> > > > index b5db4aa60da1..0e675a77285c 100644
> > > > --- a/kernel/relay.c
> > > > +++ b/kernel/relay.c
> > > > @@ -810,6 +810,41 @@ void relay_flush(struct rchan *chan)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(relay_flush);
> > > >
> > > > +/**
> > > > + *   relay_dump - dump statistics of the specified channel buffer
> > > > + *   @chan: the channel
> > > > + *   @buf: buf to store statistics
> > > > + *   @len: len of buf to check
> > > > + *   @flags: select particular information to dump
> > > > + */
> > > > +void relay_dump(struct rchan *chan, char *buf, int len, int flags)
> > > > +{
> > > > +     unsigned int i, full_counter = 0;
> > > > +     struct rchan_buf *rbuf;
> > > > +     int offset = 0;
> > > > +
> > > > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > > > +             return;
> > > > +
> > > > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > > > +             return;
> > > > +
> > > > +     if (chan->is_global) {
> > > > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > > > +             full_counter = rbuf->stats.full;
> > > > +     } else {
> > > > +             for_each_possible_cpu(i) {
> > > > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > > > +                             full_counter += rbuf->stats.full;
> > > > +     }
> > > > +
> > > > +     if (flags & RELAY_DUMP_BUF_FULL)
> > > > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> > > > +
> > > > +     snprintf(buf + offset, 1, "\n");
> > >
> > > Is there any reason to return the value as string?
> > > If it returns a digit value and the caller makes it a string,
> > > it could be more flexible for other use cases.
> >
> > Thanks for the feedback.
> >
> > I will remove the above one as you pointed out in the next revision.
> > And it seems unnecessary to add '\0' at the end of the buffer like
> > "*buf = '\0';"?
>
> Sorry, I think you missed my point. I meant it should be
>
> /* Return the number of fullfilled buffer in the channel */
> size_t relay_full(struct rchan *chan);
>
> And keep the snprintf() in the blk_dropped_read() because that function
> is responsible for formatting the output.

Oh, sorry, it's not what I want because (please see patch [4/5] [1])
relay_dump() works to print various statistics of the buffer. In this
patch, 'full' counter is the first one.

[1]: https://lore.kernel.org/all/20250512024935.64704-5-kerneljasonxing@gmail.com/

>
> static ssize_t blk_dropped_read(struct file *filp, char __user *buffer,
>                                 size_t count, loff_t *ppos)
> {
>         struct blk_trace *bt = filp->private_data;
>         char buf[16];
>
>         snprintf(buf, sizeof(buf), "%zu\n", relay_full(bt->rchan));
>
>         return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
> }
>
> But the question is that what blk_subbuf_start_callback() counts
> is really equal to what the relay_full() counts, because it seems
> relay_full() just returns the current status of the channel, but
> bt->dropped is the accumlated number of dropping events.
>
> This means that if the client (reader) reads the subbufs the
> number of full subbufs will be decreased, but the number of
> dropped event does not change.

At least in this series, I didn't give the 'full' counter any chance
to decrement, which means it's compatible with blktrace, unless
__relay_reset() is triggered :)

While discussing with you on this point, I suddenly recalled that in
some network drivers, they implemented 'wake' and 'stop' as a pair to
know what the current status of a certain queue is. I think I can add
a similar thing to the buffer about 1) how many times are the buffer
full, 2) how many times does the buffer get rid of being full.

Thanks,
Jason

>
> Can you recheck it?
>
> Thank you,
>
> >
> > While at it, I'm thinking if I can change the return value of
> > relay_dump() to "how many bytes do we actually write into the buffer"?
> > Does it sound better?
> >
> > Thanks,
> > Jason
> >
> > >
> > > Thank you,
> > >
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(relay_dump);
> > > > +
> > > >  /**
> > > >   *   relay_file_open - open file op for relay files
> > > >   *   @inode: the inode
> > > > --
> > > > 2.43.5
> > > >
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Tue, 13 May 2025 21:46:25 +0800
Jason Xing <kerneljasonxing@gmail.com> wrote:

> > > > > +
> > > > > +     if (flags & RELAY_DUMP_BUF_FULL)
> > > > > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> > > > > +
> > > > > +     snprintf(buf + offset, 1, "\n");
> > > >
> > > > Is there any reason to return the value as string?
> > > > If it returns a digit value and the caller makes it a string,
> > > > it could be more flexible for other use cases.
> > >
> > > Thanks for the feedback.
> > >
> > > I will remove the above one as you pointed out in the next revision.
> > > And it seems unnecessary to add '\0' at the end of the buffer like
> > > "*buf = '\0';"?
> >
> > Sorry, I think you missed my point. I meant it should be
> >
> > /* Return the number of fullfilled buffer in the channel */
> > size_t relay_full(struct rchan *chan);
> >
> > And keep the snprintf() in the blk_dropped_read() because that function
> > is responsible for formatting the output.
> 
> Oh, sorry, it's not what I want because (please see patch [4/5] [1])
> relay_dump() works to print various statistics of the buffer. In this
> patch, 'full' counter is the first one.
> 
> [1]: https://lore.kernel.org/all/20250512024935.64704-5-kerneljasonxing@gmail.com/

Yes, that's why I asked you to make it just returning raw value.
The string formatting of those values is blk_dropped_read()s business
(because it is a 'read' callback), not for relayfs.

For example, other relayfs user wants to summarize the values or
calculate the percentage form that value. Also, we don't need to
bother to check the buffer size etc, because blk_dropped_read()
knows that.

> 
> >
> > static ssize_t blk_dropped_read(struct file *filp, char __user *buffer,
> >                                 size_t count, loff_t *ppos)
> > {
> >         struct blk_trace *bt = filp->private_data;
> >         char buf[16];
> >
> >         snprintf(buf, sizeof(buf), "%zu\n", relay_full(bt->rchan));
> >
> >         return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
> > }
> >
> > But the question is that what blk_subbuf_start_callback() counts
> > is really equal to what the relay_full() counts, because it seems
> > relay_full() just returns the current status of the channel, but
> > bt->dropped is the accumlated number of dropping events.
> >
> > This means that if the client (reader) reads the subbufs the
> > number of full subbufs will be decreased, but the number of
> > dropped event does not change.
> 
> At least in this series, I didn't give the 'full' counter any chance
> to decrement, which means it's compatible with blktrace, unless
> __relay_reset() is triggered :)

Ah, OK. I missed what [1/5] does. I agree that this does the
same thing.

> 
> While discussing with you on this point, I suddenly recalled that in
> some network drivers, they implemented 'wake' and 'stop' as a pair to
> know what the current status of a certain queue is. I think I can add
> a similar thing to the buffer about 1) how many times are the buffer
> full, 2) how many times does the buffer get rid of being full.

Sounds nice.

Thank you,

> 
> Thanks,
> Jason
> 
> >
> > Can you recheck it?
> >
> > Thank you,
> >
> > >
> > > While at it, I'm thinking if I can change the return value of
> > > relay_dump() to "how many bytes do we actually write into the buffer"?
> > > Does it sound better?
> > >
> > > Thanks,
> > > Jason
> > >
> > > >
> > > > Thank you,
> > > >
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(relay_dump);
> > > > > +
> > > > >  /**
> > > > >   *   relay_file_open - open file op for relay files
> > > > >   *   @inode: the inode
> > > > > --
> > > > > 2.43.5
> > > > >
> > > >
> > > >
> > > > --
> > > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Wed, May 14, 2025 at 9:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 13 May 2025 21:46:25 +0800
> Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > > > > > +
> > > > > > +     if (flags & RELAY_DUMP_BUF_FULL)
> > > > > > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
> > > > > > +
> > > > > > +     snprintf(buf + offset, 1, "\n");
> > > > >
> > > > > Is there any reason to return the value as string?
> > > > > If it returns a digit value and the caller makes it a string,
> > > > > it could be more flexible for other use cases.
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > I will remove the above one as you pointed out in the next revision.
> > > > And it seems unnecessary to add '\0' at the end of the buffer like
> > > > "*buf = '\0';"?
> > >
> > > Sorry, I think you missed my point. I meant it should be
> > >
> > > /* Return the number of fullfilled buffer in the channel */
> > > size_t relay_full(struct rchan *chan);
> > >
> > > And keep the snprintf() in the blk_dropped_read() because that function
> > > is responsible for formatting the output.
> >
> > Oh, sorry, it's not what I want because (please see patch [4/5] [1])
> > relay_dump() works to print various statistics of the buffer. In this
> > patch, 'full' counter is the first one.
> >
> > [1]: https://lore.kernel.org/all/20250512024935.64704-5-kerneljasonxing@gmail.com/
>
> Yes, that's why I asked you to make it just returning raw value.
> The string formatting of those values is blk_dropped_read()s business
> (because it is a 'read' callback), not for relayfs.
>
> For example, other relayfs user wants to summarize the values or
> calculate the percentage form that value. Also, we don't need to
> bother to check the buffer size etc, because blk_dropped_read()
> knows that.

Oh, it makes sense. Thanks.

I will make relay_dump() return raw value which depends on what kind
of flag caller passes, like RELAY_DUMP_BUF_FULL or RELAY_DUMP_WRT_BIG
or even more other info. On top of that, I can then get rid of the
annoying buffer-related code snippets :)

Thanks,
Jason

>
> >
> > >
> > > static ssize_t blk_dropped_read(struct file *filp, char __user *buffer,
> > >                                 size_t count, loff_t *ppos)
> > > {
> > >         struct blk_trace *bt = filp->private_data;
> > >         char buf[16];
> > >
> > >         snprintf(buf, sizeof(buf), "%zu\n", relay_full(bt->rchan));
> > >
> > >         return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
> > > }
> > >
> > > But the question is that what blk_subbuf_start_callback() counts
> > > is really equal to what the relay_full() counts, because it seems
> > > relay_full() just returns the current status of the channel, but
> > > bt->dropped is the accumlated number of dropping events.
> > >
> > > This means that if the client (reader) reads the subbufs the
> > > number of full subbufs will be decreased, but the number of
> > > dropped event does not change.
> >
> > At least in this series, I didn't give the 'full' counter any chance
> > to decrement, which means it's compatible with blktrace, unless
> > __relay_reset() is triggered :)
>
> Ah, OK. I missed what [1/5] does. I agree that this does the
> same thing.
>
> >
> > While discussing with you on this point, I suddenly recalled that in
> > some network drivers, they implemented 'wake' and 'stop' as a pair to
> > know what the current status of a certain queue is. I think I can add
> > a similar thing to the buffer about 1) how many times are the buffer
> > full, 2) how many times does the buffer get rid of being full.
>
> Sounds nice.
>
> Thank you,
>
> >
> > Thanks,
> > Jason
> >
> > >
> > > Can you recheck it?
> > >
> > > Thank you,
> > >
> > > >
> > > > While at it, I'm thinking if I can change the return value of
> > > > relay_dump() to "how many bytes do we actually write into the buffer"?
> > > > Does it sound better?
> > > >
> > > > Thanks,
> > > > Jason
> > > >
> > > > >
> > > > > Thank you,
> > > > >
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(relay_dump);
> > > > > > +
> > > > > >  /**
> > > > > >   *   relay_file_open - open file op for relay files
> > > > > >   *   @inode: the inode
> > > > > > --
> > > > > > 2.43.5
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

On Mon, 12 May 2025 10:49:32 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:

> From: Jason Xing <kernelxing@tencent.com>
> 
> In this version, only support dumping the counter for buffer full and
> implement the framework of how it works. Users MUST pass a valid @buf
> with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> to acquire which information indicated by @flags to dump.
> 
> RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> choose to dump all the values.
> 
> Users can use this buffer to do whatever they expect in their own kernel
> module, say, print to console/dmesg or write them into the relay buffer.
> 
> ...
>  
> +/**
> + *	relay_dump - dump statistics of the specified channel buffer
> + *	@chan: the channel
> + *	@buf: buf to store statistics
> + *	@len: len of buf to check
> + *	@flags: select particular information to dump
> + */
> +void relay_dump(struct rchan *chan, char *buf, int len, int flags)

`size_t' is probably a more appropriate type for `len'.

> +{
> +	unsigned int i, full_counter = 0;
> +	struct rchan_buf *rbuf;
> +	int offset = 0;
> +
> +	if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> +		return;
> +
> +	if (len < RELAY_DUMP_BUF_MAX_LEN)
> +		return;

So we left the memory at *buf uninitialized but failed to tell the
caller this.  The caller will then proceed to use uninitialized memory.

It's a programming error, so simply going BUG seems OK.

> +	if (chan->is_global) {
> +		rbuf = *per_cpu_ptr(chan->buf, 0);
> +		full_counter = rbuf->stats.full;
> +	} else {
> +		for_each_possible_cpu(i) {

I'm thinking that at this stage in the patch series, this should be
for_each_online_cpu(), then adjust that in patch [5/5].

> +			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> +				full_counter += rbuf->stats.full;
> +	}
> +
> +	if (flags & RELAY_DUMP_BUF_FULL)
> +		offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);

This seems strange.  sizeof(unsigned int) has nothing to do with the
number of characters which are consumed by expansion of "%u"?

> +
> +	snprintf(buf + offset, 1, "\n");
> +}
> +EXPORT_SYMBOL_GPL(relay_dump);
> +
>  /**
>   *	relay_file_open - open file op for relay files
>   *	@inode: the inode

On Tue, May 13, 2025 at 8:51 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 12 May 2025 10:49:32 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > In this version, only support dumping the counter for buffer full and
> > implement the framework of how it works. Users MUST pass a valid @buf
> > with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
> > to acquire which information indicated by @flags to dump.
> >
> > RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
> > choose to dump all the values.
> >
> > Users can use this buffer to do whatever they expect in their own kernel
> > module, say, print to console/dmesg or write them into the relay buffer.
> >
> > ...
> >
> > +/**
> > + *   relay_dump - dump statistics of the specified channel buffer
> > + *   @chan: the channel
> > + *   @buf: buf to store statistics
> > + *   @len: len of buf to check
> > + *   @flags: select particular information to dump
> > + */
> > +void relay_dump(struct rchan *chan, char *buf, int len, int flags)
>
> `size_t' is probably a more appropriate type for `len'.
>
> > +{
> > +     unsigned int i, full_counter = 0;
> > +     struct rchan_buf *rbuf;
> > +     int offset = 0;
> > +
> > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > +             return;
> > +
> > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > +             return;
>
> So we left the memory at *buf uninitialized but failed to tell the
> caller this.  The caller will then proceed to use uninitialized memory.
>
> It's a programming error, so simply going BUG seems OK.

Are you suggesting that I should remove the above check because
developers should take care of the length of the buffer to write
outside of the relay_dump function? or use this instead:
WARN_ON_ONCE(len < RELAY_DUMP_BUF_MAX_LEN);
?

>
> > +     if (chan->is_global) {
> > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > +             full_counter = rbuf->stats.full;
> > +     } else {
> > +             for_each_possible_cpu(i) {
>
> I'm thinking that at this stage in the patch series, this should be
> for_each_online_cpu(), then adjust that in patch [5/5].

Point taken. Will change it.

>
> > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > +                             full_counter += rbuf->stats.full;
> > +     }
> > +
> > +     if (flags & RELAY_DUMP_BUF_FULL)
> > +             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
>
> This seems strange.  sizeof(unsigned int) has nothing to do with the
> number of characters which are consumed by expansion of "%u"?

Sorry, my bad. It should be:
offset += snprintf(buf, len, "%u", full_counter);

Passing 'len' as the second parameter.

Thanks,
Jason

>
> > +
> > +     snprintf(buf + offset, 1, "\n");
> > +}
> > +EXPORT_SYMBOL_GPL(relay_dump);
> > +
> >  /**
> >   *   relay_file_open - open file op for relay files
> >   *   @inode: the inode
>

On Tue, 13 May 2025 09:48:15 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:

> > > +{
> > > +     unsigned int i, full_counter = 0;
> > > +     struct rchan_buf *rbuf;
> > > +     int offset = 0;
> > > +
> > > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > > +             return;
> > > +
> > > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > > +             return;
> >
> > So we left the memory at *buf uninitialized but failed to tell the
> > caller this.  The caller will then proceed to use uninitialized memory.
> >
> > It's a programming error, so simply going BUG seems OK.
> 
> Are you suggesting that I should remove the above check because
> developers should take care of the length of the buffer to write
> outside of the relay_dump function? or use this instead:
> WARN_ON_ONCE(len < RELAY_DUMP_BUF_MAX_LEN);
> ?

It's a poor interface - it returns uninitialized data while not
alerting the caller to this.  You'll figure something out ;)

Perhaps

	BUG_ON(len < RELAY_DUMP_BUF_MAX_LEN);
	*buf = '\0';
	if (!chan || (flags & ~RELAY_DUMP_MASK))
		return;

We don't need to check for !buf - the oops message contains the same info.

Maybe we don't need to check !chan either.  Can it be NULL here?

On Tue, May 13, 2025 at 10:04 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Tue, 13 May 2025 09:48:15 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > > > +{
> > > > +     unsigned int i, full_counter = 0;
> > > > +     struct rchan_buf *rbuf;
> > > > +     int offset = 0;
> > > > +
> > > > +     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
> > > > +             return;
> > > > +
> > > > +     if (len < RELAY_DUMP_BUF_MAX_LEN)
> > > > +             return;
> > >
> > > So we left the memory at *buf uninitialized but failed to tell the
> > > caller this.  The caller will then proceed to use uninitialized memory.
> > >
> > > It's a programming error, so simply going BUG seems OK.
> >
> > Are you suggesting that I should remove the above check because
> > developers should take care of the length of the buffer to write
> > outside of the relay_dump function? or use this instead:
> > WARN_ON_ONCE(len < RELAY_DUMP_BUF_MAX_LEN);
> > ?
>
> It's a poor interface - it returns uninitialized data while not
> alerting the caller to this.  You'll figure something out ;)
>
> Perhaps
>
>         BUG_ON(len < RELAY_DUMP_BUF_MAX_LEN);

I'm unsure if BUG_ON is appropriate here since technically speaking
it's not a bug. For now, only sizeof(u32) is used in the buffer.

>         *buf = '\0';
>         if (!chan || (flags & ~RELAY_DUMP_MASK))
>                 return;
>
> We don't need to check for !buf - the oops message contains the same info.

Got it. Thanks.

>
> Maybe we don't need to check !chan either.  Can it be NULL here?

It depends on how users call this. If users call this without
initialization of chan, relay_dump() can avoid the crash. It works
like kfree() which prevents the NULL object from being freed.

BTW, should I merge this commit [1] into the series in V2 so that you
can easily review?

[1]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/

Thanks,
Jason

On Tue, 13 May 2025 10:26:45 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:

> >
> > Maybe we don't need to check !chan either.  Can it be NULL here?
> 
> It depends on how users call this. If users call this without
> initialization of chan, relay_dump() can avoid the crash. It works
> like kfree() which prevents the NULL object from being freed.

hm, OK.  Generally, I don't think that kernel code should be very
tolerant of bugs in the caller.  If the caller passes us bad stuff then
that's the caller's fault and the caller should be fixed.  If the
client code responds to bad input with a nice solid oops, then great! 
The caller will get fixed.

> BTW, should I merge this commit [1] into the series in V2 so that you
> can easily review?
> 
> [1]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/

That seems unrelated to this work so it seems inappropriate to combine
the two.

I skipped [1] because I'm waiting for overall clarity on what's
happening with relay[fs].

On Tue, May 13, 2025 at 11:41 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Tue, 13 May 2025 10:26:45 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > >
> > > Maybe we don't need to check !chan either.  Can it be NULL here?
> >
> > It depends on how users call this. If users call this without
> > initialization of chan, relay_dump() can avoid the crash. It works
> > like kfree() which prevents the NULL object from being freed.
>
> hm, OK.  Generally, I don't think that kernel code should be very
> tolerant of bugs in the caller.  If the caller passes us bad stuff then
> that's the caller's fault and the caller should be fixed.  If the
> client code responds to bad input with a nice solid oops, then great!
> The caller will get fixed.

I learned. Thanks. I will skip the check for that.

>
> > BTW, should I merge this commit [1] into the series in V2 so that you
> > can easily review?
> >
> > [1]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/
>
> That seems unrelated to this work so it seems inappropriate to combine
> the two.

This series is built on top of [1].

>
> I skipped [1] because I'm waiting for overall clarity on what's
> happening with relay[fs].

Do you refer to this thread[2]? Well, that conversation/reply made me
feel lost. I believe you've already seen that. If so, it seems we're
working on the dead code together....

[2]: https://lore.kernel.org/all/70293376-71b0-4b9d-b3c1-224b640f470b@kernel.dk/

Thanks,
Jason