Message-ID: <20250701181119.7b0bc5d6@batman.local.home>
Date: Tue, 1 Jul 2025 18:11:19 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Gabriele Paoloni <gpaoloni@...hat.com>
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 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?

> + * @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. ;-)

> + * - 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!

> + *
> + * 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.

-- 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