On 6/3/21 11:22 PM, Steven Rostedt wrote:

On Fri, 14 May 2021 22:51:14 +0200
Daniel Bristot de Oliveira [off-list ref] wrote:

quoted

Provides a generic read and write implementation to save/read u64 values
from a file on tracefs. The trace_ull_config structure defines where to
read/write the value, the min and the max acceptable values, and a lock
to protect the write.

This states what the patch is doing, but does not say why it is doing it.

Yeah...

quoted

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <redacted>
Cc: Alexandre Chartre <redacted>
Cc: Clark Willaims <redacted>
Cc: John Kacur <jkacur@redhat.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Daniel Bristot de Oliveira <redacted>
---
 kernel/trace/trace.c | 87 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/trace/trace.h | 19 ++++++++++
 2 files changed, 106 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 560e4c8d3825..b4cd89010813 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c

@@ -7516,6 +7516,93 @@ static const struct file_operations snapshot_raw_fops = {
 
 #endif /* CONFIG_TRACER_SNAPSHOT */
 
+/*
+ * trace_ull_config_write - Generic write function to save u64 value

That is a horrible name. What the hell is the "config"?

quoted

+ * @filp: The active open file structure
+ * @ubuf: The userspace provided buffer to read value into
+ * @cnt: The maximum number of bytes to read
+ * @ppos: The current "file" position
+ *
+ * This function provides a generic write implementation to save u64 values
+ * from a file on tracefs. The filp->private_data must point to a
+ * trace_ull_config structure that defines where to write the value, the
+ * min and the max acceptable values, and a lock to protect the write.

This doesn't seem to be a generic way to save 64 bit values (which I still
don't understand, because unsigned long long should work too). But it looks
like the rational is for having some kind of generic way to read 64 bit
values giving them a min and a max.

I see this is used later, but this patch needs to be rewritten. It makes no
sense.

The reason for this patch is that hwlat, osnoise, and timerlat have "u64 config"
options that are read/write via tracefs "files." In the previous version, I had
multiple functions doing basically the same thing:

A write function that:
	read a u64 from user-space
	get a lock,
	check for min/max acceptable values
		save the value
	release the lock.

and a read function that:
	write the config value to the "read" buffer.

And so, I tried to come up with a way to avoid code duplication.

question: are only the names that are bad? (I agree that they are bad) or do you
think that the overall idea is bad? :-)

Suggestions?

-- Daniel

-- Steve