On Wed, May 14, 2025 at 9:29 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > On Tue, 13 May 2025 21:46:25 +0800 > Jason Xing <kerneljasonxing@xxxxxxxxx> 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@xxxxxxxxx/ > > 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@xxxxxxxxxx> > > > > > > > > > -- > > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > > -- > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
