Re: [PATCH] uidgid: make sure we fit into one cacheline

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>
> ---
> 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@kernel.org>