On Tue, May 13, 2025 at 9:22 PM Masami Hiramatsu <[email protected]> wrote: > > On Tue, 13 May 2025 18:32:29 +0800 > Jason Xing <[email protected]> wrote: > > > Hi Masami, > > > > On Tue, May 13, 2025 at 5:58 PM Masami Hiramatsu <[email protected]> > > wrote: > > > > > > Hi Jason, > > > > > > On Mon, 12 May 2025 10:49:32 +0800 > > > Jason Xing <[email protected]> wrote: > > > > > > > From: Jason Xing <[email protected]> > > > > > > > > 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 <[email protected]> > > > > Signed-off-by: Jason Xing <[email protected]> > > > > --- > > > > 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/[email protected]/ > > 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) <[email protected]> > > > -- > Masami Hiramatsu (Google) <[email protected]>
