Message-ID: <CA+wEVJarZben=F3Dw0A8_tvAAR7-qb4PrjCj0AGFoq=DH1RJwg@mail.gmail.com>
Date: Wed, 2 Jul 2025 16:59:29 +0200
From: Gabriele Paoloni <gpaoloni@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	acarmina@...hat.com, chuck.wolber@...ing.com
Subject: Re: [RFC PATCH 2/2] tracing: add testable specifications for event_enable_write/read

On Wed, Jul 2, 2025 at 12:11 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Thu, 12 Jun 2025 12:43:49 +0200
> Gabriele Paoloni <gpaoloni@...hat.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 <gpaoloni@...hat.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)
>

Powered by blists - more mailing lists