* [PATCH 0/1] debugfs: Add a helper to export atomic64_t values @ 2020-10-30 7:04 Anand K Mistry 2020-10-30 7:04 ` [PATCH 1/1] " Anand K Mistry 0 siblings, 1 reply; 5+ messages in thread From: Anand K Mistry @ 2020-10-30 7:04 UTC (permalink / raw) To: linux-kernel; +Cc: Anand K Mistry, Greg Kroah-Hartman, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] debugfs: Add a helper to export atomic64_t values 2020-10-30 7:04 [PATCH 0/1] debugfs: Add a helper to export atomic64_t values Anand K Mistry @ 2020-10-30 7:04 ` Anand K Mistry 2020-10-30 7:21 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Anand K Mistry @ 2020-10-30 7:04 UTC (permalink / raw) To: linux-kernel; +Cc: Anand K Mistry, Greg Kroah-Hartman, Rafael J. Wysocki This mirrors support for exporting atomic_t values. Signed-off-by: Anand K Mistry <amistry@google.com> --- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] debugfs: Add a helper to export atomic64_t values 2020-10-30 7:04 ` [PATCH 1/1] " Anand K Mistry @ 2020-10-30 7:21 ` Greg Kroah-Hartman 2020-10-30 7:30 ` Anand K. Mistry 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2020-10-30 7:21 UTC (permalink / raw) To: Anand K Mistry; +Cc: linux-kernel, Rafael J. Wysocki 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 <amistry@google.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] debugfs: Add a helper to export atomic64_t values 2020-10-30 7:21 ` Greg Kroah-Hartman @ 2020-10-30 7:30 ` Anand K. Mistry 2020-10-30 8:04 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Anand K. Mistry @ 2020-10-30 7:30 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel, Rafael J. Wysocki On Fri, 30 Oct 2020 at 18:20, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <amistry@google.com> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] debugfs: Add a helper to export atomic64_t values 2020-10-30 7:30 ` Anand K. Mistry @ 2020-10-30 8:04 ` Greg Kroah-Hartman 0 siblings, 0 replies; 5+ messages in thread From: Greg Kroah-Hartman @ 2020-10-30 8:04 UTC (permalink / raw) To: Anand K. Mistry; +Cc: linux-kernel, Rafael J. Wysocki 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 > <gregkh@linuxfoundation.org> 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 <amistry@google.com> > > > > > > --- > > > > > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-30 8:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-30 7:04 [PATCH 0/1] debugfs: Add a helper to export atomic64_t values Anand K Mistry 2020-10-30 7:04 ` [PATCH 1/1] " Anand K Mistry 2020-10-30 7:21 ` Greg Kroah-Hartman 2020-10-30 7:30 ` Anand K. Mistry 2020-10-30 8:04 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®