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.
Tags
None
Referenced Files
F80157913: D1971.id5972.diff
Thu, Mar 28, 4:50 PM
Unknown Object (File)
Tue, Mar 26, 9:19 PM
Unknown Object (File)
Tue, Mar 26, 9:17 PM
Unknown Object (File)
Fri, Mar 15, 3:38 PM
Unknown Object (File)
Tue, Mar 5, 6:46 PM
Unknown Object (File)
Tue, Mar 5, 6:46 PM
Unknown Object (File)
Tue, Mar 5, 6:46 PM
Unknown Object (File)
Tue, Mar 5, 6:46 PM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

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 - subversion.
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 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 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
sbruno edited reviewers, added: sson; removed: sbruno.
sys/kern/imgact_binmisc.c
670

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

Restore bcopy at line 670

sbruno added inline comments.
sys/kern/imgact_binmisc.c
670

yepper, done.

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.

sson edited edge metadata.
This revision is now accepted and ready to land.Jun 8 2015, 3:09 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.

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.

sys/kern/imgact_binmisc.c
100–101

Maybe call it 'interp_list_lock'?

264

With an sx lock you can use the old logic order since you can hold it across the malloc().

315

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?

670

Whitespace diff?

sys/kern/imgact_binmisc.c
264

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?)

315

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 edited edge metadata.

Remove whitespace nit.

This revision now requires review to proceed.Jun 13 2015, 9:31 PM
sys/kern/imgact_binmisc.c
264

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.

315

Yes, this is really a hot path and we could just exclusively lock it here and not use the atomics to clear the flag.

670

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 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 added inline comments.
sys/kern/imgact_binmisc.c
100–101

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

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

Committed at svn r 284535