[PATCH v2 0/4] relayfs: misc changes

[PATCH v2 0/4] relayfs: misc changes

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

The series mostly focuss on the error counters which helps every user
debug their own kernel module. More patches making the relayfs more
robust and functional are around the corner :)

---
Note: this series is made on top of this cleanup[1] and unmerged commit[1]
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/log/?h=mm-nonmm-unstable
[2]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/

*** BLURB HERE ***

Jason Xing (4):
  relayfs: support a counter tracking if per-cpu buffers is full
  relayfs: introduce dump of relayfs statistics function
  blktrace: use rbuf->stats.full as a drop indicator in relayfs
  relayfs: support a counter tracking if data is too big to write

 include/linux/relay.h   | 19 ++++++++++++++-
 kernel/relay.c          | 51 +++++++++++++++++++++++++++++++++++------
 kernel/trace/blktrace.c | 22 ++----------------
 3 files changed, 64 insertions(+), 28 deletions(-)

-- 
2.43.5

[PATCH v2 1/4] relayfs: support a counter tracking if per-cpu buffers is full

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

When using relay mechanism, we often encounter the case where new data
are lost or old unconsumed data are overwritten because of slow reader.

Add 'full' field in per-cpu buffer structure to detect if the above case
is happening.  Relay has two modes: 1) non-overwrite mode, 2) overwrite
mode. So buffer being full here respectively means: 1) relayfs doesn't
intend to accept new data and then simply drop them, or 2) relayfs is
going to start over again and overwrite old unread data with new data.

Note: this counter doesn't need any explicit lock to protect from being
modified by different threads for the better performance consideration.
Writers calling __relay_write/relay_write should consider how to use
the lock and ensure it performs under the lock protection, thus it's
not necessary to add a new small lock here.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v2
1. rename full member
2. revise the commit log
I still chose to keep struct rchan_buf_stats to help later patches
implementing relay_dump() to dump more statistics from buffer. It means
more fields can/will be added. But I have to say, I have no strong favor
on this kind of organization. If you don't like it, I will remove in the
next re-spin :)
---
 include/linux/relay.h | 9 +++++++++
 kernel/relay.c        | 8 +++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index e10a0fdf4325..ce7a1b396872 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -28,6 +28,14 @@
  */
 #define RELAYFS_CHANNEL_VERSION		7
 
+/*
+ * Relay buffer statistics dump
+ */
+struct rchan_buf_stats
+{
+	unsigned int full_count;	/* counter for buffer full */
+};
+
 /*
  * Per-cpu relay channel buffer
  */
@@ -43,6 +51,7 @@ struct rchan_buf
 	struct irq_work wakeup_work;	/* reader wakeup */
 	struct dentry *dentry;		/* channel file dentry */
 	struct kref kref;		/* channel buffer refcount */
+	struct rchan_buf_stats stats;	/* buffer stats */
 	struct page **page_array;	/* array of current buffer pages */
 	unsigned int page_count;	/* number of current buffer pages */
 	unsigned int finalized;		/* buffer has been finalized */
