I suspect this is all done in the wrong place/moment. What you need is to specially handle root, not the symlink. Then kern_alternate_path() can go away as well I suspect.

In D38933#886562, @kib wrote:

I suspect this is all done in the wrong place/moment. What you need is to specially handle root, not the symlink. Then kern_alternate_path() can go away as well I suspect.

Are you mean by 'specially handle root' change root aka chroot()? I suspect, you are mean something else.
In my POV kern_alternate_path() is a big issue, it does more unexpected harm than helpful, however, retirement it is a POLA. Or we must have a way to 'mount" some files from /etc into this root.

At least it would be good to change root for the exec() time to guarantee that Linux rtld will not look at non-native directories.

In D38933#886684, @dchagin wrote:

In D38933#886562, @kib wrote:

I suspect this is all done in the wrong place/moment. What you need is to specially handle root, not the symlink. Then kern_alternate_path() can go away as well I suspect.

Are you mean by 'specially handle root' change root aka chroot()? I suspect, you are mean something else.
In my POV kern_alternate_path() is a big issue, it does more unexpected harm than helpful, however, retirement it is a POLA. Or we must have a way to 'mount" some files from /etc into this root.

At least it would be good to change root for the exec() time to guarantee that Linux rtld will not look at non-native directories.

No, I mean a place where the "/" is resolved in namei(). The retry with alternative path should happen somewhere around places near the calls to namei_handle_root().

Something does not add up whatsoever with your bench results -- how is this patch supposed to improve scalability for open/close/unlink? Similarly it can't be faster than stock code for regular lookups, but this one is perhaps measurement error.

That aside the entire idea of Linux binaries doing 2 lookups was incredibly dodgy from the get go and I don't think this is helping the fundamental problem, albeit it may be it makes it less iffy.

The added branchfest is definitely not nice, especially the restart clause.

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

Even if going this route, I think the functionality can be added without pessimizing existing code. Note that for example vfs_lookup is already a standalone routine.

In D38933#897702, @mjg wrote:

Something does not add up whatsoever with your bench results -- how is this patch supposed to improve scalability for open/close/unlink?

This patch is not supposed to improve scalability, I used will-it-scale to check that I do not broke hot path.

That aside the entire idea of Linux binaries doing 2 lookups was incredibly dodgy from the get go and I don't think this is helping the fundamental problem, albeit it may be it makes it less iffy.

The added branchfest is definitely not nice, especially the restart clause.

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

Even if going this route, I think the functionality can be added without pessimizing existing code. Note that for example vfs_lookup is already a standalone routine.

Well, I mostly agree with all of you statement. Except some sort of pessimization, I tried to minimize touching of the hot path for native binaries.
This patch adds only two compares (pwd->pwd_rdir != pwd->pwd_adir) on error path of namei() for native. I don't think it's worth the cost.

Implementing things you are talking about requires rewriting our Linux emulation ports infrastructure as they installs files into the base not in the ABI directory.

In D38933#897717, @dchagin wrote:

In D38933#897702, @mjg wrote:

Something does not add up whatsoever with your bench results -- how is this patch supposed to improve scalability for open/close/unlink?

This patch is not supposed to improve scalability, I used will-it-scale to check that I do not broke hot path.

I am saying that according to the graph it did improve, markedly so, but this can't be true and consequently the bench is bogus.

That aside the entire idea of Linux binaries doing 2 lookups was incredibly dodgy from the get go and I don't think this is helping the fundamental problem, albeit it may be it makes it less iffy.

The added branchfest is definitely not nice, especially the restart clause.

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

Even if going this route, I think the functionality can be added without pessimizing existing code. Note that for example vfs_lookup is already a standalone routine.

Well, I mostly agree with all of you statement. Except some sort of pessimization, I tried to minimize touching of the hot path for native binaries.
This patch adds only two compares (pwd->pwd_rdir != pwd->pwd_adir) on error path of namei() for native. I don't think it's worth the cost.

It adds a branch to set things up and another one for failed lookups. clang probably also pessimized namei entry, which already is quite bad. Perhaps I should note there several single-threaded slowdowns remaining, most of them branches and this goes counter to whacking them.

I just realized your restart label is early enough that it virtually counts as a separate namei call, included repeated copy of the buffer et al. If paying that cost, you can *avoid* modifying any of it anyway and instead call namei with a faked pwd -- it would have all the usual vnodes *and* the one for /compat/linux sneaked in as root. Should this fail to produce the result, you use the real pwd. et voila, changing namei avoided.

Another note is that should this kind of double lookup need to be optimized, the way to do it for lockless lookup would be to patch the case which finds a negative entry to check if perhaps another variant is needed. This would avoid *any* work for lookups which do succeed. Anyhow so far it looks like you should be fine rolling with *calling* namei twice.

Implementing things you are talking about requires rewriting our Linux emulation ports infrastructure as they installs files into the base not in the ABI directory.

The current state is a mess and this sounds like an opportunity to sort it out, if feasible.

I was so startled by the supposed scalability diff I did not take a proper look at the other results.

