This change replaces the mutex with a sx lock for the interpreter list to avoid the problem of holding a non-sleep lock during a page fault as reported by witness. It also uses atomics where possible to avoid having to acquire the exclusive lock. In addition, it consistently uses memset()/memcpy() instead of bzero()/bcopy().
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
Somehow, this is causing errors with builds via poudriere. It is acting like the acquisition of the sx lock is failing for multiple processes instead of spinning.
Comment Actions
I don't know why http://svnweb.freebsd.org/base/head/sys/kern/imgact_binmisc.c?r1=264282&r2=271141&pathrev=271141 showed up in this diff. Weird.
Comment Actions
For reasons that are not clear, this does not cleanly apply to head.
Updated to revision 279400. root@dirty.ysv:/usr/src # patch -p0 < /var/tmp/patch.txt Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |Index: sys/kern/imgact_binmisc.c |=================================================================== |--- sys/kern/imgact_binmisc.c |+++ sys/kern/imgact_binmisc.c -------------------------- Patching file sys/kern/imgact_binmisc.c using Plan A... Hunk #1 succeeded at 1. Hunk #2 succeeded at 39. Hunk #3 succeeded at 98. Hunk #4 succeeded at 113. Hunk #5 succeeded at 154. Hunk #6 succeeded at 203. Hunk #7 succeeded at 260. Hunk #8 succeeded at 288. Hunk #9 succeeded at 312. Hunk #10 succeeded at 333. Hunk #11 succeeded at 351. Hunk #12 succeeded at 383. Hunk #13 succeeded at 405. Hunk #14 succeeded at 420. Hunk #15 succeeded at 557. Hunk #16 failed at 594. Hunk #17 succeeded at 650. Hunk #18 succeeded at 661. Hunk #19 succeeded at 721. Hunk #20 succeeded at 737. 1 out of 20 hunks failed--saving rejects to sys/kern/imgact_binmisc.c.rej done
Comment Actions
It does cleanly apply now. Poudriere now fails with:
root@dirty.ysv:/home/sbruno # poudriere bulk -a -j 11mips64 -p 11mips32 [00:00:00] ====>> Creating the reference jail... done [00:00:01] ====>> Mounting system devices for 11mips64-11mips32 [00:00:01] ====>> Mounting ports/packages/distfiles [00:00:01] ====>> Using packages from previously failed build [00:00:01] ====>> Mounting packages from: /usr/local/poudriere/data/packages/11mips64-11mips32 [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): lang/erlang [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): lang/erlang-runtime15 [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): lang/erlang-runtime16 [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): lang/erlang-runtime17 [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): lang/ratfor [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): security/gpgme [00:00:01] ====>> Warning: Blacklisting (from /usr/local/etc/poudriere.d/blacklist): security/p5-Module-Signature /etc/resolv.conf -> /usr/local/poudriere/data/.m/11mips64-11mips32/ref/etc/resolv.conf [00:00:01] ====>> Starting jail 11mips64-11mips32 make: "/usr/ports/Mk/bsd.port.mk" line 1191: warning: "/usr/bin/uname -p" returned non-zero status make: "/usr/ports/Mk/bsd.port.mk" line 1196: warning: "/usr/bin/uname -s" returned non-zero status make: "/usr/ports/Mk/bsd.port.mk" line 1199: warning: "/usr/bin/uname -r" returned non-zero status make: "/usr/ports/Mk/bsd.port.mk" line 1219: UNAME_r () and OSVERSION (1100058) do not agree on major version number. [00:00:02] ====>> Cleaning up [00:00:02] ====>> Umounting file systems
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 669 | This should stay a bcopy() since it is an overlaying copy. memcpy() doesn't do well with that. | |
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 669 | yepper, done. | |
Comment Actions
Last diff update really confused the heck out of phabricator. Try again with line 670
reverted back to bcopy.
Comment Actions
the problem of holding a non-sleep lock during a page fault as reported by witness
What page fault?
Cursory reading suggests a M_WAITOK malloc instead coming from sbuf_auto. The allocation could be done prior to taking any locks, or would be best avoided. The string in question is always freed as the function is exiting and /dev/fd/%d" will fit on a small buffer on the stack no problem.
As such, while I don't see problems with using sx locks here, it looks like the code could be otherwise improved and then switch to rwlocks instead.
Comment Actions
The page fault is caused by the bcopy() on line 670 when there are a lot of command-line arguments. The bcopy() moves everything down to make room for the interpreter path and arguments.
Cursory reading suggests a M_WAITOK malloc instead coming from sbuf_auto. The allocation could be done prior to taking any locks, or would be best avoided. The string in question is always freed as the function is exiting and /dev/fd/%d" will fit on a small buffer on the stack no problem.
That isn't where the fault happens but, yes, it seems that buffer could be pre-allocated before getting the lock.
As such, while I don't see problems with using sx locks here, it looks like the code could be otherwise improved and then switch to rwlocks instead.
I am not sure if the bcopy() could be done without holding a lock.
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 102 | Maybe call it 'interp_list_lock'? | |
| 269–270 | With an sx lock you can use the old logic order since you can hold it across the malloc(). | |
| 314 | It would seem simpler to just use an xlock here and use plain ops for updating ibe_flags since my guess is that enabling/disabling entries isn't really a hot path? | |
| 669 | Whitespace diff? | |
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 269–270 | oh ... is this a potential race here if we *do not* hold the lock across the call to imgact_binmisc_new_entry() call (e.g. we don't restore the old lock order?) | |
| 314 | hrm ... don't we need to make this atomic because we *REALLY* want to make sure this happens during a review of the interpret list? | |
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 269–270 | No, there is no race here. It is just imgact_binmisc_new_entry() mallocs a new entry but if it doesn't need the entry it free()'s it. With sleep locks we can malloc() it while holding the lock and, therefore, don't have to go to the trouble of pre-allocating a new entry. | |
| 314 | Yes, this is really a hot path and we could just exclusively lock it here and not use the atomics to clear the flag. | |
| 669 | No, this was memcpy() but memcpy() doesn't do play well overlapping memory which is required here. Sean changed it to bcopy() so now it looks like a whitespace diff. | |
Comment Actions
Move imgact_binmiscf_find_entry() code block back to original position to
avoid extra free from malloc call.
Change atomic_clear_32 back to a bitwise &= to restore old behavior.
| sys/kern/imgact_binmisc.c | ||
|---|---|---|
| 102 | I don't think I'm going to do this. | |