Date: Tue, 2 Sep 2014 18:41:27 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r270850 - in head/sys: i386/i386 i386/include i386/isa x86/acpica Message-ID: <20140902154127.GD2737@kib.kiev.ua> In-Reply-To: <201409021100.57493.jhb@freebsd.org> References: <201408301748.s7UHmc6H059701@svn.freebsd.org> <20140830195809.GS2737@kib.kiev.ua> <201409021100.57493.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--I2bJ1vO3Y/RENlXz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 02, 2014 at 11:00:57AM -0400, John Baldwin wrote: > On Saturday, August 30, 2014 3:58:09 pm Konstantin Belousov wrote: > > On Sat, Aug 30, 2014 at 05:48:38PM +0000, John Baldwin wrote: > > > Author: jhb > > > Date: Sat Aug 30 17:48:38 2014 > > > New Revision: 270850 > > > URL: http://svnweb.freebsd.org/changeset/base/270850 > > >=20 > > > Log: > > > Save and restore FPU state across suspend and resume. In earlier= =20 > revisions > > > of this patch, resumectx() called npxresume() directly, but that do= esn't > > > work because resumectx() runs with a non-standard %cs selector. =20 > Instead, > > > all of the FPU suspend/resume handling is done in C. > > > =20 > > > MFC after: 1 week > > >=20 > > > Modified: > > > head/sys/i386/i386/mp_machdep.c > > > head/sys/i386/i386/swtch.s > > > head/sys/i386/include/npx.h > > > head/sys/i386/include/pcb.h > > > head/sys/i386/isa/npx.c > > > head/sys/x86/acpica/acpi_wakeup.c > > >=20 > > > Modified: head/sys/i386/i386/mp_machdep.c > > >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > > --- head/sys/i386/i386/mp_machdep.c Sat Aug 30 17:39:28 2014 (r270849) > > > +++ head/sys/i386/i386/mp_machdep.c Sat Aug 30 17:48:38 2014 (r270850) > > > @@ -1522,9 +1522,15 @@ cpususpend_handler(void) > > > =20 > > > cpu =3D PCPU_GET(cpuid); > > > if (savectx(susppcbs[cpu])) { > > > +#ifdef DEV_NPX > > > + npxsuspend(&suspcbs[cpu]->pcb_fpususpend); > > > +#endif > > > wbinvd(); > > > CPU_SET_ATOMIC(cpu, &suspended_cpus); > > > } else { > > > +#ifdef DEV_NPX > > > + npxresume(&suspcbs[cpu]->pcb_fpususpend); > > > +#endif > > > pmap_init_pat(); > > > PCPU_SET(switchtime, 0); > > > PCPU_SET(switchticks, ticks); > > >=20 > > > Modified: head/sys/i386/i386/swtch.s > > >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > > --- head/sys/i386/i386/swtch.s Sat Aug 30 17:39:28 2014 (r270849) > > > +++ head/sys/i386/i386/swtch.s Sat Aug 30 17:48:38 2014 (r270850) > > > @@ -416,45 +416,6 @@ ENTRY(savectx) > > > sldt PCB_LDT(%ecx) > > > str PCB_TR(%ecx) > > > =20 > > > -#ifdef DEV_NPX > > > - /* > > > - * If fpcurthread =3D=3D NULL, then the npx h/w state is irrelevant= and=20 > the > > > - * state had better already be in the pcb. This is true for forks > > > - * but not for dumps (the old book-keeping with FP flags in the pcb > > > - * always lost for dumps because the dump pcb has 0 flags). > > > - * > > > - * If fpcurthread !=3D NULL, then we have to save the npx h/w state= to > > > - * fpcurthread's pcb and copy it to the requested pcb, or save to t= he > > > - * requested pcb and reload. Copying is easier because we would > > > - * have to handle h/w bugs for reloading. We used to lose the > > > - * parent's npx state for forks by forgetting to reload. > > > - */ > > > - pushfl > > > - CLI > > > - movl PCPU(FPCURTHREAD),%eax > > > - testl %eax,%eax > > > - je 1f > > > - > > > - pushl %ecx > > > - movl TD_PCB(%eax),%eax > > > - movl PCB_SAVEFPU(%eax),%eax > > > - pushl %eax > > > - pushl %eax > > > - call npxsave > > > - addl $4,%esp > > > - popl %eax > > > - popl %ecx > > > - > > > - pushl $PCB_SAVEFPU_SIZE > > > - leal PCB_USERFPU(%ecx),%ecx > > > - pushl %ecx > > > - pushl %eax > > > - call bcopy > > > - addl $12,%esp > > > -1: > > > - popfl > > > -#endif /* DEV_NPX */ > > > - > > > movl $1,%eax > > > ret > > > END(savectx) > > > @@ -519,10 +480,6 @@ ENTRY(resumectx) > > > movl PCB_DR7(%ecx),%eax > > > movl %eax,%dr7 > > > =20 > > > -#ifdef DEV_NPX > > > - /* XXX FIX ME */ > > > -#endif > > > - > > > /* Restore other registers */ > > > movl PCB_EDI(%ecx),%edi > > > movl PCB_ESI(%ecx),%esi > > >=20 > > > Modified: head/sys/i386/include/npx.h > > >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > > --- head/sys/i386/include/npx.h Sat Aug 30 17:39:28 2014 (r270849) > > > +++ head/sys/i386/include/npx.h Sat Aug 30 17:48:38 2014 (r270850) > > > @@ -53,8 +53,10 @@ void npxexit(struct thread *td); > > > int npxformat(void); > > > int npxgetregs(struct thread *td); > > > void npxinit(void); > > > +void npxresume(union savefpu *addr); > > > void npxsave(union savefpu *addr); > > > void npxsetregs(struct thread *td, union savefpu *addr); > > > +void npxsuspend(union savefpu *addr); > > > int npxtrap_x87(void); > > > int npxtrap_sse(void); > > > void npxuserinited(struct thread *); > > >=20 > > > Modified: head/sys/i386/include/pcb.h > > >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > > --- head/sys/i386/include/pcb.h Sat Aug 30 17:39:28 2014 (r270849) > > > +++ head/sys/i386/include/pcb.h Sat Aug 30 17:48:38 2014 (r270850) > > > @@ -90,6 +90,8 @@ struct pcb { > > > struct region_descriptor pcb_idt; > > > uint16_t pcb_ldt; > > > uint16_t pcb_tr; > > > + > > > + union savefpu pcb_fpususpend; > > > }; > > Now pcb consumes 512 bytes from each thread' kernel stack, which mostly > > stay unused. > >=20 > > Amd64 only stores the pointer to the fpususpend context in pcb, and > > acpu_wakeup() allocates the memory as needed. Even this is a waste of 8 > > bytes which are not needed for normal kernel operations. > >=20 > > Suspend FPU context, as well as amd64 MSRs should go out of pcb into > > some per-cpu suspend data block. >=20 > I thought about that. I could easily make a parallel array, or perhaps u= se a=20 > separate 'susppcb' structure that includes a pcb and the savefpu union and > change susppcbs to be an array of those. Which do you prefer? If we want > to move some state out of the PCB on amd64 into this, then a separate str= uct > for susppcbs might be the sanest. Yes, separate structure seems to be a way forward. FWIW, I do not understand the need for pcb in its current form, allocated on the thread kernel stack, at all. It looks like a vestige of the u-area, but serves no real purpose except to consume now precious stack space. The idea of the part of the thread state that can be swapped out, together with the stack, seems to become alien. Most of the thread state which is not needed when the thread is not runnable, now goes to struct thread anyway. Might be, we should move the pcb into td_md. People actively object to the idea of the swappable kernel stack when they learn hard that local vars cannot participate in the global lists. The only thing which is currently allocated below the pcb and which seems to be reasonable to swap out, is the FPU context on amd64. --I2bJ1vO3Y/RENlXz Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUBeUmAAoJEJDCuSvBvK1BRY8P/jJrYZlapubg7Gq/IcOlHD30 j8wk2GUR09kK+97FlpZ84SFcuQTNjguI9ygaDXbjYxmEcEGcV2WJFFIBA5MPpi2R D4MCYtHwdYHAdQ3umKBRWi/Jnx1Ctpq69pDYlzbMd39EnBfNR9MJbN5AHJ95Z7Kq Hm9TWYVIXIvi6eTcwz4q9agbbcn6S3UqUGQYbOQ5u8fUcbuzxhmxx8j/d9F8xg1H 3myJ1N8A2gf5b5Ux2+bLlE8+V5SsHutNj/2fcIVEppSAVBNxlhYBkZKcuZw9TgQL y4F73p3y53ppfaK+cyClwbnV35qH5MxOgWqAEusVr2DvLMNfQbWOnkUkXTG4pKIL BPPaaJAOUqEbdU+fE5EbKSoHKPY5fD+gYD70Eyml5G6H72HzJYvzLgpfIj4M2s0X v9MWAJvdo1aHRuu2D54A00fsmBUp9CVKp2cyfOZChJTJhnlTTcP7PgFCoDNG6MKt Q7ukiXjzl+B+c0aeqxkn4vy7RTR5K4drkbyqlExg7lrYjhryrt+k/LWJALIMVQnX qMjylHOvtQ0vxroZb0Goh8Mreovr7fKtxJfQsyaAiBTU8H3pKsQNm7F70Eq6LYBR wncIIpiHdM+xLNpo+bPA2SYpsxNOAGkqJ4s1+H0kpxf4bFQc8bm8dmofsVwoJQeg EhOErA1bkAZE7SZbfvM3 =fmsT -----END PGP SIGNATURE----- --I2bJ1vO3Y/RENlXz--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140902154127.GD2737>