[PATCH v3 0/4] relayfs: misc changes

[PATCH v3 0/4] relayfs: misc changes

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

The series mostly focuses on the error counters which helps every user
debug their own kernel module.

---
Note: this series is made on top of this cleanup[1] and unmerged commit[2]
[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/

Jason Xing (4):
  relayfs: support a counter tracking if per-cpu buffers is full
  relayfs: introduce getting 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          | 52 +++++++++++++++++++++++++++++++++++------
 kernel/trace/blktrace.c | 22 ++---------------
 3 files changed, 65 insertions(+), 28 deletions(-)

-- 
2.43.5

[PATCH v3 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>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v3
1. add Jens reviewed-by tag

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..cd77eb285a48 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -28,6 +28,14 @@
  */
 #define RELAYFS_CHANNEL_VERSION		7
 
+/*
+ * Relay buffer statistics
+ */
+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 v3 2/4] relayfs: introduce getting relayfs statistics function

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

In this version, only support getting 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>
---
v3
1. rename and adjust relay_stats()
2. move rbuf init out of if statement

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        | 30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index cd77eb285a48..5310967f9d74 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -31,6 +31,12 @@
 /*
  * Relay buffer statistics
  */
+enum {
+	RELAY_STATS_BUF_FULL = (1 << 0),
+
+	RELAY_STATS_LAST = RELAY_STATS_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);
+size_t relay_stats(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..fd70d0e03216 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -701,6 +701,36 @@ void relay_flush(struct rchan *chan)
 }
 EXPORT_SYMBOL_GPL(relay_flush);
 
+/**
+ *	relay_stats - get channel buffer statistics
+ *	@chan: the channel
+ *	@flags: select particular information to get
+ *
+ *	Returns the count of certain field that caller specifies.
+ */
+size_t relay_stats(struct rchan *chan, int flags)
+{
+	unsigned int i, count = 0;
+	struct rchan_buf *rbuf;
+
+	if (!chan || flags > RELAY_STATS_LAST)
+		return 0;
+
+	if (chan->is_global) {
+		rbuf = *per_cpu_ptr(chan->buf, 0);
+		if (flags & RELAY_STATS_BUF_FULL)
+			count = rbuf->stats.full_count;
+	} else {
+		for_each_online_cpu(i) {
+			rbuf = *per_cpu_ptr(chan->buf, i);
+			if (rbuf && flags & RELAY_STATS_BUF_FULL)
+				count += rbuf->stats.full_count;
+		}
+	}
+
+	return count;
+}
+
 /**
  *	relay_file_open - open file op for relay files
  *	@inode: the inode
-- 
2.43.5

[PATCH v3 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. 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.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v3
1. add Jens tag
2. refine the commit log

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..545e6063f451 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_stats(bt->rchan, RELAY_STATS_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 v3 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_stats() 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        | 18 ++++++++++--------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index 5310967f9d74..6772a7075840 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -33,13 +33,15 @@
  */
 enum {
 	RELAY_STATS_BUF_FULL = (1 << 0),
+	RELAY_STATS_WRT_BIG = (1 << 1),
 
-	RELAY_STATS_LAST = RELAY_STATS_BUF_FULL,
+	RELAY_STATS_LAST = RELAY_STATS_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 fd70d0e03216..bcc889dc0970 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,17 @@ size_t relay_stats(struct rchan *chan, int flags)
 		rbuf = *per_cpu_ptr(chan->buf, 0);
 		if (flags & RELAY_STATS_BUF_FULL)
 			count = rbuf->stats.full_count;
+		else if (flags & RELAY_STATS_WRT_BIG)
+			count = rbuf->stats.big_count;
 	} else {
 		for_each_online_cpu(i) {
 			rbuf = *per_cpu_ptr(chan->buf, i);
-			if (rbuf && flags & RELAY_STATS_BUF_FULL)
-				count += rbuf->stats.full_count;
+			if (rbuf) {
+				if (flags & RELAY_STATS_BUF_FULL)
+					count += rbuf->stats.full_count;
+				else if (flags & RELAY_STATS_WRT_BIG)
+					count += rbuf->stats.big_count;
+			}
 		}
 	}
 
-- 
2.43.5

Re: [PATCH v3 0/4] relayfs: misc changes

[Jason Xing]

On Sun, May 18, 2025 at 10:57 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> From: Jason Xing <kernelxing@tencent.com>
>
> The series mostly focuses on the error counters which helps every user
> debug their own kernel module.
>
> ---
> Note: this series is made on top of this cleanup[1] and unmerged commit[2]
> [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/
>
> Jason Xing (4):
>   relayfs: support a counter tracking if per-cpu buffers is full
>   relayfs: introduce getting 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          | 52 +++++++++++++++++++++++++++++++++++------
>  kernel/trace/blktrace.c | 22 ++---------------
>  3 files changed, 65 insertions(+), 28 deletions(-)

Hi Andrew,

Any comments on the series?

Thanks,
Jason

>
> --
> 2.43.5
>

Re: [PATCH v3 0/4] relayfs: misc changes

[Andrew Morton]

On Fri, 23 May 2025 07:16:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:

> On Sun, May 18, 2025 at 10:57 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The series mostly focuses on the error counters which helps every user
> > debug their own kernel module.
> >
> > ---
> > Note: this series is made on top of this cleanup[1] and unmerged commit[2]
> > [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/
> >
> > Jason Xing (4):
> >   relayfs: support a counter tracking if per-cpu buffers is full
> >   relayfs: introduce getting 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          | 52 +++++++++++++++++++++++++++++++++++------
> >  kernel/trace/blktrace.c | 22 ++---------------
> >  3 files changed, 65 insertions(+), 28 deletions(-)
> 
> Hi Andrew,
> 
> Any comments on the series?
> 

We're at -rc7 now, so I'll park this for consideration for -rc1.

Re: [PATCH v3 0/4] relayfs: misc changes

[Jason Xing]

On Fri, May 23, 2025 at 7:34 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 23 May 2025 07:16:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > On Sun, May 18, 2025 at 10:57 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > The series mostly focuses on the error counters which helps every user
> > > debug their own kernel module.
> > >
> > > ---
> > > Note: this series is made on top of this cleanup[1] and unmerged commit[2]
> > > [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/
> > >
> > > Jason Xing (4):
> > >   relayfs: support a counter tracking if per-cpu buffers is full
> > >   relayfs: introduce getting 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          | 52 +++++++++++++++++++++++++++++++++++------
> > >  kernel/trace/blktrace.c | 22 ++---------------
> > >  3 files changed, 65 insertions(+), 28 deletions(-)
> >
> > Hi Andrew,
> >
> > Any comments on the series?
> >
>
> We're at -rc7 now, so I'll park this for consideration for -rc1.

Okay. IIUC, it seems probably two or three weeks later. I think I'll
keep track of the series and remind you at that time just in case you
might forget it :)

Thanks,
Jason

Re: [PATCH v3 0/4] relayfs: misc changes

[Masami Hiramatsu]

On Sun, 18 May 2025 10:57:30 +0800
Jason Xing <kerneljasonxing@gmail.com> wrote:

> From: Jason Xing <kernelxing@tencent.com>
> 
> The series mostly focuses on the error counters which helps every user
> debug their own kernel module.
> 

The series looks good to me.

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

Thanks,

> ---
> Note: this series is made on top of this cleanup[1] and unmerged commit[2]
> [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/
> 
> Jason Xing (4):
>   relayfs: support a counter tracking if per-cpu buffers is full
>   relayfs: introduce getting 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          | 52 +++++++++++++++++++++++++++++++++++------
>  kernel/trace/blktrace.c | 22 ++---------------
>  3 files changed, 65 insertions(+), 28 deletions(-)
> 
> -- 
> 2.43.5
> 


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

Re: [PATCH v3 0/4] relayfs: misc changes

[Jason Xing]

On Fri, May 23, 2025 at 10:39 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Sun, 18 May 2025 10:57:30 +0800
> Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The series mostly focuses on the error counters which helps every user
> > debug their own kernel module.
> >
>
> The series looks good to me.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks for the review.

Thanks,
Jason