Since the exec_elf{32,64}() function needs to drop the vnode lock anyway, try to do it as early as possible. This just moves the unlock; the number of lock operations shouldn't change.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23636 Build 22621: arc lint + arc unit
Event Timeline
I do not understand why this patch is needed, and you did not bothered to explain.
My impression is that the change is for worse: previous approach was that the vnode is locked most of the time, and only unlocked when needed. Now it is locked only when needed, which should increase the number of atomics executed, and increase contention with other lock pretenders. Also, if going this route, the use of shared lock should be examined.
sys/kern/imgact_elf.c | ||
---|---|---|
661 | There is ASSERT_VOP_LOCKED() which must be used for such asserts, and which prints useful information about the vnode. | |
962 | You are locking vnode for each loadable segment, i.e. typically twice. This is arguably worse than keeping the vnode locked. |
You are right regarding the shared lock, this is the logical next step. I wanted to avoid mixing that change with refactoring.
But I don't see how the patch increases the number of locks/unlocks. Could you explain?
I did not count exact number of locks you take, but you now lock per segment, and do this more than once. So the number of locks taken is O(num of segments) instead of O(1).
That said, even if we assume that executables all have one segment (in fact two), then there is still more locks after your patch. I did not checked this.
And that's the part I don't get - where? From what I see, the lock is taken once, around load_sections().
That said, even if we assume that executables all have one segment (in fact two), then there is still more locks after your patch. I did not checked this.
There is one additional lock, at the end - we have to return with an exclusive lock held. I'll think about this some more.
sys/kern/imgact_elf.c | ||
---|---|---|
661 | Thanks; committed separately. |
But previously we were dropping and picking up the lock just before, to call malloc(9), so this should equal out?
We only drop lock for the malloc, where the layout of the binary is unusual, i.e. either note of phdr segments are not in the first page. There, we can do the trick, if this is what bothers you:
ptr = malloc(M_NOWAIT); if (ptr == NULL) { VOP_UNLOCK(); ptr = malloc(M_WAITOK); vn_lock(LK_RETRY); }
This would eliminate all unlocks except under the memory stress, even for the weird binaries.
But we also only call vn_rdwr in the exact same situations.
There, we can do the trick, if this is what bothers you:
ptr = malloc(M_NOWAIT); if (ptr == NULL) { VOP_UNLOCK(); ptr = malloc(M_WAITOK); vn_lock(LK_RETRY); }This would eliminate all unlocks except under the memory stress, even for the weird binaries.
But it would still leave the whole thing under exclusive lock.
I still fail to understand why the change is needed.
sys/kern/imgact_elf.c | ||
---|---|---|
1163 | Now look at this (and previous) goto ret. |
It reduces lock coverage. I have another patch (on top of this one), which moves load_sections() to the very end, which reduces lock coverage even more and gets rid of one lock/unlock pair.
sys/kern/imgact_elf.c | ||
---|---|---|
1163 | Yeah, just noticed that. |
sys/kern/imgact_elf.c | ||
---|---|---|
1059 | That would be much worse, imho. Look at error handling in do_execve(). How does the 'locked' variable make things harder to follow? Its meaning is quite obvious - iff it's true, the vp is locked. | |
1094 | But why? It would make it less obvious that the lock is not really needed at all on entry to this function. |
sys/kern/imgact_elf.c | ||
---|---|---|
1094 | You do have a point, though - the first conditional is used in loop from kern_exec(). Let me fix it. |
sys/kern/imgact_elf.c | ||
---|---|---|
1059 | Because locked variable basically means that the code author (in this case, the code mangler) gave up on understanding which blocks of code have the vnode locked, and which are not. So in essence, any addition of the code would require two branches, one for the locked case, another for unlocked. I strongly disagree with all versions of the proposed rearrangement of the chairs that you proposed so far. | |
1087 | No, I did not suggested that. Move the VOP_UNLOCK() after the if() that checks for alignment of phdr. So far this is the only good part. |
sys/kern/imgact_elf.c | ||
---|---|---|
1059 | Locked variable sometimes leads to that, yes. But not in this case - here it's very obvious that it's only used for error handling. An alternative would be to either add vn_lock() before each goto statement, or use a weird code at the end which looks like this: ret: ... return (error) ret_unlocked: vn_lock() goto ret; } Which is, IMHO, quite ugly. Also, this is not "rearrangement of chairs" - current approach to locking in ELF loader is hilariously bad. This, along with two other diffs, removes all the unnceccesary unlock/lock pairs and makes it use shared locking. | |
1087 | But why? There's nothing that requires the vnode lock there. |
I do not see that this could end in anything useful.
Start from the other end, lets talk about your supposed shared-locking patch for the exe vnode.
sys/kern/imgact_elf.c | ||
---|---|---|
1059 | No, it is not ugly. No, the current approach of locking in ELF loader is not bad as you try to represent it. Your diff does make the mess from the code that is understandable. |
Ok, let's do that. The larger patch is here: https://reviews.freebsd.org/D19909
sys/kern/imgact_elf.c | ||
---|---|---|
1059 | Yes, it is ugly. And yes, current "approach of locking" in ELF loader, which is basically "slap an exclusive lock around the whole thing and then drop it in random places so INVARIANTS doesn't panic" is pretty bad. |