Page MenuHomeFreeBSD

Update imgact_binmisc.c to use a sx lock instead of a mutex.
ClosedPublic

Authored by sbruno on Feb 26 2015, 2:03 PM.

Details

Reviewers
jhb
mjg
sson
Summary

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

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sson updated this revision to Diff 3991.Feb 26 2015, 2:03 PM
sson retitled this revision from to Update imgact_binmisc.c to use a sx lock instead of a mutex..
sson updated this object.
sson edited the test plan for this revision. (Show Details)
sson added a reviewer: sbruno.
sson set the repository for this revision to rS FreeBSD src repository.
sbruno requested changes to this revision.Feb 27 2015, 1:01 AM
sbruno edited edge metadata.

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.

This revision now requires changes to proceed.Feb 27 2015, 1:01 AM
sson updated this revision to Diff 4011.Feb 27 2015, 1:51 PM
sson edited edge metadata.

Simplify change by just acquiring exclusive lock and not trying to upgrade it.

sbruno requested changes to this revision.Feb 28 2015, 7:14 PM
sbruno edited edge metadata.

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
This revision now requires changes to proceed.Feb 28 2015, 7:14 PM
sson updated this revision to Diff 4077.EditedMar 2 2015, 8:24 PM
sson edited edge metadata.

This one should apply cleanly to head.

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
sbruno requested changes to this revision.Jun 4 2015, 8:35 PM
sbruno edited edge metadata.

Can you close this review out and "abandon" it in preference to D2123?

This revision now requires changes to proceed.Jun 4 2015, 8:35 PM
sson abandoned this revision.Jun 4 2015, 8:41 PM

This has been replaced by https://reviews.freebsd.org/D2123

sbruno commandeered this revision.Jun 5 2015, 6:08 PM
sbruno edited reviewers, added: sson; removed: sbruno.
sson added inline comments.Jun 5 2015, 6:10 PM
sys/kern/imgact_binmisc.c
669

This should stay a bcopy() since it is an overlaying copy. memcpy() doesn't do well with that.

sbruno updated this revision to Diff 5971.Jun 5 2015, 6:12 PM

Restore bcopy at line 670

sbruno marked an inline comment as done.Jun 5 2015, 6:13 PM
sbruno added inline comments.
sys/kern/imgact_binmisc.c
669

yepper, done.

sbruno updated this revision to Diff 5972.Jun 5 2015, 6:27 PM
sbruno marked an inline comment as done.

Last diff update really confused the heck out of phabricator. Try again with line 670
reverted back to bcopy.

sbruno added reviewers: mjg, jhb.Jun 5 2015, 7:03 PM
sson accepted this revision.Jun 8 2015, 3:09 PM
sson edited edge metadata.
This revision is now accepted and ready to land.Jun 8 2015, 3:09 PM
mjg edited edge metadata.Jun 8 2015, 5:03 PM

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.

sson added a comment.Jun 8 2015, 7:02 PM
In D1971#52645, @mjg wrote:

the problem of holding a non-sleep lock during a page fault as reported by witness

What page fault?

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.

emaste added a subscriber: emaste.Jun 13 2015, 4:14 PM
jhb added inline comments.Jun 13 2015, 6:14 PM
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?

sbruno added inline comments.Jun 13 2015, 9:30 PM
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?

sbruno updated this revision to Diff 6180.Jun 13 2015, 9:31 PM
sbruno edited edge metadata.

Remove whitespace nit.

This revision now requires review to proceed.Jun 13 2015, 9:31 PM
sbruno marked 2 inline comments as done.Jun 13 2015, 9:32 PM
sson added inline comments.Jun 14 2015, 3:01 AM
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.

sbruno updated this revision to Diff 6279.Jun 18 2015, 12:47 AM
sbruno edited edge metadata.

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.

sbruno marked 8 inline comments as done.Jun 18 2015, 12:48 AM
sbruno added inline comments.
sys/kern/imgact_binmisc.c
102

I don't think I'm going to do this.

sson accepted this revision.Jun 18 2015, 1:30 AM
sson edited edge metadata.
This revision is now accepted and ready to land.Jun 18 2015, 1:30 AM
sbruno closed this revision.Jun 18 2015, 2:07 AM
sbruno marked an inline comment as done.

Committed at svn r 284535