On 07.08.24 15:35, Steven Rostedt wrote:
> On Wed, 7 Aug 2024 13:51:39 +0200
> Mathias Krause <[email protected]> wrote:
>
>> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
>> index f704d8348357..a7769857962a 100644
>> --- a/fs/tracefs/internal.h
>> +++ b/fs/tracefs/internal.h
>> @@ -10,10 +10,8 @@ enum {
>> };
>>
>> struct tracefs_inode {
>> - union {
>> - struct inode vfs_inode;
>> - struct rcu_head rcu;
>> - };
>> + struct inode vfs_inode;
>> + struct rcu_head rcu;
>
> I rather not make this structure any bigger for the rcu element that is not
> used until freed.
Uhm, at least for my config, it won't consume more memory, as the slab
object is big enough to cover up for the additional two machine words:
root@deb11-amd64:~# slabinfo tracefs_inode_cache
Slabcache: tracefs_inode_cache Aliases: 0 Order : 3 Objects: 144
** Reclaim accounting active
Sizes (bytes) Slabs Debug Memory
------------------------------------------------------------------------
Object : 1200 Total : 6 Sanity Checks : Off Total: 196608
SlabObj: 1328 Full : 4 Redzoning : Off Used : 172800
SlabSiz: 32768 Partial: 0 Poisoning : Off Loss : 23808
Loss : 128 CpuSlab: 2 Tracking : Off Lalig: 18432
Align : 8 Objects: 24 Tracing : Off Lpadd: 5376
[...]
While the size of 'struct tracefs_inode' is 1200 bytes for my kernel
build (LOCKDEP bloats it quite a lot), the slab object size is 1328
bytes, i.e. 128 bytes wasted per object which can, for sure, cover up
for these additional members.
>
>> /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
>> struct list_head list;
>> unsigned long flags;
>
> Perhaps:
>
> diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> index f704d8348357..ab6d6c3d835d 100644
> --- a/fs/tracefs/internal.h
> +++ b/fs/tracefs/internal.h
> @@ -10,12 +10,12 @@ enum {
> };
>
> struct tracefs_inode {
> + struct inode vfs_inode;
> + /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
> union {
> - struct inode vfs_inode;
> + struct list_head list;
> struct rcu_head rcu;
> };
> - /* The below gets initialized with memset_after(ti, 0, vfs_inode) */
> - struct list_head list;
> unsigned long flags;
> void *private;
> };
I'd rather not exchange trashing one RCU-walked list for another. Or how
will this play out for the RCU walk in tracefs_apply_options() if
there's a concurrent call to tracefs_free_inode() which will now trash
the list_head tracefs_apply_options() is walking over?
Thanks,
Mathias
