Date: Thu, 27 Mar 2014 14:07:20 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Chris Torek <torek@torek.net> Cc: freebsd-hackers@freebsd.org Subject: Re: slight problem with r254457 (kthread_add fix) Message-ID: <20140327120720.GN21331@kib.kiev.ua> In-Reply-To: <201403270902.s2R92nPo064876@elf.torek.net> References: <201403270902.s2R92nPo064876@elf.torek.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--dngyMJhgXGAL5Gb8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 27, 2014 at 03:02:49AM -0600, Chris Torek wrote: > This fix: >=20 > http://lists.freebsd.org/pipermail/svn-src-head/2013-August/050550.html >=20 > has introduced a bit of a problem with kernel threads that enable > the FPU, on amd64. >=20 > Before the fix, a new thread added to proc0 (when kthread_add() is > called with p =3D=3D NULL) was essentially "forked from" thread0: >=20 > if (p =3D=3D NULL) { > p =3D &proc0; > oldtd =3D &thread0; > } >=20 > Now, each such new thread "forks from" the *newest* thread in proc0: >=20 > if (p =3D=3D NULL) > p =3D &proc0; > ... > PROC_LOCK(p); > oldtd =3D FIRST_THREAD_IN_PROC(p); >=20 > The problem is that "first thread in proc" is really the *last* > (i.e., most recent) thread in the proc, as thread_link() does a > TAILQ_INSERT_HEAD() on p->p_threads, and FIRST_THREAD_IN_PROC > takes the TAILQ_FIRST element. >=20 > What this means is that if something enables the FPU (e.g., a > device creates a taskq thread and enables the FPU in that thread, > so that the device can use XMM registers), every subsequent taskq > also has the FPU enabled. (We found this by unconditionally > enabling the extensions in various threads, assuming that new ones > did not have FPU enabled for kernel use yet. That worked until > we picked up this particular change.) >=20 > The fix itself is fine but "forking from" the most-recent kernel > thread, rather than thread0, for this case seems wrong. I see > three obvious ways to fix *that*: >=20 > - Restore the old behavior: >=20 > if (p =3D=3D NULL) { > p =3D &proc0; > oldtd =3D &thread0; > } else > oldtd =3D NULL; > ... > PROC_LOCK(p); > if (oldtd =3D=3D NULL) > oldtd =3D FIRST_THREAD_IN_PROC(p); >=20 > Conservative, will definitely work, but still seems a bit odd > behavior-wise: a new "not in a proc" kthread clones from thread0, > but a new "is in a proc" kthread clones from most-recent-thread > in that kernel proc. (But that's what this did before the fix, > modulo the bug where it cloned from a dead thread...) >=20 > - Pick the *last* (i.e., earliest still-alive) thread in the proc: >=20 > PROC_LOCK(p); > oldtd =3D LAST_THREAD_IN_PROC(p); >=20 > where LAST_THREAD_IN_PROC uses the tail entry on the tailq > (pretty straightforward). >=20 > - Get rid of FIRST_THREAD_IN_PROC entirely, as it's not clear > what "first" means (at least it was not to me: I assumed at > first that it meant the *oldest*, i.e., first-created, one): > add new macro accessors NEWEST_THREAD_IN_PROC, > OLDEST_THREAD_IN_PROC, and (for when you don't care about > order) SOME_THREAD_IN_PROC [%], and start using those where > appropriate. The thread_link() routine then puts "newest" and > "oldest" at whichever end of the tailq is best for kernel code > space/speed, and only it and the macros need to agree. >=20 > [% Needs better name. Perhaps just THREAD_IN_PROC?] >=20 > I actually like the last option best, but it is the largest > overall change and it messes with all kinds of other kernel code > (there are 62 occurrences of FIRST_THREAD_IN_PROC in a count I > just ran). However, it becomes clearer which thread is really > wanted. >=20 > (I'm going to do the first fix, for now at least, in our kernel.) I agree with the problem statement, but disagree with the proposed 'fix'. I.e., I agree that on the creation of a new kernel thread, the current thread FPU grab state must not leak into the created thread. In fact, cpu_set_upcall() already almost handles this correctly, by resetting the pcb_save pointer back to the user save area. What is not done is the clearing of PCB_KERNFPU flag, which seems to be the issue you met. Am I right ? If yes, then the following patch should handle the problem, hopefully. diff --git a/sys/amd64/amd64/vm_machdep.c b/sys/amd64/amd64/vm_machdep.c index 335a9c9..bc7b311 100644 --- a/sys/amd64/amd64/vm_machdep.c +++ b/sys/amd64/amd64/vm_machdep.c @@ -446,7 +446,8 @@ cpu_set_upcall(struct thread *td, struct thread *td0) * values here. */ bcopy(td0->td_pcb, pcb2, sizeof(*pcb2)); - clear_pcb_flags(pcb2, PCB_FPUINITDONE | PCB_USERFPUINITDONE); + clear_pcb_flags(pcb2, PCB_FPUINITDONE | PCB_USERFPUINITDONE | + PCB_KERNFPU); pcb2->pcb_save =3D get_pcb_user_save_pcb(pcb2); bcopy(get_pcb_user_save_td(td0), pcb2->pcb_save, cpu_max_ext_state_size); diff --git a/sys/i386/i386/vm_machdep.c b/sys/i386/i386/vm_machdep.c index ba61bdf..3562954 100644 --- a/sys/i386/i386/vm_machdep.c +++ b/sys/i386/i386/vm_machdep.c @@ -457,7 +457,8 @@ cpu_set_upcall(struct thread *td, struct thread *td0) * values here. */ bcopy(td0->td_pcb, pcb2, sizeof(*pcb2)); - pcb2->pcb_flags &=3D ~(PCB_NPXINITDONE | PCB_NPXUSERINITDONE); + pcb2->pcb_flags &=3D ~(PCB_NPXINITDONE | PCB_NPXUSERINITDONE | + PCB_KERNNPX); pcb2->pcb_save =3D &pcb2->pcb_user_save; =20 /* --dngyMJhgXGAL5Gb8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQIcBAEBAgAGBQJTNBR3AAoJEJDCuSvBvK1BpwUP/3W4HV8fedrekWaaMPH8wHQ/ D++VO5csIj6tPlYV5VcmTO89zapDgUq9I5YHM4qIHbA/fgidqafu9JN/ugkl6EBh 7uaRKPzUL9mk/jTKRlGmzuKcKQwHt/hqJZyFbOxF8I9NlwEv3PRgXnnXNi+/0XRu k6cIj5c5gjQdinHhM74HFrC7MJkJciwz6s3dG+GEq9qebvFD9OJHPBdNLsvgws/q F6KyWUhinuGbvMMS87w4NPKXB9/5AGqvpHo5lSJWKXSUSurXQxpF4A8tuGk7srk8 xqo/2eWZiwVFRcHm9Ol2C7q/5HPHz2TPuTAHbrN2hHD0byWPAsbgNKaYlWTIq2ks y4Rgfb50LYhGajPFLZ3/mM46rxw7ujWPPuCDr4mAn5ztpUC5Phzn3Lbqf49Jpokz n/cFJBc6tOWMsqsm7y3aakdphIAWpT30KCFSHvhLGgYuetosbAR9tsnzJN+nmHSI +udvWWQ82GPbbxL9BU7ULWpMR4SrAG9Na+KccOLMukT80wpL1aUj6JtfhkcnftK4 DftDhNx1ysm7FGRQDPqvsn0AqlqOwoH5awoMXaWajArjYd+ecVVOgzxLQ2hzJcPJ 0LwvwI/sToZjXALm7eZWDCDjvxy/4g9YOwbG+NJaIqEXswndxNVFdyTnpgA5mjmt 1ESeauS1zDHZj//FDN5K =gUZi -----END PGP SIGNATURE----- --dngyMJhgXGAL5Gb8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140327120720.GN21331>