Details

Reviewers

kmacy
wblock
adrian
rwatson
sson

Group Reviewers

manpages

Summary

This is based on the work of Stacey Son (sson@).
It was hammered into 12-current based on the patches
from 2014_06_01 and NextBSD.

It is considered WIP: handle with care, beware of dog.
sson's notes may apply:
https://people.freebsd.org/~sson/thrworkq/TODO.txt

Test Plan

It is only compile-tested on amd64 at this time.

MIPS build fails with a linking issue for the tests.
usr/lib/libpthread.so: undefined reference to `__sync_sub_and_fetch_4'

SPARC64 build fails:
head/sys/kern/kern_thrworkq.c:841: error: too few arguments to function 'cpu_set_upcall'

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6240
Build 6483: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

wblock added a subscriber: wblock.Jul 25 2015, 2:12 AM

wblock added inline comments.

share/man/man3/pthread_workqueue_np.3
73	s/the following/these/
80	Better without the leading "The".
82	Better without the leading "The".
84	Better without the leading "The".
87	Better without the leading "The".
89	Better without the leading "The".
117	This should be either ...by the .Fa workq parameter. Or better, just ...by .Fa workq .
141	Avoid pauses when possible. Generally, "may" is for permission, and "might" is for possibility: More threads will be started as needed when the overcommit flag is set. This might overcommit the physical resources of the system.
145	s/the following/these/
148	How about: Queued work items with this attribute will be given higher priority by
151	As above, "Queued..."
154	As above, "Queued..."
168	This is confusing worded and formatted. Is it this way because more error conditions and return codes will be added? function will fail in the following conditions with these return codes: (I avoid "the following" because it is usually filler. Here, it replaces the confusing "fail in these conditions".)
176	As above, confusing wording and table. Same for the rest of these.
237	Typo: s/destory/destroy/ This sentence can be written without the pauses: There is no current way to remove or destroy work queues or pending

I will incorporate this in to my Mach/Darwin/launchd branch after some further review. This will at least give the change some wider exposure.

In D3189#68797, @kmacy wrote:

I will incorporate this in to my Mach/Darwin/launchd branch after some further review. This will at least give the change some wider exposure.

Great! I expect to be back on this in a couple of weeks, when the GSoC is over.

Update manpage.
(Kicking a tinderbox build in today's -current.)

Herald edited edge metadata. · View Herald TranscriptAug 14 2015, 8:04 PM

Build break:
...

> lib/libthr (obj,depend,all,install)

/scratch/tmp/pfg/head/lib/libthr/thread/thr_init.c: In function
'init_private':
/scratch/tmp/pfg/head/lib/libthr/thread/thr_init.c:484: error:
'_thr_stack_initial' undeclared (first use in this function)
/scratch/tmp/pfg/head/lib/libthr/thread/thr_init.c:484: error: (Each
undeclared identifier is reported only once
/scratch/tmp/pfg/head/lib/libthr/thread/thr_init.c:484: error: for each
function it appears in.)

It looks like _thr_stack_initial is no more so just drop the assignment
for now.

(This really needs some review by sson@)

Herald edited edge metadata. · View Herald TranscriptAug 14 2015, 8:18 PM

wblock added inline comments.Aug 14 2015, 10:45 PM

share/man/man3/pthread_workqueue_np.3
141	That did not help. The original version of this sentence was missing commas. Corrected, it would be like this: If the overcommit flag is set, then more threads will be started, if needed, which might overcommit the physical resources of the system. It has a lot of pauses and is not clear. If you really want to keep the if/then structure, how about this: When the overcommit flag is set, more threads will be started as needed. This might overcommit the physical resources of the system.
237	Typo: s/currentlu/currently/

More manpage fixes (Thanks Warren!).

This appears to build but I make no claims ... not after having brutally
hacked it into compilation :-/

Herald edited edge metadata. · View Herald TranscriptAug 15 2015, 12:52 AM

wblock added inline comments.Aug 15 2015, 2:52 AM

share/man/man3/pthread_workqueue_np.3
142	Good, but new sentences should start on a new line. So: When the overcommit flag is set, more threads will be started as needed. This might overcommit the physical resources of the system.

More man page cleanup.

Leave the '_thr_stack_initial' line commented as a reminder that
we may want to preserve rlim.rlim_cur.

Herald edited edge metadata. · View Herald TranscriptAug 15 2015, 2:35 PM

pfg marked an inline comment as done.Aug 15 2015, 2:39 PM

pfg added inline comments.

share/man/man3/pthread_workqueue_np.3
143	I was considering a colon there When the overcommit flag is set, more threads will be started as needed: this might overcommit the physical resources of the system. I left it as you suggested though.

Approved for the current version of the man page. Remember that it never hurts to check man pages with igor -R and mandoc -Tlint, and also remember to update the .Dd before commit. Thanks!

share/man/man3/pthread_workqueue_np.3
144	A semicolon would be usable there, but breaking the sentence in two is simpler.

This revision is now accepted and ready to land.Aug 16 2015, 2:46 AM

Just a note that this patch has never been compiled with THRWORKQ actually enabled.

kmacy added inline comments.Aug 23 2015, 1:53 AM

sys/kern/kern_thr.c
338	It's not a difficult change. But the fact that this code could never have compiled concerns me.

What happened to thr_workq.c from his patch? That appears to be completely missing.

In D3189#70646, @kmacy wrote:

Just a note that this patch has never been compiled with THRWORKQ actually enabled.

I honestly only made sure the patch still applies so that it doesn't get lost and I haven't really gone through enabling it or much less testing it. The code needs real review but I also think it's critical to find a set of tests or at least find examples from Apple.

I am also busy putting together the results of the GSoC 2015 and can't play with this at this time. So .. yes you have reasons for concern: I am just hoping that keeping the patch in some form will make it easier to re-take where it was left off.

pfg updated this object.Aug 23 2015, 2:47 AM

pfg edited edge metadata.

In D3189#70648, @kmacy wrote:

What happened to thr_workq.c from his patch? That appears to be completely missing.

I don't know what happened in arcanist ... I will reload the patch.

Add back thr_workq.c which for some reason disappeared.

This revision now requires review to proceed.Aug 23 2015, 2:58 AM

Thanks. I've updated it a bit in my tree and am enabling libdispatch to use it. I'll point you at the set of commits when I've finished.

I've committed this patch in four parts now that it compiles, links, and doesn't cause panics.

Add kernel support:
https://github.com/freebsd/freebsd/commit/1b67658d064193c0e676058eadec0940076bebb6
Add man page:
https://github.com/freebsd/freebsd/commit/228ecc72a86e4e26e43cfc13be8806ba9b5c900a
add user-side support:
https://github.com/freebsd/freebsd/commit/9ecf34aacb8b0bbc789553c91cd968b14c438336
fix workq initialization related panics:
https://github.com/freebsd/freebsd/commit/2ab13e4ec1e283d4c51a8a9f2f31cd712748d272

I'm seeing assertion failures in jemalloc in client programs in tsd_arenas_cache_set. I'll update this again if and when this appears to be a stable implementation.

And this so that suspend doesn't get confused by unscheduled workq threads:
https://github.com/freebsd/freebsd/commit/3d020b415731cec113f049191a4520e3ffda0580

and this: https://github.com/freebsd/freebsd/commit/550d458c1108c9e12574fa226c93fc0cb299e95d

Wow, really nice Kip!

I am somewhat busy at this time ... perhaps you could take over the revision? I will still keep around and will be willing to shepherd the code into the tree when it's ready.

brueffer added a subscriber: brueffer.Aug 24 2015, 2:45 PM

brueffer added inline comments.

share/man/man3/pthread_workqueue_np.3
103	is .Dv NULL then...
142	New sentences should start on a new line.
191	Cannot
244	.An Stacey Son Aq Mt sson@FreeBSD.org .

Bring Kip Macy's fixes and enhancements.

Address brueffer's comments.

Build on MIPS is broken:

> lib/libc/tests/gen/posix_spawn (all)

2+0 records in
2+0 records out
2048 bytes transferred in 0.000042 secs (48360056 bytes/sec)
/scratch/tmp/pfg/obj/mips.mips64el/scratch/tmp/pfg/head/tmp/usr/lib/libpthread.so: undefined reference to `sync_sub_and_fetch_4'
/scratch/tmp/pfg/obj/mips.mips64el/scratch/tmp/pfg/head/tmp/usr/lib/libpthread.so: undefined reference to `sync_add_and_fetch_4'

nice_test ---
[nice_test] Error code 1

make[9]: stopped in /scratch/tmp/pfg/head/lib/libc/tests/gen
1 error

Update just enough to get this to compile in 12-current.
May not pass a universe build just yet.

Missing manpage.

Add missing untracked file: pthread_workqueue.h

Another missing file: thr_workq.c.

Bring some fixes from NextBSD.

Also remove a re-declaration in
sys/i386/include/atomic.h

This builds on AMD64 but I still get failures on MIPS:

...

> lib/libc/tests/db (all)

> lib/libc/tests/gen (all)

dlopen_empty_test.o: In function `atfu_dlopen_empty_test_body':
/scratch/tmp/pfg/head/lib/libc/tests/gen/dlopen_empty_test.c:66: warning:
warning: mktemp() possibly used unsafely; consider using mkstemp()
/scratch/tmp/pfg/obj/mips.mips/scratch/tmp/pfg/head/tmp/usr/lib/libpthread.so:
undefined reference to `sync_sub_and_fetch_4'
/scratch/tmp/pfg/obj/mips.mips/scratch/tmp/pfg/head/tmp/usr/lib/libpthread.so:
undefined reference to `sync_add_and_fetch_4'

