On Wed, Apr 16, 2025 at 08:04:26AM -0700, Linus Torvalds wrote: > 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. Sorry, I just had time to get back to this now. > > 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. Good point. > > 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. Right. > > 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. Yes, agreed. I've mentioned that various times on other patch series. It was just a matter of time and I had prioritized other stuff. > > 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? Yes, but I'd also like to add the shortcut to i_user_ns() because it's called in quite a few places that it might actually matter. So both initial_imdapping() and i_user_ns() should short-circuit ad initial_idmapping() should take an inode as the argument just as i_user_ns(). I'll send a new version out today hopefully. Thanks for taking the time to review this!