diff --git a/kernel/relay.c b/kernel/relay.c
index 94f79f52d826..eb3f630f3896 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -252,8 +252,13 @@ EXPORT_SYMBOL_GPL(relay_buf_full);
 static int relay_subbuf_start(struct rchan_buf *buf, void *subbuf,
 			      void *prev_subbuf)
 {
+	int full = relay_buf_full(buf);
+
+	if (full)
+		buf->stats.full_count++;
+
 	if (!buf->chan->cb->subbuf_start)
-		return !relay_buf_full(buf);
+		return !full;
 
 	return buf->chan->cb->subbuf_start(buf, subbuf,
 					   prev_subbuf);
@@ -298,6 +303,7 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
 	buf->finalized = 0;
 	buf->data = buf->start;
 	buf->offset = 0;
+	buf->stats.full_count = 0;
 
 	for (i = 0; i < buf->chan->n_subbufs; i++)
 		buf->padding[i] = 0;
-- 
2.43.5

[PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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 can pass certain flag to fetch what field/statistics they expect
to know. Each time it only returns one result. So do not pass multiple
flags.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v2
1. refactor relay_dump() and make it only return a pure size_t result
of the value that users specifies.
2. revise the commit log.
---
 include/linux/relay.h |  7 +++++++
 kernel/relay.c        | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index ce7a1b396872..3fb285716e34 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -31,6 +31,12 @@
 /*
  * Relay buffer statistics dump
  */
+enum {
+	RELAY_DUMP_BUF_FULL = (1 << 0),
+
+	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
+};
+
 struct rchan_buf_stats
 {
 	unsigned int full_count;	/* counter for buffer full */
@@ -167,6 +173,7 @@ struct rchan *relay_open(const char *base_filename,
 			 void *private_data);
 extern void relay_close(struct rchan *chan);
 extern void relay_flush(struct rchan *chan);
+extern size_t relay_dump(struct rchan *chan, 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 eb3f630f3896..f47fc750e559 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -701,6 +701,37 @@ void relay_flush(struct rchan *chan)
 }
 EXPORT_SYMBOL_GPL(relay_flush);
 
+/**
+ *	relay_dump - dump channel buffer statistics
+ *	@chan: the channel
+ *	@flags: select particular information to dump
+ *
+ *	Returns the count of certain field that caller specifies.
+ */
+size_t relay_dump(struct rchan *chan, int flags)
+{
+	unsigned int i, count = 0;
+	struct rchan_buf *rbuf;
+
+	if (!chan || flags > RELAY_DUMP_LAST)
+		return 0;
+
+	if (chan->is_global) {
+		rbuf = *per_cpu_ptr(chan->buf, 0);
+		if (flags & RELAY_DUMP_BUF_FULL)
+			count = rbuf->stats.full_count;
+	} else {
+		for_each_online_cpu(i) {
+			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
+				if (flags & RELAY_DUMP_BUF_FULL)
+					count += rbuf->stats.full_count;
+		}
+	}
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(relay_dump);
+
 /**
  *	relay_file_open - open file op for relay files
  *	@inode: the inode
-- 
2.43.5

[PATCH v2 3/4] blktrace: use rbuf->stats.full as a drop indicator in relayfs

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Replace internal subbuf_start in blktrace with the default policy
in relayfs.

Remove dropped field from struct blktrace. Correspondingly, call the
common helper in relay. Through incrementing full_count to keep track
of how many times we encountered a full buffer issue, it aids the user
space app in telling how many lost events appear.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v2
1. adjust accordingly together with patch 2.
---
 kernel/trace/blktrace.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index d1a89714e805..f4c103aceacd 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -415,9 +415,10 @@ 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;
+	size_t dropped = relay_dump(bt->rchan, RELAY_DUMP_BUF_FULL);
 	char buf[16];
 
-	snprintf(buf, sizeof(buf), "%u\n", atomic_read(&bt->dropped));
+	snprintf(buf, sizeof(buf), "%lu\n", dropped);
 
 	return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
 }
@@ -456,23 +457,6 @@ static const struct file_operations blk_msg_fops = {
 	.llseek =	noop_llseek,
 };
 
-/*
- * Keep track of how many times we encountered a full subbuffer, to aid
- * the user space app in telling how many lost events there were.
- */
-static int blk_subbuf_start_callback(struct rchan_buf *buf, void *subbuf,
-				     void *prev_subbuf)
-{
-	struct blk_trace *bt;
-
-	if (!relay_buf_full(buf))
-		return 1;
-
-	bt = buf->chan->private_data;
-	atomic_inc(&bt->dropped);
-	return 0;
-}
-
 static int blk_remove_buf_file_callback(struct dentry *dentry)
 {
 	debugfs_remove(dentry);
@@ -491,7 +475,6 @@ static struct dentry *blk_create_buf_file_callback(const char *filename,
 }
 
 static const struct rchan_callbacks blk_relay_callbacks = {
-	.subbuf_start		= blk_subbuf_start_callback,
 	.create_buf_file	= blk_create_buf_file_callback,
 	.remove_buf_file	= blk_remove_buf_file_callback,
 };
@@ -580,7 +563,6 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	}
 
 	bt->dev = dev;
-	atomic_set(&bt->dropped, 0);
 	INIT_LIST_HEAD(&bt->running_list);
 
 	ret = -EIO;
-- 
2.43.5

[PATCH v2 4/4] relayfs: support a counter tracking if data is too big to write

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

It really doesn't matter if the user/admin knows what the last too
big value is. Record how many times this case is triggered would be
helpful.

Solve the existing issue where relay_reset() doesn't restore
the value.

Store the counter in the per-cpu buffer structure instead of the global
buffer structure. It also solves the racy condition which is likely
to happen when a few of per-cpu buffers encounter the too big data case
and then access the global field last_toobig without lock protection.

Remove the printk in relay_close() since kernel module can directly call
relay_dump() as they want.

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

diff --git a/include/linux/relay.h b/include/linux/relay.h
index 3fb285716e34..08c1888c9c1a 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -33,13 +33,15 @@
  */
 enum {
 	RELAY_DUMP_BUF_FULL = (1 << 0),
+	RELAY_DUMP_WRT_BIG = (1 << 1),
 
-	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
+	RELAY_DUMP_LAST = RELAY_DUMP_WRT_BIG,
 };
 
 struct rchan_buf_stats
 {
 	unsigned int full_count;	/* counter for buffer full */
+	unsigned int big_count;		/* counter for too big to write */
 };
 
 /*
@@ -79,7 +81,6 @@ struct rchan
 	const struct rchan_callbacks *cb; /* client callbacks */
 	struct kref kref;		/* channel refcount */
 	void *private_data;		/* for user-defined data */
-	size_t last_toobig;		/* tried to log event > subbuf size */
 	struct rchan_buf * __percpu *buf; /* per-cpu channel buffers */
 	int is_global;			/* One global buffer ? */
 	struct list_head list;		/* for channel list */
diff --git a/kernel/relay.c b/kernel/relay.c
index f47fc750e559..f4d1dcac164a 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -304,6 +304,7 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
 	buf->data = buf->start;
 	buf->offset = 0;
 	buf->stats.full_count = 0;
+	buf->stats.big_count = 0;
 
 	for (i = 0; i < buf->chan->n_subbufs; i++)
 		buf->padding[i] = 0;
@@ -603,7 +604,7 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
 	return length;
 
 toobig:
-	buf->chan->last_toobig = length;
+	buf->stats.big_count++;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(relay_switch_subbuf);
@@ -663,11 +664,6 @@ void relay_close(struct rchan *chan)
 			if ((buf = *per_cpu_ptr(chan->buf, i)))
 				relay_close_buf(buf);
 
-	if (chan->last_toobig)
-		printk(KERN_WARNING "relay: one or more items not logged "
-		       "[item size (%zd) > sub-buffer size (%zd)]\n",
-		       chan->last_toobig, chan->subbuf_size);
-
 	list_del(&chan->list);
 	kref_put(&chan->kref, relay_destroy_channel);
 	mutex_unlock(&relay_channels_mutex);
@@ -720,11 +716,15 @@ size_t relay_dump(struct rchan *chan, int flags)
 		rbuf = *per_cpu_ptr(chan->buf, 0);
 		if (flags & RELAY_DUMP_BUF_FULL)
 			count = rbuf->stats.full_count;
+		else if (flags & RELAY_DUMP_WRT_BIG)
+			count = rbuf->stats.big_count;
 	} else {
 		for_each_online_cpu(i) {
 			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
 				if (flags & RELAY_DUMP_BUF_FULL)
 					count += rbuf->stats.full_count;
+				else if (flags & RELAY_DUMP_WRT_BIG)
+					count += rbuf->stats.big_count;
 		}
 	}
 
-- 
2.43.5

Re: [PATCH v2 1/4] relayfs: support a counter tracking if per-cpu buffers is full

[Jens Axboe]

Looks fine:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jens Axboe]

On 5/15/25 12:16 AM, Jason Xing 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 can pass certain flag to fetch what field/statistics they expect
> to know. Each time it only returns one result. So do not pass multiple
> flags.
> 
> Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> v2
> 1. refactor relay_dump() and make it only return a pure size_t result
> of the value that users specifies.
> 2. revise the commit log.
> ---
>  include/linux/relay.h |  7 +++++++
>  kernel/relay.c        | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index ce7a1b396872..3fb285716e34 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -31,6 +31,12 @@
>  /*
>   * Relay buffer statistics dump
>   */
> +enum {
> +	RELAY_DUMP_BUF_FULL = (1 << 0),
> +
> +	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> +};
> +
>  struct rchan_buf_stats
>  {
>  	unsigned int full_count;	/* counter for buffer full */
> @@ -167,6 +173,7 @@ struct rchan *relay_open(const char *base_filename,
>  			 void *private_data);
>  extern void relay_close(struct rchan *chan);
>  extern void relay_flush(struct rchan *chan);
> +extern size_t relay_dump(struct rchan *chan, 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 eb3f630f3896..f47fc750e559 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -701,6 +701,37 @@ void relay_flush(struct rchan *chan)
>  }
>  EXPORT_SYMBOL_GPL(relay_flush);
>  
> +/**
> + *	relay_dump - dump channel buffer statistics
> + *	@chan: the channel
> + *	@flags: select particular information to dump
> + *
> + *	Returns the count of certain field that caller specifies.
> + */
> +size_t relay_dump(struct rchan *chan, int flags)
> +{
> +	unsigned int i, count = 0;
> +	struct rchan_buf *rbuf;
> +
> +	if (!chan || flags > RELAY_DUMP_LAST)
> +		return 0;
> +
> +	if (chan->is_global) {
> +		rbuf = *per_cpu_ptr(chan->buf, 0);
> +		if (flags & RELAY_DUMP_BUF_FULL)
> +			count = rbuf->stats.full_count;
> +	} else {
> +		for_each_online_cpu(i) {
> +			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> +				if (flags & RELAY_DUMP_BUF_FULL)
> +					count += rbuf->stats.full_count;

Kernel tends to avoid the rolled-into-one assignment and check, it's
easy to misread. This:

	rbuf = *per_cpu_ptr(chan->buf, i);
	if (rbuf && flags & RELAY_DUMP_BUF_FULL)
		count += rbuf->stats.full_count;

reads much easier. IMHO.

-- 
Jens Axboe

Re: [PATCH v2 3/4] blktrace: use rbuf->stats.full as a drop indicator in relayfs

[Jens Axboe]

Looks fine:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH v2 3/4] blktrace: use rbuf->stats.full as a drop indicator in relayfs

[Jens Axboe]

On 5/15/25 12:16 AM, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Replace internal subbuf_start in blktrace with the default policy
> in relayfs.
> 
> Remove dropped field from struct blktrace. Correspondingly, call the
> common helper in relay. Through incrementing full_count to keep track
> of how many times we encountered a full buffer issue, it aids the user
> space app in telling how many lost events appear.

Forgot, I'd rewrite this, "telling how many lost events appear" doesn't
make any sense at all, as these events obviously don't appears in the
first place.

By incrementing full_count to keep track of how many times we
encountered a full buffer issue, user space will know how many events
were lost.

-- 
Jens Axboe

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

On Fri, May 16, 2025 at 2:19 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 5/15/25 12:16 AM, Jason Xing 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 can pass certain flag to fetch what field/statistics they expect
> > to know. Each time it only returns one result. So do not pass multiple
> > flags.
> >
> > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > v2
> > 1. refactor relay_dump() and make it only return a pure size_t result
> > of the value that users specifies.
> > 2. revise the commit log.
> > ---
> >  include/linux/relay.h |  7 +++++++
> >  kernel/relay.c        | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index ce7a1b396872..3fb285716e34 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -31,6 +31,12 @@
> >  /*
> >   * Relay buffer statistics dump
> >   */
> > +enum {
> > +     RELAY_DUMP_BUF_FULL = (1 << 0),
> > +
> > +     RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> > +};
> > +
> >  struct rchan_buf_stats
> >  {
> >       unsigned int full_count;        /* counter for buffer full */
> > @@ -167,6 +173,7 @@ struct rchan *relay_open(const char *base_filename,
> >                        void *private_data);
> >  extern void relay_close(struct rchan *chan);
> >  extern void relay_flush(struct rchan *chan);
> > +extern size_t relay_dump(struct rchan *chan, 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 eb3f630f3896..f47fc750e559 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -701,6 +701,37 @@ void relay_flush(struct rchan *chan)
> >  }
> >  EXPORT_SYMBOL_GPL(relay_flush);
> >
> > +/**
> > + *   relay_dump - dump channel buffer statistics
> > + *   @chan: the channel
> > + *   @flags: select particular information to dump
> > + *
> > + *   Returns the count of certain field that caller specifies.
> > + */
> > +size_t relay_dump(struct rchan *chan, int flags)
> > +{
> > +     unsigned int i, count = 0;
> > +     struct rchan_buf *rbuf;
> > +
> > +     if (!chan || flags > RELAY_DUMP_LAST)
> > +             return 0;
> > +
> > +     if (chan->is_global) {
> > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > +             if (flags & RELAY_DUMP_BUF_FULL)
> > +                     count = rbuf->stats.full_count;
> > +     } else {
> > +             for_each_online_cpu(i) {
> > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > +                             if (flags & RELAY_DUMP_BUF_FULL)
> > +                                     count += rbuf->stats.full_count;
>
> Kernel tends to avoid the rolled-into-one assignment and check, it's
> easy to misread. This:
>
>         rbuf = *per_cpu_ptr(chan->buf, i);
>         if (rbuf && flags & RELAY_DUMP_BUF_FULL)
>                 count += rbuf->stats.full_count;
>
> reads much easier. IMHO.

Agreed. Thanks. I will rewrite this part in V3 :)

Thanks,
Jason

>
> --
> Jens Axboe

Re: [PATCH v2 3/4] blktrace: use rbuf->stats.full as a drop indicator in relayfs

[Jason Xing]

On Fri, May 16, 2025 at 2:21 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 5/15/25 12:16 AM, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Replace internal subbuf_start in blktrace with the default policy
> > in relayfs.
> >
> > Remove dropped field from struct blktrace. Correspondingly, call the
> > common helper in relay. Through incrementing full_count to keep track
> > of how many times we encountered a full buffer issue, it aids the user
> > space app in telling how many lost events appear.
>
> Forgot, I'd rewrite this, "telling how many lost events appear" doesn't
> make any sense at all, as these events obviously don't appears in the
> first place.
>
> By incrementing full_count to keep track of how many times we
> encountered a full buffer issue, user space will know how many events
> were lost.

Oh right, I will adjust it!

Thanks,
Jason

>
> --
> Jens Axboe

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Masami Hiramatsu]

On Thu, 15 May 2025 14:16:41 +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 can pass certain flag to fetch what field/statistics they expect
> to know. Each time it only returns one result. So do not pass multiple
> flags.
> 
> Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> v2
> 1. refactor relay_dump() and make it only return a pure size_t result
> of the value that users specifies.
> 2. revise the commit log.
> ---
>  include/linux/relay.h |  7 +++++++
>  kernel/relay.c        | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index ce7a1b396872..3fb285716e34 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -31,6 +31,12 @@
>  /*
>   * Relay buffer statistics dump
>   */
> +enum {
> +	RELAY_DUMP_BUF_FULL = (1 << 0),
> +
> +	RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> +};
> +
>  struct rchan_buf_stats
>  {
>  	unsigned int full_count;	/* counter for buffer full */
> @@ -167,6 +173,7 @@ struct rchan *relay_open(const char *base_filename,
>  			 void *private_data);
>  extern void relay_close(struct rchan *chan);
>  extern void relay_flush(struct rchan *chan);
> +extern size_t relay_dump(struct rchan *chan, 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 eb3f630f3896..f47fc750e559 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -701,6 +701,37 @@ void relay_flush(struct rchan *chan)
>  }
>  EXPORT_SYMBOL_GPL(relay_flush);
>  
> +/**
> + *	relay_dump - dump channel buffer statistics

nit: relay_dump() can mislead to dump relay channel contents.
Can you rename it to relay_stats() or relay_get_stats()?

Thanks,

> + *	@chan: the channel
> + *	@flags: select particular information to dump
> + *
> + *	Returns the count of certain field that caller specifies.
> + */
> +size_t relay_dump(struct rchan *chan, int flags)
> +{
> +	unsigned int i, count = 0;
> +	struct rchan_buf *rbuf;
> +
> +	if (!chan || flags > RELAY_DUMP_LAST)
> +		return 0;
> +
> +	if (chan->is_global) {
> +		rbuf = *per_cpu_ptr(chan->buf, 0);
> +		if (flags & RELAY_DUMP_BUF_FULL)
> +			count = rbuf->stats.full_count;
> +	} else {
> +		for_each_online_cpu(i) {
> +			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> +				if (flags & RELAY_DUMP_BUF_FULL)
> +					count += rbuf->stats.full_count;
> +		}
> +	}
> +
> +	return count;
> +}
> +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>

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

On Fri, May 16, 2025 at 10:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Thu, 15 May 2025 14:16:41 +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 can pass certain flag to fetch what field/statistics they expect
> > to know. Each time it only returns one result. So do not pass multiple
> > flags.
> >
> > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > v2
> > 1. refactor relay_dump() and make it only return a pure size_t result
> > of the value that users specifies.
> > 2. revise the commit log.
> > ---
> >  include/linux/relay.h |  7 +++++++
> >  kernel/relay.c        | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index ce7a1b396872..3fb285716e34 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -31,6 +31,12 @@
> >  /*
> >   * Relay buffer statistics dump
> >   */
> > +enum {
> > +     RELAY_DUMP_BUF_FULL = (1 << 0),
> > +
> > +     RELAY_DUMP_LAST = RELAY_DUMP_BUF_FULL,
> > +};
> > +
> >  struct rchan_buf_stats
> >  {
> >       unsigned int full_count;        /* counter for buffer full */
> > @@ -167,6 +173,7 @@ struct rchan *relay_open(const char *base_filename,
> >                        void *private_data);
> >  extern void relay_close(struct rchan *chan);
> >  extern void relay_flush(struct rchan *chan);
> > +extern size_t relay_dump(struct rchan *chan, 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 eb3f630f3896..f47fc750e559 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -701,6 +701,37 @@ void relay_flush(struct rchan *chan)
> >  }
> >  EXPORT_SYMBOL_GPL(relay_flush);
> >
> > +/**
> > + *   relay_dump - dump channel buffer statistics
>
> nit: relay_dump() can mislead to dump relay channel contents.
> Can you rename it to relay_stats() or relay_get_stats()?

Sure, both are good names. I would choose relay_stats().

Thanks,
Jason

>
> Thanks,
>
> > + *   @chan: the channel
> > + *   @flags: select particular information to dump
> > + *
> > + *   Returns the count of certain field that caller specifies.
> > + */
> > +size_t relay_dump(struct rchan *chan, int flags)
> > +{
> > +     unsigned int i, count = 0;
> > +     struct rchan_buf *rbuf;
> > +
> > +     if (!chan || flags > RELAY_DUMP_LAST)
> > +             return 0;
> > +
> > +     if (chan->is_global) {
> > +             rbuf = *per_cpu_ptr(chan->buf, 0);
> > +             if (flags & RELAY_DUMP_BUF_FULL)
> > +                     count = rbuf->stats.full_count;
> > +     } else {
> > +             for_each_online_cpu(i) {
> > +                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
> > +                             if (flags & RELAY_DUMP_BUF_FULL)
> > +                                     count += rbuf->stats.full_count;
> > +             }
> > +     }
> > +
> > +     return count;
> > +}
> > +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>

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Christoph Hellwig]

On Thu, May 15, 2025 at 02:16:41PM +0800, Jason Xing wrote:
> +extern size_t relay_dump(struct rchan *chan, int flags);

Please don't add pointless externs for function prototypes.

> +EXPORT_SYMBOL_GPL(relay_dump);

This export seems unused even with the entire series applied.

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

On Fri, May 16, 2025 at 12:37 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, May 15, 2025 at 02:16:41PM +0800, Jason Xing wrote:
> > +extern size_t relay_dump(struct rchan *chan, int flags);
>
> Please don't add pointless externs for function prototypes.

Do you mean make it inline in include/linux/relay.h like how
relay_write() works?

Will do that.

>
> > +EXPORT_SYMBOL_GPL(relay_dump);
>
> This export seems unused even with the entire series applied.

My initial thought was to provide a symbol for some kernel modules to use.

Thanks,
Jason

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Christoph Hellwig]

On Fri, May 16, 2025 at 01:02:59PM +0800, Jason Xing wrote:
> Do you mean make it inline in include/linux/relay.h like how
> relay_write() works?
> 
> Will do that.

Just drop the extern, which is not needed:

size_t relay_dump(struct rchan *chan, int flags);

> >
> > This export seems unused even with the entire series applied.
> 
> My initial thought was to provide a symbol for some kernel modules to use.

The only user is blktrace, which can't be modular.  Or you plan to
submit a modular user soon, like in the next merge window/  If so
note that and add a pointer to it in the cover letter.

Re: [PATCH v2 2/4] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

On Fri, May 16, 2025 at 1:06 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, May 16, 2025 at 01:02:59PM +0800, Jason Xing wrote:
> > Do you mean make it inline in include/linux/relay.h like how
> > relay_write() works?
> >
> > Will do that.
>
> Just drop the extern, which is not needed:
>
> size_t relay_dump(struct rchan *chan, int flags);

Got it.

>
> > >
> > > This export seems unused even with the entire series applied.
> >
> > My initial thought was to provide a symbol for some kernel modules to use.
>
> The only user is blktrace, which can't be modular.  Or you plan to
> submit a modular user soon, like in the next merge window/  If so
> note that and add a pointer to it in the cover letter.

I'm still thinking about how to upstream my internal module... So in
this series, I will remove it.

Thanks,
Jason