Re: [PATCH v2 5/6] debugfs: add debugfs_create_atomic64_t for atomic64_t

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: David Laight <David.Laight@ACULAB.COM>,
	'Luis Chamberlain' <mcgrof@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Cc: "david@redhat.com" <david@redhat.com>,
	"patches@lists.linux.dev" <patches@lists.linux.dev>,
	"linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"pmladek@suse.com" <pmladek@suse.com>,
	"petr.pavlu@suse.com" <petr.pavlu@suse.com>,
	"prarit@redhat.com" <prarit@redhat.com>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"song@kernel.org" <song@kernel.org>,
	"rppt@kernel.org" <rppt@kernel.org>,
	"dave@stgolabs.net" <dave@stgolabs.net>,
	"willy@infradead.org" <willy@infradead.org>,
	"vbabka@suse.cz" <vbabka@suse.cz>,
	"mhocko@suse.com" <mhocko@suse.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"colin.i.king@gmail.com" <colin.i.king@gmail.com>,
	"jim.cromie@gmail.com" <jim.cromie@gmail.com>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"jbaron@akamai.com" <jbaron@akamai.com>,
	"rick.p.edgecombe@intel.com" <rick.p.edgecombe@intel.com>
Subject: Re: [PATCH v2 5/6] debugfs: add debugfs_create_atomic64_t for atomic64_t
Date: Thu, 6 Apr 2023 13:38:07 +0000	[thread overview]
Message-ID: <48f2c6f2-f776-be29-7d74-67f1b6cf5467@csgroup.eu> (raw)
In-Reply-To: <979c8cf9ab2d4fcba811adc0c563b8bb@AcuMS.aculab.com>

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)