On Wed, 16 Apr 2025 at 06:17, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> Let's inline all low-level helpers and use likely()/unlikely() to help
> the compiler along.
Hmm. This looks like it might be a mistake - code generation will
probably not be great, because you still end up calling a real
function for some of the cases (ie from_kuid() in the actual real
translation case), so all the register allocation etc issues with
having a function call largely do remain.
Yes, it inlines things into generic_permission(), and that will avoid
the function call overhead for the common case (good), but it also
does make for bigger code generation. And it doesn't actually *change*
the code - it ends up doing all the same accesses, just the
instruction flow is slightly different.
So I think you'd actually be better off with *just* the IOP_USERNS
patch, and only inlining *that* fast-path case, and keep the other
cases out-of-line.
IOW - instead of only checking IOP_USERNS only in i_user_ns() and
making it return 'init_user_ns' without doing the pointer following, I
think you should make just our *existing* inlined i_uid_into_vfsuid()
helper be the minimal inlined wrapper around just the IOP_USERNS
logic.
Because right now the problem with i_uid_into_vfsuid() is two-fold
- it does that pointer chasing by calling i_user_ns(inode)
- it then calls make_vfsuid() which does lots of pointless extra work
in the common case
and I think both should be fixed.
Btw, make_vfsuid() itself is kind of odd. It does:
if (idmap == &nop_mnt_idmap)
return VFSUIDT_INIT(kuid);
if (idmap == &invalid_mnt_idmap)
return INVALID_VFSUID;
if (initial_idmapping(fs_userns))
uid = __kuid_val(kuid);
else
uid = from_kuid(fs_userns, kuid);
if (uid == (uid_t)-1)
return INVALID_VFSUID;
return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));
and honestly, that looks just horrendous for the actual simple cases.
I think it's a historical accident, but the
return VFSUIDT_INIT(kuid);
and the
uid = __kuid_val(kuid);
....
return VFSUIDT_INIT_RAW(map_id_down(&idmap->uid_map, uid));
things are actually the same exact "no mapping" code for the case we
care about most. We shouldn't even do that
if (uid == (uid_t)-1)
return INVALID_VFSUID;
case at all for that case, because the no-mapping situation is that
INVALID_VFSUID *is* (uid_t)-1, so all of this is entirely pointless.
So I think the inlined fast-case should be that
if (idmap == &nop_mnt_idmap || initial_idmapping(fs_userns))
return VFSUIDT_INIT(kuid);
code, and then the 'initial_idmapping()' thing should check the
IOP_USERNS bit explicitly, and never use the i_user_ns() helper at all
etc.
That case should then be "likely()", and the rest can remain out-of-line.
IOW: instead of inlining all the helpers, just make the *one* helper
that we already have (and is already a trivial inline function) be
much more targeted, and make that fast-case much more explicit.
Hmm?
Linus
