Make lockToCurrentCPU re-entrant #3346
Conversation
|
I guess there could be an issue if some part of the program forks. I think nix may use pthreads, but I do not think it works with multiple processes. In that setup, the above patch works correctly. The example you describe in #3345 is not an IFD (or did I miss something ?). But you use a builtin that loads local filesystem data (filterSource & related). This works a bit differently than other derivations indeed. |
|
It is unfortunately IFD related, if the argument to |
|
Oh, I did not look closely enough. I did check that it works on the example you provided. So I can tell that it fixes your use case. But I do not know how nix forks and threads itself. That being said, the comment is quite clear about this being "only" a cpu cache optimization. So the change cat at worst miss an optimization opportunity. |
|
@edolstra WDYT? |
|
I marked this as stale due to inactivity. → More info |
There was a problem hiding this comment.
👍 on the idea. A couple small comments though.
Also, can you add a regression test for #3345 ? Either in tests/nix-shell.sh or as its own thing (unless it’s too complex to test properly, but I don’t think it should be)
| @@ -18,6 +20,12 @@ static cpu_set_t savedAffinity; | |||
| void setAffinityTo(int cpu) | |||
| { | |||
| #if __linux__ | |||
| if (didSaveAffinity) { | |||
| printError("setAffinity cannot be re-entered"); | |||
There was a problem hiding this comment.
| printError("setAffinity cannot be re-entered"); | |
| debug("Ignoring a second setAffinity call"); |
(or at least make it a warning rather than an error. But I don’t think it should pop-up like an important thing, esp. given that it’s only a small potential optimization that can’t be made)
| // `lockToCurrentCPU` is re-entrant, because it is idempotent. | ||
| // But we have to avoid calling `setAffinity` twice, | ||
| // as that would corrupt the `savedAffinity`. | ||
| if (!didLockCPU && cpu != -1) { | ||
| didLockCPU = true; | ||
| setAffinityTo(cpu); | ||
| } |
There was a problem hiding this comment.
I’m not sure I understand what the reason for this. Since setAffinityTo is pseudo-reentrant now, this code could stay unchanged (and we could avoid the need for the didLockCPU variable altogether), no?
Fixes #3345