[PATCH v1 0/5] relayfs: misc changes

[PATCH v1 0/5] 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 unmerged commit[1]
[1]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/

Jason Xing (5):
  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
  relayfs: uniformally use possible cpu iteration

 include/linux/relay.h   | 22 ++++++++++++++-
 kernel/relay.c          | 62 +++++++++++++++++++++++++++++++++++------
 kernel/trace/blktrace.c | 23 ++-------------
 3 files changed, 76 insertions(+), 31 deletions(-)

-- 
2.43.5

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Add 'full' field in per-cpu buffer structure to detect if the buffer is
full, which means: 1) relayfs doesn't intend to accept new data in
non-overwrite mode that is also by default, or 2) relayfs is going to
start over again and overwrite old unread data when kernel module has
its own subbuf_start callback to support overwrite mode. This counter
works for both overwrite and non-overwrite modes.

This counter doesn't have any explicit lock to protect from being
modified by different threads at the same time for the better
performance consideration. In terms of the atomic operation, it's not
introduced for incrementing the counter like blktrace because side
effect may arise when multiple threads access the counter simultaneously
on the machine equipped with many cpus, say, more than 200. As we can
see in relay_write() and __relay_write(), the writer at the beginning
should consider how to use the lock for the whole write process, thus
it's not necessary to add another lock to make sure the counter is
accurate.

Using 'pahole --hex -C rchan_buf vmlinux' so you can see this field just
fits into one 4-byte hole in the cacheline 2.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 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 f80b0eb1e905..022cf11e5a92 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -28,6 +28,14 @@
  */
 #define RELAYFS_CHANNEL_VERSION		7
 
+/*
+ * Relay buffer error statistics dump
+ */
+struct rchan_buf_error_stats
+{
+	unsigned int full;		/* 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_error_stats stats; /* error 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 5aeb9226e238..b5db4aa60da1 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 buf_full = relay_buf_full(buf);
+
+	if (buf_full)
+		buf->stats.full++;
+
 	if (!buf->chan->cb->subbuf_start)
-		return !relay_buf_full(buf);
+		return !buf_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 = 0;
 
 	for (i = 0; i < buf->chan->n_subbufs; i++)
 		buf->padding[i] = 0;
-- 
2.43.5

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

[Andrew Morton]

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

> From: Jason Xing <kernelxing@tencent.com>
> 
> Add 'full' field in per-cpu buffer structure to detect if the buffer is
> full, which means: 1) relayfs doesn't intend to accept new data in
> non-overwrite mode that is also by default, or 2) relayfs is going to
> start over again and overwrite old unread data when kernel module has
> its own subbuf_start callback to support overwrite mode. This counter
> works for both overwrite and non-overwrite modes.
> 
> This counter doesn't have any explicit lock to protect from being
> modified by different threads at the same time for the better
> performance consideration. In terms of the atomic operation, it's not
> introduced for incrementing the counter like blktrace because side
> effect may arise when multiple threads access the counter simultaneously
> on the machine equipped with many cpus, say, more than 200. As we can
> see in relay_write() and __relay_write(), the writer at the beginning
> should consider how to use the lock for the whole write process, thus
> it's not necessary to add another lock to make sure the counter is
> accurate.
> 
> Using 'pahole --hex -C rchan_buf vmlinux' so you can see this field just
> fits into one 4-byte hole in the cacheline 2.

Does this alter blktrace output?  If so is that backward-compatible
(and do we care).  Is there any blktrace documentation which should be
updated?

Also, please check Documentation/filesystems/relay.rst and see if any
updates should be made to reflect the changes in this patchset.

I'm not really clear on the use cases of this counter - perhaps you can
be more verbose about this in the changelog.

> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index f80b0eb1e905..022cf11e5a92 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -28,6 +28,14 @@
>   */
>  #define RELAYFS_CHANNEL_VERSION		7
>  
> +/*
> + * Relay buffer error statistics dump
> + */
> +struct rchan_buf_error_stats
> +{
> +	unsigned int full;		/* counter for buffer full */
> +};

Why a struct?

>  /*
>   * 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_error_stats stats; /* error stats */

Could simply use

	unsigned int full;

here?

Also, the name "full" implies to me "it is full".  Perhaps "full_count"
would be better.

>  	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 5aeb9226e238..b5db4aa60da1 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 buf_full = relay_buf_full(buf);
> +
> +	if (buf_full)
> +		buf->stats.full++;

I don't understand the changelog's description of this, sorry.

Is it saying "this operation is protected by a lock"?  If so, please
specifically state which lock this is.

Or is it saying "we don't care if this races because the counter will
be close enough".  If so then maybe so, but things like KCSAN will
probably detect and warn and then people will want to address this.

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

[Jason Xing]

On Tue, May 13, 2025 at 8:51 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 12 May 2025 10:49:31 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Add 'full' field in per-cpu buffer structure to detect if the buffer is
> > full, which means: 1) relayfs doesn't intend to accept new data in
> > non-overwrite mode that is also by default, or 2) relayfs is going to
> > start over again and overwrite old unread data when kernel module has
> > its own subbuf_start callback to support overwrite mode. This counter
> > works for both overwrite and non-overwrite modes.
> >
> > This counter doesn't have any explicit lock to protect from being
> > modified by different threads at the same time for the better
> > performance consideration. In terms of the atomic operation, it's not
> > introduced for incrementing the counter like blktrace because side
> > effect may arise when multiple threads access the counter simultaneously
> > on the machine equipped with many cpus, say, more than 200. As we can
> > see in relay_write() and __relay_write(), the writer at the beginning
> > should consider how to use the lock for the whole write process, thus
> > it's not necessary to add another lock to make sure the counter is
> > accurate.
> >
> > Using 'pahole --hex -C rchan_buf vmlinux' so you can see this field just
> > fits into one 4-byte hole in the cacheline 2.
>
> Does this alter blktrace output?  If so is that backward-compatible
> (and do we care).  Is there any blktrace documentation which should be
> updated?

