On Tue, 2024-09-10 at 10:16 +0200, Christian Brauner wrote: > When I expanded uidgid mappings I intended for a struct uid_gid_map to > fit into a single cacheline on x86 as they tend to be pretty > performance sensitive (idmapped mounts etc). But a 4 byte hole was added > that brought it over 64 bytes. Fix that by adding the static extent > array and the extent counter into a substruct. C's type punning for > unions guarantees that we can access ->nr_extents even if the last > written to member wasn't within the same object. This is also what we > rely on in struct_group() and friends. This of course relies on > non-strict aliasing which we don't do. > > 99) If the member used to read the contents of a union object is not the > same as the member last used to store a value in the object, the > appropriate part of the object representation of the value is > reinterpreted as an object representation in the new type as > described in 6.2.6 (a process sometimes called "type punning"). > > Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > --- > Before: > > struct uid_gid_map { > u32 nr_extents; /* 0 4 */ > > /* XXX 4 bytes hole, try to pack */ > > union { > struct uid_gid_extent extent[5]; /* 8 60 */ > struct { > struct uid_gid_extent * forward; /* 8 8 */ > struct uid_gid_extent * reverse; /* 16 8 */ > }; /* 8 16 */ > }; /* 8 64 */ > > /* size: 72, cachelines: 2, members: 2 */ > /* sum members: 68, holes: 1, sum holes: 4 */ > /* last cacheline: 8 bytes */ > }; > > After: > > struct uid_gid_map { > union { > struct { > struct uid_gid_extent extent[5]; /* 0 60 */ > u32 nr_extents; /* 60 4 */ > }; /* 0 64 */ > struct { > struct uid_gid_extent * forward; /* 0 8 */ > struct uid_gid_extent * reverse; /* 8 8 */ > }; /* 0 16 */ > }; /* 0 64 */ > > /* size: 64, cachelines: 1, members: 1 */ > }; > --- > include/linux/user_namespace.h | 6 ++++-- > kernel/user.c | 6 +++--- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > index 6030a8235617..3625096d5f85 100644 > --- a/include/linux/user_namespace.h > +++ b/include/linux/user_namespace.h > @@ -21,9 +21,11 @@ struct uid_gid_extent { > }; > > struct uid_gid_map { /* 64 bytes -- 1 cache line */ > - u32 nr_extents; > union { > - struct uid_gid_extent extent[UID_GID_MAP_MAX_BASE_EXTENTS]; > + struct { > + struct uid_gid_extent extent[UID_GID_MAP_MAX_BASE_EXTENTS]; > + u32 nr_extents; > + }; Is this any different from just moving nr_extents to the end of struct_uid_gid_map? I don't quite get how moving it into the union improves things. > struct { > struct uid_gid_extent *forward; > struct uid_gid_extent *reverse; > diff --git a/kernel/user.c b/kernel/user.c > index aa1162deafe4..f46b1d41163b 100644 > --- a/kernel/user.c > +++ b/kernel/user.c > @@ -36,33 +36,33 @@ EXPORT_SYMBOL_GPL(init_binfmt_misc); > */ > struct user_namespace init_user_ns = { > .uid_map = { > - .nr_extents = 1, > { > .extent[0] = { > .first = 0, > .lower_first = 0, > .count = 4294967295U, > }, > + .nr_extents = 1, > }, > }, > .gid_map = { > - .nr_extents = 1, > { > .extent[0] = { > .first = 0, > .lower_first = 0, > .count = 4294967295U, > }, > + .nr_extents = 1, > }, > }, > .projid_map = { > - .nr_extents = 1, > { > .extent[0] = { > .first = 0, > .lower_first = 0, > .count = 4294967295U, > }, > + .nr_extents = 1, > }, > }, > .ns.count = REFCOUNT_INIT(3), > > --- > base-commit: 698e7d1680544ef114203b0cf656faa0c1216ebc > change-id: 20240910-work-uid_gid_map-cce46aee1b76 > Jeff Layton <jlayton@xxxxxxxxxx>
