| Message ID | 20230405022702.753323-6-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: avoid userspace pressure on unwanted allocations | expand |
On Tue, Apr 4, 2023 at 7:27 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > Sometimes you want to add debugfs entries for atomic counters which > can be pretty large using atomic64_t. Add support for these. So I realize why you use atomic64, but I really suspect you'd be better off with just the regular "atomic_long". This is not some debug stat that we care deeply about on 32-bit, and "atomic64" is often really really nasty on 32-bit architectures. For example, on x86, instead of being a single instruction, it ends up being a cmpxchg loop. In fact, even a single atomic read is a cmpxchg (admittedly without the need for looping). And yeah, I realize that we don't have a "atomic_long" debugfs interface either. But I think we could just use atomic_long for the module code (avoiding all the horrors of 64-bit atomics on 32-bit architectures), and then using just 'var->counter' for the value. It's not like the debugfs stuff actually does any truly atomic updates. So something like debugfs_create_ulong(... &val->counter ..); instead of debugfs_create_atomic64(... &val ..); Hmm? I dunno. I just think this is not something that may be worth introducing a new thing for, when it is *so* painful on 32-bit, and doesn't seem worth it. Linus
On Wed, Apr 05, 2023 at 08:26:18AM -0700, Linus Torvalds wrote: > So I realize why you use atomic64, but I really suspect you'd be > better off with just the regular "atomic_long". <-- snip --> > So something like > > debugfs_create_ulong(... &val->counter ..); > > instead of > > debugfs_create_atomic64(... &val ..); > > Hmm? We already have debugfs_create_ulong(), it just uses unsigned long with no atomic_long. I can just use that then. Luis
On Wed, Apr 05, 2023 at 09:04:27AM -0700, Luis Chamberlain wrote: > On Wed, Apr 05, 2023 at 08:26:18AM -0700, Linus Torvalds wrote: > > So I realize why you use atomic64, but I really suspect you'd be > > better off with just the regular "atomic_long". > > <-- snip --> > > > So something like > > > > debugfs_create_ulong(... &val->counter ..); > > > > instead of > > > > debugfs_create_atomic64(... &val ..); > > > > Hmm? > > We already have debugfs_create_ulong(), it just uses unsigned long > with no atomic_long. I can just use that then. Oh but I don't get the atomic incs, so we'd need debugfs_create_atomic_long_t(). Luis
On Wed, Apr 5, 2023 at 9:11 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > Oh but I don't get the atomic incs, so we'd need debugfs_create_atomic_long_t(). No, you misunderstand what I meant. Just use "atomic_long_t" in the module code. But then the debugfs code should do debugfs_create_ulong(... &val->counter ..); to expose said atomic_long values. No need for new debugfs interfaces. Because "atomic_long" is just a regular "long" as far as plain read/set operations are concerned - which is all that the debugfs code does anyway. So I think you can do something like atomic_long_t total_mod_size; ... debugfs_create_ulong("total_mod_size", 0400, mod_debugfs_root, &total_mod_size.counter); but I didn't actually try to compile that kind of version. (I think "counter" is actually a _signed_ long, so maybe the types don't match). Linus
On Wed, Apr 05, 2023 at 09:23:09AM -0700, Linus Torvalds wrote: > On Wed, Apr 5, 2023 at 9:11 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > Oh but I don't get the atomic incs, so we'd need debugfs_create_atomic_long_t(). > > debugfs_create_ulong("total_mod_size", > 0400, mod_debugfs_root, > &total_mod_size.counter); > > but I didn't actually try to compile that kind of version. > > (I think "counter" is actually a _signed_ long, so maybe the types don't match). I see yes, sadly we'd have to cast to (unsigned long *) to make that work as atomic_long counters are long long int: debugfs_create_ulong("total_mod_size", 0400, mod_debugfs_root, - &total_mod_size.counter); + (unsigned long *)&total_mod_size.counter); That's: unsigned long min bits 32 long long min bits 64 But since we'd be doing our accounting in atomic_long and just use debugfs for prints I think that's fine. Just a bit ugly. Luis
From: Luis Chamberlain Luis Chamberlain > Sent: 05 April 2023 17:53 > > On Wed, Apr 05, 2023 at 09:23:09AM -0700, Linus Torvalds wrote: > > On Wed, Apr 5, 2023 at 9:11 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > Oh but I don't get the atomic incs, so we'd need debugfs_create_atomic_long_t(). > > > > debugfs_create_ulong("total_mod_size", > > 0400, mod_debugfs_root, > > &total_mod_size.counter); > > > > but I didn't actually try to compile that kind of version. > > > > (I think "counter" is actually a _signed_ long, so maybe the types don't match). > > I see yes, sadly we'd have to cast to (unsigned long *) to make that > work as atomic_long counters are long long int: > > debugfs_create_ulong("total_mod_size", > 0400, mod_debugfs_root, > - &total_mod_size.counter); > + (unsigned long *)&total_mod_size.counter); > > That's: > > unsigned long min bits 32 > long long min bits 64 > > But since we'd be doing our accounting in atomic_long and just use > debugfs for prints I think that's fine. Just a bit ugly. That isn't going to work. It is pretty much never right to do *(integer_type *)&integer_variable. But are you really sure 'atomic_long' is 'long long' doesn't sound right at all. 'long long' is 128bit on 64bit and 64bit on 32bit - so is always a double-register access. This is worse than atomic_u64. I should probably try to lookup and/or measure the performance of atomic operations (esp. cmpxchg) on x86. Historically they were a separate read and write bus cycles with the 'lock' signal asserted (and still are if they cross cache line boundaries). But it is possible that at least some of the locked operations (esp. the xchg ones) are implemented within the cache itself so are just a single cpu -> cache operation. If not it is actually possible that the _local variants that seem to have been added should not use the locked instructions! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Le 06/04/2023 à 10:15, David Laight a écrit : > From: Luis Chamberlain Luis Chamberlain >> Sent: 05 April 2023 17:53 >> >> On Wed, Apr 05, 2023 at 09:23:09AM -0700, Linus Torvalds wrote: >>> On Wed, Apr 5, 2023 at 9:11 AM Luis Chamberlain <mcgrof@kernel.org> wrote: >>>> >>>> Oh but I don't get the atomic incs, so we'd need debugfs_create_atomic_long_t(). >>> >>> debugfs_create_ulong("total_mod_size", >>> 0400, mod_debugfs_root, >>> &total_mod_size.counter); >>> >>> but I didn't actually try to compile that kind of version. >>> >>> (I think "counter" is actually a _signed_ long, so maybe the types don't match). >> >> I see yes, sadly we'd have to cast to (unsigned long *) to make that >> work as atomic_long counters are long long int: >> >> debugfs_create_ulong("total_mod_size", >> 0400, mod_debugfs_root, >> - &total_mod_size.counter); >> + (unsigned long *)&total_mod_size.counter); >> >> That's: >> >> unsigned long min bits 32 >> long long min bits 64 >> >> But since we'd be doing our accounting in atomic_long and just use >> debugfs for prints I think that's fine. Just a bit ugly. > > That isn't going to work. > It is pretty much never right to do *(integer_type *)&integer_variable. > > But are you really sure 'atomic_long' is 'long long' > doesn't sound right at all. > 'long long' is 128bit on 64bit and 64bit on 32bit - so is always > a double-register access. > This is worse than atomic_u64. On powerpc 'long long' is 64 bits on both PPC32 and PPC64. Christophe > > I should probably try to lookup and/or measure the performance > of atomic operations (esp. cmpxchg) on x86. > Historically they were a separate read and write bus cycles with > the 'lock' signal asserted (and still are if they cross cache > line boundaries). > But it is possible that at least some of the locked operations > (esp. the xchg ones) are implemented within the cache itself > so are just a single cpu -> cache operation. > If not it is actually possible that the _local variants that > seem to have been added should not use the locked instructions! > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Christophe Leroy > Sent: 06 April 2023 14:38 ... > On powerpc 'long long' is 64 bits on both PPC32 and PPC64. It probably in on x85-64 as well. By brain is getting frazzled. On 64bit long long ought to be 128bit. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 06.04.23 15:48, David Laight wrote: > From: Christophe Leroy >> Sent: 06 April 2023 14:38 > ... >> On powerpc 'long long' is 64 bits on both PPC32 and PPC64. > > It probably in on x85-64 as well. > By brain is getting frazzled. > > On 64bit long long ought to be 128bit. I might have been living under a rock, but the only requirement I am aware of is for long long to be at least 64bit wide. AFAIK, that is also the case on s390x and aarch64.
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 1f971c880dde..76d923503861 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -780,6 +780,42 @@ 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_SIGNED(fops_atomic64_t, debugfs_atomic64_t_get, + debugfs_atomic64_t_set, "%lld\n"); +DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(fops_atomic64_t_ro, debugfs_atomic64_t_get, NULL, + "%lld\n"); +DEFINE_DEBUGFS_ATTRIBUTE_SIGNED(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 ea2d919fd9c7..f5cc613a545e 100644 --- a/include/linux/debugfs.h +++ b/include/linux/debugfs.h @@ -136,6 +136,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); void debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent, bool *value); void debugfs_create_str(const char *name, umode_t mode,