Thanks for the review.

Surely no, I tested blktrace by running 'blktrace -d /dev/vda1' to
verify the dropped field after the entire series applied. So it's
compatible.

>
> Also, please check Documentation/filesystems/relay.rst and see if any
> updates should be made to reflect the changes in this patchset.

Right, will do it accordingly.

>
> I'm not really clear on the use cases of this counter - perhaps you can
> be more verbose about this in the changelog.

Will add more.

The existing code 'blk_dropped_read' in blktrace might give you a
hint. In real production, we sometimes encounter the case where the
reader cannot consume the data as fast as possible, which leads to
data loss.

>
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index f80b0eb1e905..022cf11e5a92 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -28,6 +28,14 @@
> >   */
> >  #define RELAYFS_CHANNEL_VERSION              7
> >
> > +/*
> > + * Relay buffer error statistics dump
> > + */
> > +struct rchan_buf_error_stats
> > +{
> > +     unsigned int full;              /* counter for buffer full */
> > +};
>
> Why a struct?

It is because I'm going to add more counters related to the error
information, like patch [4/5]. Does it make any sense?

>
> >  /*
> >   * 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_error_stats stats; /* error stats */
>
> Could simply use
>
>         unsigned int full;
>
> here?
>
> Also, the name "full" implies to me "it is full".  Perhaps "full_count"
> would be better.

Got it. Makes sense to me.

>
> >       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 5aeb9226e238..b5db4aa60da1 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 buf_full = relay_buf_full(buf);
> > +
> > +     if (buf_full)
> > +             buf->stats.full++;
>
> I don't understand the changelog's description of this, sorry.
>
> Is it saying "this operation is protected by a lock"?  If so, please
> specifically state which lock this is.
>
> Or is it saying "we don't care if this races because the counter will
> be close enough".  If so then maybe so, but things like KCSAN will
> probably detect and warn and then people will want to address this.
>

Sorry for the confusion. I meant the whole write process should
perform with the lock protection which users are supposed to pay more
attention to. User calls __relay_write which means he already
considers the racy case, adding additional lock before writing. On the
assumption that the whole write process is protected, there is no need
to add any form of lock internally (even like atomic operations) to
ensure the consistency of full_count.

I will revise it in the next re-spin.

Thanks,
Jason

[PATCH v1 2/5] 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 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

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Andrew Morton]

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

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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
>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Andrew Morton]

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?

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Andrew Morton]

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].

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Masami Hiramatsu]

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>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Masami Hiramatsu]

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>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Masami Hiramatsu]

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>

Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

[Jason Xing]

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>

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

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Replace internal subbuf_start with the default policy in relayfs.

Remove dropped field from struct blk_trace. Correspondingly, use per-cpu
buffer mechanism to replace atomic operation. Through incrementing
full counter to keep track of how many times we encountered a full
subbuffer, it aids the user space app in telling how many lost events
there were.

Sum up all the fields gathered from all cpus when application is reading.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 kernel/trace/blktrace.c | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index d1a89714e805..09d42c40ac9f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -415,9 +415,9 @@ 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];
+	char buf[RELAY_DUMP_BUF_MAX_LEN];
 
