Message-ID: <CAL+tcoD63WBnmMeRhs77YBLEzsb4wQUYqO_U1wXcCUY4XNXptw@mail.gmail.com>
Date: Wed, 14 May 2025 10:06:39 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: axboe@...nel.dk, rostedt@...dmis.org, mathieu.desnoyers@...icios.com, 
	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org, 
	linux-block@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>, Yushan Zhou <katrinzhou@...cent.com>
Subject: Re: [PATCH v1 2/5] relayfs: introduce dump of relayfs statistics function

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

Powered by blists - more mailing lists