700k ops/s is abysmall, perhaps your cpu is shafted with meltdown or you are running a debug kernel or fast path lookup is disabled in your case

What's more important is that it flattens for scalability, which would not happen for separate file case. I just verified it scales almost linearly up to 52 workers (i don't have more cores on my test box).

tl;dr the test is defo wrong

In D38933#897702, @mjg wrote:

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

This functionality (double lookup in the ugly current form) was added exactly to avoid requiring users doing what you described above.

In D38933#897720, @mjg wrote:

In D38933#897717, @dchagin wrote:

In D38933#897702, @mjg wrote:

Something does not add up whatsoever with your bench results -- how is this patch supposed to improve scalability for open/close/unlink?

This patch is not supposed to improve scalability, I used will-it-scale to check that I do not broke hot path.

I am saying that according to the graph it did improve, markedly so, but this can't be true and consequently the bench is bogus.

That aside the entire idea of Linux binaries doing 2 lookups was incredibly dodgy from the get go and I don't think this is helping the fundamental problem, albeit it may be it makes it less iffy.

The added branchfest is definitely not nice, especially the restart clause.

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

Even if going this route, I think the functionality can be added without pessimizing existing code. Note that for example vfs_lookup is already a standalone routine.

Well, I mostly agree with all of you statement. Except some sort of pessimization, I tried to minimize touching of the hot path for native binaries.
This patch adds only two compares (pwd->pwd_rdir != pwd->pwd_adir) on error path of namei() for native. I don't think it's worth the cost.

It adds a branch to set things up and another one for failed lookups. clang probably also pessimized namei entry, which already is quite bad. Perhaps I should note there several single-threaded slowdowns remaining, most of them branches and this goes counter to whacking them.

it's adds only two comparison and conditional jumping for the native binary (namei does not restart for native)

I just realized your restart label is early enough that it virtually counts as a separate namei call, included repeated copy of the buffer et al. If paying that cost, you can *avoid* modifying any of it anyway and instead call namei with a faked pwd -- it would have all the usual vnodes *and* the one for /compat/linux sneaked in as root. Should this fail to produce the result, you use the real pwd. et voila, changing namei avoided.

you propose to preserve the ugly kern_alternate_path way ?

Another note is that should this kind of double lookup need to be optimized, the way to do it for lockless lookup would be to patch the case which finds a negative entry to check if perhaps another variant is needed. This would avoid *any* work for lookups which do succeed. Anyhow so far it looks like you should be fine rolling with *calling* namei twice.

Interesting, if you don't mind, could you please tell me where to start there, for lockless lookup part?

Implementing things you are talking about requires rewriting our Linux emulation ports infrastructure as they installs files into the base not in the ABI directory.

The current state is a mess and this sounds like an opportunity to sort it out, if feasible.

Our ports are still centos-7 (EOL in 2024), there is no one on the horizon who is willing to do anything with that

You can implement namei_altroot(struct nameidata *nd, struct vnode *altroot) (or whatever the name) where altroot is guaranteed v_usecount > 0. Then it can handle faking pwd for the first pass without polluting any consumers.

you could store the vnode in that linux-specific struct

also note this avoids growing struct pwd which right now is 32 bytes, fitting very nicely uma

In D38933#897702, @mjg wrote:

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux or whatever you are looking up against and then have nullfs mounts inside for /home, /tmp and whatever else which makes sense to share. This avoids any suspicious lookups like failing to find a file in Linux because it is missing when it should not and trying to pick up the FreeBSD one. This also avoids adding any complexity to the kernel.

FWIW, https://reviews.freebsd.org/D25501

In D38933#897702, @mjg wrote:

I strongly suspect the right way is to have linux binaries auto chrooted to /compat/linux

No, please, lets don't do that. It'd be impossible to run some Linux text editor that accesses your $HOME.

PR: 72920

So there is still a case. Imagine that Linux process is chrooted into a subtree with its own '/compat/linux'. It does not start using this new adir. Might be it needs a namei() in chroot (or rather it should be sysent method?).

Otherwise, it looks fine to me, mostly. It would be easier to proceed if you split this patch into _many_ pieces. For instance, you could move all Linux changes into separate commit. Same for the removal of sv_imgact_try. Not sure about core namei() changes.

In D38933#910452, @kib wrote:

So there is still a case. Imagine that Linux process is chrooted into a subtree with its own '/compat/linux'. It does not start using this new adir. Might be it needs a namei() in chroot (or rather it should be sysent method?).

ugh, now this code in https://reviews.freebsd.org/D40090

Im sorry, can’t imagine how it can, linux_pwd_exec calls namei() unconditionally, and returns namei error only if not in chroot (jail).
I’d say more, now the one can do jexec chroot /compat/ubuntu or jexec /compat/ubuntu/bin/bash, i.e., fully isolate Linuxulator.

Otherwise, it looks fine to me, mostly. It would be easier to proceed if you split this patch into _many_ pieces. For instance, you could move all Linux changes into separate commit. Same for the removal of sv_imgact_try. Not sure about core namei() changes.

done,