Here's my story:
Once upon a time, there lived a developer that wanted to export an
atomic64_t value to userspace using debugfs to help with debugging. But
that developer found there was no helper function to do so and was very
sad.
The End.
Yeah, it's a sad story. Here's my patch, merge me maybe?
Anand K Mistry (1):
debugfs: Add a helper to export atomic64_t values
fs/debugfs/file.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/debugfs.h | 6 ++++++
2 files changed, 43 insertions(+)
--
2.29.1.341.ge80a0c044ae-goog
This mirrors support for exporting atomic_t values.
Signed-off-by: Anand K Mistry <[email protected]>
---
fs/debugfs/file.c | 37 +++++++++++++++++++++++++++++++++++++
include/linux/debugfs.h | 6 ++++++
2 files changed, 43 insertions(+)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index a768a09430c3..798bd3bdedec 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -770,6 +770,43 @@ void debugfs_create_atomic_t(const char *name, umode_t mode,
}
EXPORT_SYMBOL_GPL(debugfs_create_atomic_t);
+static int debugfs_atomic64_t_set(void *data, u64 val)
+{
+ atomic64_set((atomic64_t *)data, val);
+ return 0;
+}
+static int debugfs_atomic64_t_get(void *data, u64 *val)
+{
+ *val = atomic64_read((atomic64_t *)data);
+ return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t, debugfs_atomic64_t_get,
+ debugfs_atomic64_t_set, "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL,
+ "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_wo, NULL, debugfs_atomic64_t_set,
+ "%lld\n");
+
+/**
+ * debugfs_create_atomic64_t - create a debugfs file that is used to read and
+ * write an atomic64_t value
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file. This should be a
+ * directory dentry if set. If this parameter is %NULL, then the
+ * file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read to and write
+ * from.
+ */
+void debugfs_create_atomic64_t(const char *name, umode_t mode,
+ struct dentry *parent, atomic64_t *value)
+{
+ debugfs_create_mode_unsafe(name, mode, parent, value,
+ &fops_atomic64_t, &fops_atomic64_t_ro,
+ &fops_atomic64_t_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_atomic64_t);
+
ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
size_t count, loff_t *ppos)
{
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 851dd1f9a8a5..0fac84c53eab 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -126,6 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
struct dentry *parent, size_t *value);
void debugfs_create_atomic_t(const char *name, umode_t mode,
struct dentry *parent, atomic_t *value);
+void debugfs_create_atomic64_t(const char *name, umode_t mode,
+ struct dentry *parent, atomic64_t *value);
struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent, bool *value);
@@ -291,6 +293,10 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
atomic_t *value)
{ }
+static inline void debugfs_create_atomic64_t(const char *name, umode_t mode,
+ struct dentry *parent, atomic64_t *value)
+{ }
+
static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
struct dentry *parent,
bool *value)
--
2.29.1.341.ge80a0c044ae-goog
On Fri, Oct 30, 2020 at 06:04:42PM +1100, Anand K Mistry wrote:
> This mirrors support for exporting atomic_t values.
>
> Signed-off-by: Anand K Mistry <[email protected]>
>
> ---
>
> fs/debugfs/file.c | 37 +++++++++++++++++++++++++++++++++++++
> include/linux/debugfs.h | 6 ++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index a768a09430c3..798bd3bdedec 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -770,6 +770,43 @@ void debugfs_create_atomic_t(const char *name, umode_t mode,
> }
> EXPORT_SYMBOL_GPL(debugfs_create_atomic_t);
>
> +static int debugfs_atomic64_t_set(void *data, u64 val)
> +{
> + atomic64_set((atomic64_t *)data, val);
> + return 0;
> +}
> +static int debugfs_atomic64_t_get(void *data, u64 *val)
> +{
> + *val = atomic64_read((atomic64_t *)data);
> + return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t, debugfs_atomic64_t_get,
> + debugfs_atomic64_t_set, "%lld\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL,
> + "%lld\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_wo, NULL, debugfs_atomic64_t_set,
> + "%lld\n");
> +
> +/**
> + * debugfs_create_atomic64_t - create a debugfs file that is used to read and
> + * write an atomic64_t value
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have
> + * @parent: a pointer to the parent dentry for this file. This should be a
> + * directory dentry if set. If this parameter is %NULL, then the
> + * file will be created in the root of the debugfs filesystem.
> + * @value: a pointer to the variable that the file should read to and write
> + * from.
> + */
> +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> + struct dentry *parent, atomic64_t *value)
> +{
> + debugfs_create_mode_unsafe(name, mode, parent, value,
> + &fops_atomic64_t, &fops_atomic64_t_ro,
> + &fops_atomic64_t_wo);
> +}
> +EXPORT_SYMBOL_GPL(debugfs_create_atomic64_t);
> +
> ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> size_t count, loff_t *ppos)
> {
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 851dd1f9a8a5..0fac84c53eab 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -126,6 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
> struct dentry *parent, size_t *value);
> void debugfs_create_atomic_t(const char *name, umode_t mode,
> struct dentry *parent, atomic_t *value);
> +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> + struct dentry *parent, atomic64_t *value);
> struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> struct dentry *parent, bool *value);
>
> @@ -291,6 +293,10 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
> atomic_t *value)
> { }
>
> +static inline void debugfs_create_atomic64_t(const char *name, umode_t mode,
> + struct dentry *parent, atomic64_t *value)
> +{ }
> +
> static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> struct dentry *parent,
> bool *value)
Looks good, but where is the user of this code? I can't add new apis
without a user.
And are you _SURE_ you want to be using an atomic64_t in the first
place? We are starting to reduce the "raw" usage of atomic variables as
almost no one needs them, they should be using something else instead,
or just a u64 as atomics are not needed for simple statistics.
thanks,
greg k-h
On Fri, 30 Oct 2020 at 18:20, Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Oct 30, 2020 at 06:04:42PM +1100, Anand K Mistry wrote:
> > This mirrors support for exporting atomic_t values.
> >
> > Signed-off-by: Anand K Mistry <[email protected]>
> >
> > ---
> >
> > fs/debugfs/file.c | 37 +++++++++++++++++++++++++++++++++++++
> > include/linux/debugfs.h | 6 ++++++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index a768a09430c3..798bd3bdedec 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -770,6 +770,43 @@ void debugfs_create_atomic_t(const char *name, umode_t mode,
> > }
> > EXPORT_SYMBOL_GPL(debugfs_create_atomic_t);
> >
> > +static int debugfs_atomic64_t_set(void *data, u64 val)
> > +{
> > + atomic64_set((atomic64_t *)data, val);
> > + return 0;
> > +}
> > +static int debugfs_atomic64_t_get(void *data, u64 *val)
> > +{
> > + *val = atomic64_read((atomic64_t *)data);
> > + return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t, debugfs_atomic64_t_get,
> > + debugfs_atomic64_t_set, "%lld\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL,
> > + "%lld\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_wo, NULL, debugfs_atomic64_t_set,
> > + "%lld\n");
> > +
> > +/**
> > + * debugfs_create_atomic64_t - create a debugfs file that is used to read and
> > + * write an atomic64_t value
> > + * @name: a pointer to a string containing the name of the file to create.
> > + * @mode: the permission that the file should have
> > + * @parent: a pointer to the parent dentry for this file. This should be a
> > + * directory dentry if set. If this parameter is %NULL, then the
> > + * file will be created in the root of the debugfs filesystem.
> > + * @value: a pointer to the variable that the file should read to and write
> > + * from.
> > + */
> > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > + struct dentry *parent, atomic64_t *value)
> > +{
> > + debugfs_create_mode_unsafe(name, mode, parent, value,
> > + &fops_atomic64_t, &fops_atomic64_t_ro,
> > + &fops_atomic64_t_wo);
> > +}
> > +EXPORT_SYMBOL_GPL(debugfs_create_atomic64_t);
> > +
> > ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> > size_t count, loff_t *ppos)
> > {
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index 851dd1f9a8a5..0fac84c53eab 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -126,6 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
> > struct dentry *parent, size_t *value);
> > void debugfs_create_atomic_t(const char *name, umode_t mode,
> > struct dentry *parent, atomic_t *value);
> > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > + struct dentry *parent, atomic64_t *value);
> > struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> > struct dentry *parent, bool *value);
> >
> > @@ -291,6 +293,10 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
> > atomic_t *value)
> > { }
> >
> > +static inline void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > + struct dentry *parent, atomic64_t *value)
> > +{ }
> > +
> > static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> > struct dentry *parent,
> > bool *value)
>
> Looks good, but where is the user of this code? I can't add new apis
> without a user.
Fair enough. Right now, the user is just some local
debugging/performance measuring which will never be upstreamed.
Happy to let this drop.
>
> And are you _SURE_ you want to be using an atomic64_t in the first
> place? We are starting to reduce the "raw" usage of atomic variables as
> almost no one needs them, they should be using something else instead,
> or just a u64 as atomics are not needed for simple statistics.
I understand, and would generally never use atomics in real code. I
used an atomic since I wanted accuracy
(for some of the benchmarks I want to run) but can't use anything that
blocks (spinlock/mutex) since
the code is somewhere inside the scheduler.
>
> thanks,
>
> greg k-h
--
Anand K. Mistry
Software Engineer
Google Australia
On Fri, Oct 30, 2020 at 06:30:49PM +1100, Anand K. Mistry wrote:
> On Fri, 30 Oct 2020 at 18:20, Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Fri, Oct 30, 2020 at 06:04:42PM +1100, Anand K Mistry wrote:
> > > This mirrors support for exporting atomic_t values.
> > >
> > > Signed-off-by: Anand K Mistry <[email protected]>
> > >
> > > ---
> > >
> > > fs/debugfs/file.c | 37 +++++++++++++++++++++++++++++++++++++
> > > include/linux/debugfs.h | 6 ++++++
> > > 2 files changed, 43 insertions(+)
> > >
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index a768a09430c3..798bd3bdedec 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -770,6 +770,43 @@ void debugfs_create_atomic_t(const char *name, umode_t mode,
> > > }
> > > EXPORT_SYMBOL_GPL(debugfs_create_atomic_t);
> > >
> > > +static int debugfs_atomic64_t_set(void *data, u64 val)
> > > +{
> > > + atomic64_set((atomic64_t *)data, val);
> > > + return 0;
> > > +}
> > > +static int debugfs_atomic64_t_get(void *data, u64 *val)
> > > +{
> > > + *val = atomic64_read((atomic64_t *)data);
> > > + return 0;
> > > +}
> > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t, debugfs_atomic64_t_get,
> > > + debugfs_atomic64_t_set, "%lld\n");
> > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL,
> > > + "%lld\n");
> > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_atomic64_t_wo, NULL, debugfs_atomic64_t_set,
> > > + "%lld\n");
> > > +
> > > +/**
> > > + * debugfs_create_atomic64_t - create a debugfs file that is used to read and
> > > + * write an atomic64_t value
> > > + * @name: a pointer to a string containing the name of the file to create.
> > > + * @mode: the permission that the file should have
> > > + * @parent: a pointer to the parent dentry for this file. This should be a
> > > + * directory dentry if set. If this parameter is %NULL, then the
> > > + * file will be created in the root of the debugfs filesystem.
> > > + * @value: a pointer to the variable that the file should read to and write
> > > + * from.
> > > + */
> > > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > > + struct dentry *parent, atomic64_t *value)
> > > +{
> > > + debugfs_create_mode_unsafe(name, mode, parent, value,
> > > + &fops_atomic64_t, &fops_atomic64_t_ro,
> > > + &fops_atomic64_t_wo);
> > > +}
> > > +EXPORT_SYMBOL_GPL(debugfs_create_atomic64_t);
> > > +
> > > ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
> > > size_t count, loff_t *ppos)
> > > {
> > > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > > index 851dd1f9a8a5..0fac84c53eab 100644
> > > --- a/include/linux/debugfs.h
> > > +++ b/include/linux/debugfs.h
> > > @@ -126,6 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
> > > struct dentry *parent, size_t *value);
> > > void debugfs_create_atomic_t(const char *name, umode_t mode,
> > > struct dentry *parent, atomic_t *value);
> > > +void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > > + struct dentry *parent, atomic64_t *value);
> > > struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> > > struct dentry *parent, bool *value);
> > >
> > > @@ -291,6 +293,10 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
> > > atomic_t *value)
> > > { }
> > >
> > > +static inline void debugfs_create_atomic64_t(const char *name, umode_t mode,
> > > + struct dentry *parent, atomic64_t *value)
> > > +{ }
> > > +
> > > static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
> > > struct dentry *parent,
> > > bool *value)
> >
> > Looks good, but where is the user of this code? I can't add new apis
> > without a user.
>
> Fair enough. Right now, the user is just some local
> debugging/performance measuring which will never be upstreamed.
> Happy to let this drop.
Now dropped!
> > And are you _SURE_ you want to be using an atomic64_t in the first
> > place? We are starting to reduce the "raw" usage of atomic variables as
> > almost no one needs them, they should be using something else instead,
> > or just a u64 as atomics are not needed for simple statistics.
>
> I understand, and would generally never use atomics in real code. I
> used an atomic since I wanted accuracy (for some of the benchmarks I
> want to run) but can't use anything that blocks (spinlock/mutex) since
> the code is somewhere inside the scheduler.
You can't be "accurate" for a single value from a single file read by
userspace as you don't know if it changed right after you read the value :)
Again, people abuse atomic values thinking they are something they
really aren't...
thanks,
greg k-h