Message ID | 20230405022702.753323-6-mcgrof@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp22007vqo; Tue, 4 Apr 2023 19:57:47 -0700 (PDT) X-Google-Smtp-Source: AKy350YlNpLA6VZW6tXkIFupLPN8pieUWsWfJp2kD88gCzU3I+bd4u3EGjVGijufIEpj/wjOys/t X-Received: by 2002:a17:902:c44b:b0:1a2:beda:73d3 with SMTP id m11-20020a170902c44b00b001a2beda73d3mr3891218plm.5.1680663467642; Tue, 04 Apr 2023 19:57:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680663467; cv=none; d=google.com; s=arc-20160816; b=V17xlrZvZf7ZEWHdqIAwa5awlZNFOXuznqZfZOoUahO1tTxOhR1ULQckz7sjGvtaHa 6UctssKsPGCBCB4TlwXDHsImzpin12xRYrl83093OJr9ixq2dQTUh2XbcHGYfnQSqVWd 9Mts+gorZRMwLQ6iD2RMprmblMbNHufoKiEpFoeErbOiof/XVKYKQUofM44TE1gagdfy rQLK0hLL6e+93u2+PEUnA5Ayx54OY7WwIVkbhtfN4i4/qzUDfyO1TdbTcWapl0taIvB8 1RYtj9ymbHiHMeytQWvZsX7jaGZht7ZMvfMgo4ktivVGAZHuezLC9kNVNY0dIhsYEGG4 MA7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=g3xEUG+GeiNt8COYV9iOFWQdMbRf7j0U/HejYbLvBNc=; b=ocYqhVtm76fXzT3gNxy5jHNK0rLqe5mXuYgrc/OkR6yC1e/jHSDjVx9HSyXWRwHacG XmkThkoCJRGMYhEggq+lqGmlHBvQlsPGZLSMwEeJHe+GdRHiZSfAuFNROvXkfa/RsW3Y vARnWS+rUh/TcjBUB5WAhAJ2wndK6Q8rgD05m06qAXTUlNtcG/eLcvanGabwicnoinZ8 XSPl1yqvViFR36VMxFe7qvu2F9xnypPYxae6DgBgcQvLAthfAz//BEwrv30jX/dJWS9t 67SnyLrZ0hu4t1ltqPpEL8WheqYJQ2WCvIA7XpybZHbY56nHnflZwBXUyolxL1VJFVY2 4wGQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@infradead.org header.s=bombadil.20210309 header.b="P3/p5oxl"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ik3-20020a170902ab0300b0019ce74dd5c8si11004738plb.529.2023.04.04.19.57.31; Tue, 04 Apr 2023 19:57:47 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@infradead.org header.s=bombadil.20210309 header.b="P3/p5oxl"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236741AbjDEC1S (ORCPT <rfc822;lkml4gm@gmail.com> + 99 others); Tue, 4 Apr 2023 22:27:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236548AbjDEC1Q (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 4 Apr 2023 22:27:16 -0400 Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:3::133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 821241FE3; Tue, 4 Apr 2023 19:27:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Sender:Content-Transfer-Encoding: MIME-Version:References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From: Reply-To:Content-Type:Content-ID:Content-Description; bh=g3xEUG+GeiNt8COYV9iOFWQdMbRf7j0U/HejYbLvBNc=; b=P3/p5oxlg67BaA9syMKtlCOWxa zYmjIpVx3QEiOmEABFzXo2pysW+8lq+nqFFDLk+WjISclD2WN2FmtQq1NVkWL7yrqK1/NAb5xuYqT 5BCderbYd+4Dk5Jajn2kXgFQxjFDBogHY2rRTnYs+AoSk8Xy27dt5WdFgc9GE84/6qD1y2Ys1vp3u 2zuKwg4YkPfQBhusX/yQJOadceV9l9egFgrt0amv2UEqxSl8X3XUqPBmbn0ji815gBc2qqklHj1gs 4lB4IiM8fJzkNk4ngNVDgAV+54FujVshmckISckIVcb2l3w9SGfGFIvqVRVsQZDbDxtm5/v2LcDDu CuXsxSGw==; Received: from mcgrof by bombadil.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1pjsrb-003A0Y-29; Wed, 05 Apr 2023 02:27:07 +0000 From: Luis Chamberlain <mcgrof@kernel.org> To: david@redhat.com, patches@lists.linux.dev, linux-modules@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, pmladek@suse.com, petr.pavlu@suse.com, prarit@redhat.com, torvalds@linux-foundation.org, gregkh@linuxfoundation.org, rafael@kernel.org Cc: christophe.leroy@csgroup.eu, tglx@linutronix.de, peterz@infradead.org, song@kernel.org, rppt@kernel.org, dave@stgolabs.net, willy@infradead.org, vbabka@suse.cz, mhocko@suse.com, dave.hansen@linux.intel.com, colin.i.king@gmail.com, jim.cromie@gmail.com, catalin.marinas@arm.com, jbaron@akamai.com, rick.p.edgecombe@intel.com, mcgrof@kernel.org Subject: [PATCH v2 5/6] debugfs: add debugfs_create_atomic64_t for atomic64_t Date: Tue, 4 Apr 2023 19:27:01 -0700 Message-Id: <20230405022702.753323-6-mcgrof@kernel.org> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20230405022702.753323-1-mcgrof@kernel.org> References: <20230405022702.753323-1-mcgrof@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: Luis Chamberlain <mcgrof@infradead.org> X-Spam-Status: No, score=-2.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762303376306534050?= X-GMAIL-MSGID: =?utf-8?q?1762303376306534050?= |
Series |
module: avoid userspace pressure on unwanted allocations
|
|
Commit Message
Luis Chamberlain
April 5, 2023, 2:27 a.m. UTC
Sometimes you want to add debugfs entries for atomic counters which can be pretty large using atomic64_t. Add support for these. Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- fs/debugfs/file.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/debugfs.h | 2 ++ 2 files changed, 38 insertions(+)
Comments
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,