-	snprintf(buf, sizeof(buf), "%u\n", atomic_read(&bt->dropped));
+	relay_dump(bt->rchan, buf, RELAY_DUMP_BUF_MAX_LEN, RELAY_DUMP_BUF_FULL);
 
 	return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
 }
@@ -456,23 +456,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 +474,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 +562,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 v1 4/5] relayfs: support a counter tracking if data is too big to write

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

It doesn't really matter to let the user/admin know what the last too
big value is. Just record how many times this case is triggered.

Also solve the 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 previous racy condition because
in terms of the global structure, it is likely to happen when a few of
per-cpu buffers encounter the too big data case.

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        | 19 +++++++++++--------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/linux/relay.h b/include/linux/relay.h
index 7a442c4cbead..0f5f6ff17824 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -33,8 +33,9 @@
  */
 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,
 	RELAY_DUMP_MASK = (RELAY_DUMP_LAST - 1) | RELAY_DUMP_LAST
 };
 
@@ -43,6 +44,7 @@ enum {
 struct rchan_buf_error_stats
 {
 	unsigned int full;		/* counter for buffer full */
+	unsigned int big;		/* counter for too big to write */
 };
 
 /*
@@ -82,7 +84,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 0e675a77285c..27f7e701724f 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 = 0;
+	buf->stats.big = 0;
 
 	for (i = 0; i < buf->chan->n_subbufs; i++)
 		buf->padding[i] = 0;
@@ -712,7 +713,7 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
 	return length;
 
 toobig:
-	buf->chan->last_toobig = length;
+	buf->stats.big++;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(relay_switch_subbuf);
@@ -772,11 +773,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);
@@ -819,7 +815,7 @@ EXPORT_SYMBOL_GPL(relay_flush);
  */
 void relay_dump(struct rchan *chan, char *buf, int len, int flags)
 {
-	unsigned int i, full_counter = 0;
+	unsigned int i, full_counter = 0, big_counter = 0;
 	struct rchan_buf *rbuf;
 	int offset = 0;
 
@@ -832,15 +828,22 @@ void relay_dump(struct rchan *chan, char *buf, int len, int flags)
 	if (chan->is_global) {
 		rbuf = *per_cpu_ptr(chan->buf, 0);
 		full_counter = rbuf->stats.full;
+		big_counter = rbuf->stats.big;
 	} else {
 		for_each_possible_cpu(i) {
-			if ((rbuf = *per_cpu_ptr(chan->buf, i)))
+			if ((rbuf = *per_cpu_ptr(chan->buf, i))) {
 				full_counter += rbuf->stats.full;
+				big_counter += rbuf->stats.big;
+			}
+		}
 	}
 
 	if (flags & RELAY_DUMP_BUF_FULL)
 		offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);
 
+	if (flags & RELAY_DUMP_WRT_BIG)
+		offset += snprintf(buf, sizeof(unsigned int), "%u", big_counter);
+
 	snprintf(buf + offset, 1, "\n");
 }
 EXPORT_SYMBOL_GPL(relay_dump);
-- 
2.43.5

[PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
hotplug cpu which doesn't have its own file.

Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 kernel/relay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/relay.c b/kernel/relay.c
index 27f7e701724f..dcb099859e83 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
 	kref_init(&chan->kref);
 
 	mutex_lock(&relay_channels_mutex);
-	for_each_online_cpu(i) {
+	for_each_possible_cpu(i) {
 		buf = relay_open_buf(chan, i);
 		if (!buf)
 			goto free_bufs;
@@ -615,7 +615,7 @@ int relay_late_setup_files(struct rchan *chan,
 	 * no files associated. So it's safe to call relay_setup_buf_file()
 	 * on all currently online CPUs.
 	 */
-	for_each_online_cpu(i) {
+	for_each_possible_cpu(i) {
 		buf = *per_cpu_ptr(chan->buf, i);
 		if (unlikely(!buf)) {
 			WARN_ONCE(1, KERN_ERR "CPU has no buffer!\n");
-- 
2.43.5

Re: [PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Andrew Morton]

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

> From: Jason Xing <kernelxing@tencent.com>
> 
> Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
> hotplug cpu which doesn't have its own file.

I don't understand this.  Exactly what problem are we trying to solve?

> Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  kernel/relay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 27f7e701724f..dcb099859e83 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
>  	kref_init(&chan->kref);
>  
>  	mutex_lock(&relay_channels_mutex);
> -	for_each_online_cpu(i) {
> +	for_each_possible_cpu(i) {

num_possible_cpus() can sometimes greatly exceed num_online_cpus(), so
this is an unfortunate change.  It would be better to implement the
hotplug notifier?

>  		buf = relay_open_buf(chan, i);
>  		if (!buf)
>  			goto free_bufs;
> @@ -615,7 +615,7 @@ int relay_late_setup_files(struct rchan *chan,
>  	 * no files associated. So it's safe to call relay_setup_buf_file()
>  	 * on all currently online CPUs.
>  	 */
> -	for_each_online_cpu(i) {
> +	for_each_possible_cpu(i) {
>  		buf = *per_cpu_ptr(chan->buf, i);
>  		if (unlikely(!buf)) {
>  			WARN_ONCE(1, KERN_ERR "CPU has no buffer!\n");
> -- 
> 2.43.5

Re: [PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Jason Xing]

On Tue, May 13, 2025 at 8:52 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 12 May 2025 10:49:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
> > hotplug cpu which doesn't have its own file.
>
> I don't understand this.  Exactly what problem are we trying to solve?

The reason behind this change is can we directly allocate per possible
cpu at the initialization phase. After this, even if some cpu goes
online, we don't need to care about it.

The idea is directly borrowed from the networking area where people
use possible cpu iteration for most cases.

>
> > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  kernel/relay.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index 27f7e701724f..dcb099859e83 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
> >       kref_init(&chan->kref);
> >
> >       mutex_lock(&relay_channels_mutex);
> > -     for_each_online_cpu(i) {
> > +     for_each_possible_cpu(i) {
>
> num_possible_cpus() can sometimes greatly exceed num_online_cpus(), so
> this is an unfortunate change.

Are you worried about too much extra memory to waste in this case?

A relevant thing I would like to share here:
To keep the high performance of transferring data between kernel space
and user space, the per-cpu mechanism is needed like how relay works
at the moment. It allocates many unnecessary/big memory chunks
especially when the cpu number is very large, say, 256. I'm still
working on this to see if we can figure out a good approach to balance
the performance and memory.

> It would be better to implement the
> hotplug notifier?

Right, but sorry, I hesitate to do so because it involves much more
work and corresponding tests.

Thanks,
Jason

>
> >               buf = relay_open_buf(chan, i);
> >               if (!buf)
> >                       goto free_bufs;
> > @@ -615,7 +615,7 @@ int relay_late_setup_files(struct rchan *chan,
> >        * no files associated. So it's safe to call relay_setup_buf_file()
> >        * on all currently online CPUs.
> >        */
> > -     for_each_online_cpu(i) {
> > +     for_each_possible_cpu(i) {
> >               buf = *per_cpu_ptr(chan->buf, i);
> >               if (unlikely(!buf)) {
> >                       WARN_ONCE(1, KERN_ERR "CPU has no buffer!\n");
> > --
> > 2.43.5

Re: [PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Andrew Morton]

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

> On Tue, May 13, 2025 at 8:52 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Mon, 12 May 2025 10:49:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
> > > hotplug cpu which doesn't have its own file.
> >
> > I don't understand this.  Exactly what problem are we trying to solve?
> 
> The reason behind this change is can we directly allocate per possible
> cpu at the initialization phase. After this, even if some cpu goes
> online, we don't need to care about it.
> 
> The idea is directly borrowed from the networking area where people
> use possible cpu iteration for most cases.

I'd rephrase that as "I know this is wrong but I'm lazy so I'll do it
this way for now and I'll fix it up later but I never actually do so".

Yes, handling hotplug is a hassle and for_each_possible_cpu() saves a
couple of hours at development time, but its cost is forever.

> >
> > > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  kernel/relay.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/relay.c b/kernel/relay.c
> > > index 27f7e701724f..dcb099859e83 100644
> > > --- a/kernel/relay.c
> > > +++ b/kernel/relay.c
> > > @@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
> > >       kref_init(&chan->kref);
> > >
> > >       mutex_lock(&relay_channels_mutex);
> > > -     for_each_online_cpu(i) {
> > > +     for_each_possible_cpu(i) {
> >
> > num_possible_cpus() can sometimes greatly exceed num_online_cpus(), so
> > this is an unfortunate change.
> 
> Are you worried about too much extra memory to waste in this case?

That, and the cost of iterating over 1024 CPUs instead of 4 CPUs!

> A relevant thing I would like to share here:
> To keep the high performance of transferring data between kernel space
> and user space, the per-cpu mechanism is needed like how relay works
> at the moment. It allocates many unnecessary/big memory chunks
> especially when the cpu number is very large, say, 256. I'm still
> working on this to see if we can figure out a good approach to balance
> the performance and memory.
> 
> > It would be better to implement the
> > hotplug notifier?
> 
> Right, but sorry, I hesitate to do so because it involves much more
> work and corresponding tests.

I did this conversion a few times, a million years ago.  It's
surprisingly easy.

Re: [PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Jason Xing]

On Tue, May 13, 2025 at 11:21 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Tue, 13 May 2025 10:03:01 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > On Tue, May 13, 2025 at 8:52 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Mon, 12 May 2025 10:49:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
> > > > hotplug cpu which doesn't have its own file.
> > >
> > > I don't understand this.  Exactly what problem are we trying to solve?
> >
> > The reason behind this change is can we directly allocate per possible
> > cpu at the initialization phase. After this, even if some cpu goes
> > online, we don't need to care about it.
> >
> > The idea is directly borrowed from the networking area where people
> > use possible cpu iteration for most cases.
>
> I'd rephrase that as "I know this is wrong but I'm lazy so I'll do it
> this way for now and I'll fix it up later but I never actually do so".
>
> Yes, handling hotplug is a hassle and for_each_possible_cpu() saves a
> couple of hours at development time, but its cost is forever.
>
> > >
> > > > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > >  kernel/relay.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/relay.c b/kernel/relay.c
> > > > index 27f7e701724f..dcb099859e83 100644
> > > > --- a/kernel/relay.c
> > > > +++ b/kernel/relay.c
> > > > @@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
> > > >       kref_init(&chan->kref);
> > > >
> > > >       mutex_lock(&relay_channels_mutex);
> > > > -     for_each_online_cpu(i) {
> > > > +     for_each_possible_cpu(i) {
> > >
> > > num_possible_cpus() can sometimes greatly exceed num_online_cpus(), so
> > > this is an unfortunate change.
> >
> > Are you worried about too much extra memory to waste in this case?
>
> That, and the cost of iterating over 1024 CPUs instead of 4 CPUs!
>
> > A relevant thing I would like to share here:
> > To keep the high performance of transferring data between kernel space
> > and user space, the per-cpu mechanism is needed like how relay works
> > at the moment. It allocates many unnecessary/big memory chunks
> > especially when the cpu number is very large, say, 256. I'm still
> > working on this to see if we can figure out a good approach to balance
> > the performance and memory.
> >
> > > It would be better to implement the
> > > hotplug notifier?
> >
> > Right, but sorry, I hesitate to do so because it involves much more
> > work and corresponding tests.
>
> I did this conversion a few times, a million years ago.  It's
> surprisingly easy.

I see. Let me try this :)

Thanks,
Jason

Re: [PATCH v1 5/5] relayfs: uniformally use possible cpu iteration

[Jason Xing]

On Tue, May 13, 2025 at 8:52 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 12 May 2025 10:49:35 +0800 Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Use for_each_possible_cpu to create per-cpu relayfs file to avoid later
> > hotplug cpu which doesn't have its own file.
>
> I don't understand this.  Exactly what problem are we trying to solve?
>
> > Reviewed-by: Yushan Zhou <katrinzhou@tencent.com>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  kernel/relay.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index 27f7e701724f..dcb099859e83 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -519,7 +519,7 @@ struct rchan *relay_open(const char *base_filename,
> >       kref_init(&chan->kref);
> >
> >       mutex_lock(&relay_channels_mutex);
> > -     for_each_online_cpu(i) {
> > +     for_each_possible_cpu(i) {
>
> num_possible_cpus() can sometimes greatly exceed num_online_cpus(), so
> this is an unfortunate change.  It would be better to implement the
> hotplug notifier?

For the record, the hotplug notifier has been retired by the commit
530e9b76ae8f("cpu/hotplug: Remove obsolete cpu hotplug
register/unregister functions"). And now in relay, a similar feature
called hotplug state machine has already been implemented by the
commit e6d4989a9ad1 ("relayfs: Convert to hotplug state machine"). So
the relay has hotplug support.

Sorry for missing this point. I would drop this patch.

Thanks,
Jason

>
> >               buf = relay_open_buf(chan, i);
> >               if (!buf)
> >                       goto free_bufs;
> > @@ -615,7 +615,7 @@ int relay_late_setup_files(struct rchan *chan,
> >        * no files associated. So it's safe to call relay_setup_buf_file()
> >        * on all currently online CPUs.
> >        */
> > -     for_each_online_cpu(i) {
> > +     for_each_possible_cpu(i) {
> >               buf = *per_cpu_ptr(chan->buf, i);
> >               if (unlikely(!buf)) {
> >                       WARN_ONCE(1, KERN_ERR "CPU has no buffer!\n");
> > --
> > 2.43.5