On Wed, Jul 2, 2025 at 12:11 AM Steven Rostedt <rost...@goodmis.org> wrote:
>
> On Thu, 12 Jun 2025 12:43:49 +0200
> Gabriele Paoloni <gpaol...@redhat.com> wrote:
>
> > This patch implements the documentation of event_enable_write and
> > event_enable_read in the form of testable function's expectations.
> >
> > Signed-off-by: Gabriele Paoloni <gpaol...@redhat.com>
> > ---
> > kernel/trace/trace_events.c | 72 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 72 insertions(+)
> >
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 5e84ef01d0c8..eb3c5e6e557d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -1771,6 +1771,46 @@ static void p_stop(struct seq_file *m, void *p)
> > mutex_unlock(&event_mutex);
> > }
> >
> > +/**
> > + * event_enable_read - read from a trace event file to retrieve its status.
> > + * @filp: file pointer associated with the target trace event;
> > + * @ubuf: user space buffer where the event status is copied to;
> > + * @cnt: number of bytes to be copied to the user space buffer;
>
> Why is the above ending with semicolons?
Sorry it should be a full stop, I'll fix it in v4.
>
> > + * @ppos: the current position in the buffer.
> > + *
> > + * This is a way for user space executables to retrieve the status of a
> > + * specific event
> > + *
> > + * Function's expectations:
> > + * - This function shall lock the global event_mutex before performing any
> > + * operation on the target event file and unlock it after all operations
> > on
> > + * the target event file have completed;
> > + * - This function shall retrieve the status flags from the file associated
> > + * with the target event;
> > + * - This function shall format the string to report the event status to
> > user
> > + * space:
> > + * - The first character of the string to be copied to user space shall
> > be
> > + * set to "1" if the enabled flag is set AND the soft_disabled flag is
> > not
> > + * set, else it shall be set to "0";
> > + * - The second character of the string to be copied to user space shall
> > be
> > + * set to "*" if either the soft_disabled flag or the soft_mode flag is
> > + * set, else it shall be set to "\n";
> > + * - The third character of the string to be copied to user space shall b
> > + * set to "\n" if either the soft_disabled flag or the soft_mode flag
> > is
> > + * set, else it shall be set to "0";
> > + * - Any other character in the string to be copied to user space shall
> > be
> > + * set to "0";
>
> The above could use some new lines. Like between each bullet. My
> eyesight isn't as good anymore and I find reading gobs of text more
> difficult. Newlines give my eyes a break.
>
> I know this is being written so that a tool could parse it, but also
> needs to be parsed by aging developers. ;-)
Agreed, I'll fix it in v4.
>
> > + * - This function shall check if the requested cnt bytes are equal or
> > greater
> > + * than the length of the string to be copied to user space (else a
> > + * corrupted event status could be reported);
> > + * - This function shall invoke simple_read_from_buffer() to perform the
> > copy
> > + * of the kernel space string to ubuf.
>
> Hmm, and it is also verbose. This is a relatively simple function, and
> it caused quite a bit of requirements. I wonder if it could be
> shortened a bit. Otherwise more complex and larger functions are going
> to be overwhelming and likely not acceptable. And those are the
> functions that I think this will benefit the most!
Mmm got it. What about
* Function's expectations:
* - This function shall lock the global event_mutex before performing any
* operation on the target event file and unlock it after all operations on
* the target event file have completed;
*
* - This function shall format the string copied to userspace according to
* the status flags retrieved from the target event file:
* - The first character shall be set to "1" if the enabled flag is
set AND the
* soft_disabled flag is not set, else it shall be set to "0";
* - The second character is optional and shall be set to "*" if either the
* soft_disabled flag or the soft_mode flag is set;
* - The string shall be terminated by a newline ("\n") and any remaining
* character shall be set to "0";
*
* - This function shall invoke simple_read_from_buffer() to perform the copy
* of the kernel space string to ubuf.
(pls note that the check on cnt has been removed in v3 that is out already)
>
> > + *
> > + * Returns the number of copied bytes on success, -ENODEV if the event file
> > + * cannot be retrieved from the input filp, -EINVAL if cnt is less than the
> > + * length of the string to be copied to ubuf, any error returned by
> > + * simple_read_from_buffer
>
> Returns should be formatted better where each return value is on a
> separate line.
Understood, I'll fix this in v4
Many Thanks!
Gab
>
> -- Steve
>
> > + */
> > static ssize_t
> > event_enable_read(struct file *filp, char __user *ubuf, size_t cnt,
> > loff_t *ppos)
> > @@ -1808,6 +1848,38 @@ event_enable_read(struct file *filp, char __user
> > *ubuf, size_t cnt,
> > return simple_read_from_buffer(ubuf, cnt, ppos, buf, strlen(buf));
> > }
> >
> > +/**
> > + * event_enable_write - write to a trace event file to enable/disable it.
> > + * @filp: file pointer associated with the target trace event;
> > + * @ubuf: user space buffer where the enable/disable value is copied from;
> > + * @cnt: number of bytes to be copied from the user space buffer;
> > + * @ppos: the current position in the buffer.
> > + *
> > + * This is a way for user space executables to enable or disable event
> > + * recording.
> > + *
> > + * Function's expectations:
> > + * - This function shall copy cnt bytes from the input ubuf buffer to a
> > kernel
> > + * space buffer (or the whole input ubuf if cnt is larger than ubuf size)
> > + * and shall convert the string within the kernel space buffer into a
> > decimal
> > + * base format number;
> > + * - This function shall lock the global event_mutex before performing any
> > + * operation on the target event file and unlock it after all operations
> > on
> > + * the target event file have completed;
> > + * - This function shall check the size of the per-cpu ring-buffers used
> > for
> > + * the event trace data and, if smaller than TRACE_BUF_SIZE_DEFAULT,
> > expand
> > + * them to TRACE_BUF_SIZE_DEFAULT bytes (sizes larger than
> > + * TRACE_BUF_SIZE_DEFAULT are not allowed);
> > + * - This function shall invoke ftrace_event_enable_disable to enable or
> > + * disable the target trace event according to the value read from user
> > space
> > + * (0 - disable, 1 - enable);
> > + *
> > + * Returns 0 on success, any error returned by kstrtoul_from_user, -ENODEV
> > if
> > + * the event file cannot be retrieved from the input filp, any error
> > returned by
> > + * tracing_update_buffers, any error returned by
> > ftrace_event_enable_disable,
> > + * -EINVAL if the value copied from the user space ubuf is different from
> > 0 or
> > + * 1.
> > + */
> > static ssize_t
> > event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
> > loff_t *ppos)
>
