Messages in this thread | | | Subject | Re: [PATCH V3 5/9] tracing/trace: Add a generic function to read/write u64 values from tracefs | From | Daniel Bristot de Oliveira <> | Date | Fri, 4 Jun 2021 18:05:06 +0200 |
| |
On 6/3/21 11:22 PM, Steven Rostedt wrote: > On Fri, 14 May 2021 22:51:14 +0200 > Daniel Bristot de Oliveira <bristot@redhat.com> wrote: > >> 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...
>> >> 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 <tglx@linutronix.de> >> Cc: Alexandre Chartre <alexandre.chartre@oracle.com> >> Cc: Clark Willaims <williams@redhat.com> >> 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 <bristot@redhat.com> >> --- >> 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"? > >> + * @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
| |