nice_test.full ---

...
This seems to be a bug in the testsuite, but needs more investigation.

This really needs more help.

pfg edited the test plan for this revision. (Show Details)Dec 14 2016, 8:36 PM

wblock added inline comments.Dec 16 2016, 6:54 PM

share/man/man3/pthread_workqueue_np.3
91	"can" rather than "may" sounds better to me.

so it's just missing those atomic ops for MIPS32/MIPS64? It compiles/works fine on other architectures?

In D3189#182628, @adrian wrote:

so it's just missing those atomic ops for MIPS32/MIPS64? It compiles/works fine on other architectures?

It is also failing on SPARC64 :(.

Apparently it works on amd64 and i386, other platforms build but remain untested. I think sson@ told me at BSDCan 2014 that it was done but the TODO list has:

kern_thrworkq code:

The code to assign CPU affinity needs to be fixed up and activited. I am not sure if this is really needed.

Bring up to date with latest head.

Recent changes cause breakage in lib/libthr/thread/thr_workq.c:

> lib/libthr (obj,all,install)

cc1: warnings being treated as errors
/scratch/tmp/head/lib/libthr/thread/thr_workq.c: In function 'thr_workq_init':
/scratch/tmp/head/lib/libthr/thread/thr_workq.c:266: warning: assignment from incompatible pointer type

thr_workq.o ---

[thr_workq.o] Error code 1

make[6]: stopped in /scratch/tmp/head/lib/libthr
1 error

make[6]: stopped in /scratch/tmp/head/lib/libthr

lib/libthr__L ---

[lib/libthr__L] Error code 2

make[5]: stopped in /scratch/tmp/head
1 error

make[5]: stopped in /scratch/tmp/head

libraries ---

[libraries] Error code 2

make[4]: stopped in /scratch/tmp/head
1 error

make[4]: stopped in /scratch/tmp/head

It actually needs a complete revision.

Experimental Workqueue supportAbandonedPublicActions

Details

Diff Detail

Event Timeline

> lib/libthr (obj,depend,all,install)

> lib/libc/tests/gen/posix_spawn (all)

> lib/libc/tests/db (all)

> lib/libc/tests/gen (all)

> lib/libthr (obj,all,install)

Revision ContentsChangeset List

Diff 22918

include/Makefile

include/pthread_workqueue.h

lib/libc/sys/Symbol.map

lib/libthr/pthread.map

lib/libthr/thread/Makefile.inc

lib/libthr/thread/thr_init.c

lib/libthr/thread/thr_private.h

lib/libthr/thread/thr_stack.c

lib/libthr/thread/thr_workq.c

share/man/man3/Makefile

share/man/man3/pthread_workqueue_np.3

sys/conf/NOTES

sys/conf/files

sys/conf/options

sys/kern/init_sysent.c

sys/kern/kern_exec.c

sys/kern/kern_exit.c

sys/kern/kern_mutex.c

sys/kern/kern_proc.c

sys/kern/kern_synch.c

sys/kern/kern_thr.c

sys/kern/kern_thread.c

sys/kern/kern_thrworkq.c

sys/kern/p1003_1b.c

sys/kern/syscalls.c

sys/kern/systrace_args.c

sys/sys/proc.h

sys/sys/syscall.h

sys/sys/syscall.mk

sys/sys/syscallsubr.h

sys/sys/sysproto.h

sys/sys/thr.h

sys/sys/thrworkq.h

Experimental Workqueue support
AbandonedPublic
Actions

Revision Contents
Changeset List