Date: Fri, 20 Jul 2012 03:44:02 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Mark Linimon <linimon@lonesome.com>, freebsd-amd64@FreeBSD.org Subject: Re: amd64/169927: siginfo, si_code for fpe errors when error occurs using the SSE math processor Message-ID: <20120720032018.Q3406@besplex.bde.org> In-Reply-To: <20120719123138.GG2676@deviant.kiev.zoral.com.ua> References: <201207172150.q6HLoFvB096742@freefall.freebsd.org> <20120718103210.E834@besplex.bde.org> <20120718035220.GO2676@deviant.kiev.zoral.com.ua> <20120718140523.X1862@besplex.bde.org> <20120718050958.GQ2676@deviant.kiev.zoral.com.ua> <20120718153554.O2195@besplex.bde.org> <20120718081003.GT2676@deviant.kiev.zoral.com.ua> <20120719111846.G780@besplex.bde.org> <20120719123138.GG2676@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 19 Jul 2012, Konstantin Belousov wrote:
> On Thu, Jul 19, 2012 at 01:10:29PM +1000, Bruce Evans wrote:
>> On Wed, 18 Jul 2012, Konstantin Belousov wrote:
>>
>>> On Wed, Jul 18, 2012 at 04:59:38PM +1000, Bruce Evans wrote:
>>>> On Wed, 18 Jul 2012, Konstantin Belousov wrote:
>>>> Putting the definiton in machine/pcpu.h should avoid changing the
>>>> prerequistes for machine/pcb.h.
>>>>
>>>>> #ifndef _KERNEL
>>>>> /* stuff that *used* to be included by user.h, or is now needed */
>>>>>
>>>>> Please note the location in pcb.h an not in machine/pcpu.h, where it
>>>>> cannot work for technical reasons (struct pcpu is not defined yet).
>>>>
>>>> Not applicable -- see above.
>>> No, this cannot work. machine/pcpu.h defines PCPU_MD_FIELDS which is used
>>> to provide md part of the struct pcpu.
>>
>> I see. Oops.
>>
>> But this problem is easy to avoid. There are at least the following
>> alternatives:
>> (1) hard-code the offset, as for curthread. This is ugly, but not too
>> hard to maintain. The offset has been 4 * sizeof(struct thread *),
>> for more than 10 years, and you can't change this without breaking
>> the ABI.
> I did this, adding CTASSERTs. See the patch below.
Looks good. I had forgotten that CTASSERT() makes this safe.
>> To do the same thing for curpcb, provide a PCPU_NONVOLATILE_GET()
>> which is the same as PCPU_GET() without the volatile in the asm,
>> and #define curpcb as PCPU_NONVOLATILE_GET(curpcb). Or maybe
>> start with the inline function used for curthread, and macroize it.
>>
>> This could be used for curthread too, but I think we want to keep it
>> as an inline function since the macro expansions are still horrible.
> I do not like this, prefer the inline functions approach.
Maybe use even more inlines. I wonder if an inline that is always parsed
but rarely used takes longer to compile than a complicated macro that is
often but not always expanded (where each expansion gives an asm similar
to the inline).
>> BTW, can you explain the locking for the pcpu access functions? It seems
>> to be quite broken, with the volatile in the asms neither necessary nor
>> sufficient. PCPU_PTR() seems to be almost unusable, but it is used.
>> ...
>> - [curthread and curpcb are special]
>> as obviously correctly locked as the above 2.
>> - counter variables. Now you want the %fs pointer to change to track
>> the CPU.
> They are 'locked' by the fact that the increment is atomic regarind
> context switches and interrupts, since CPU guarantees that interrupts
> only happen on exact instruction boundaries.
Also, PCPU_INC() only supports 1, 2 and 4-byte variables on i386. But
I increments of int64_t variables using PCPU_PTR() (or DPCPU_PTR()?)
aren't safe on i386.
>> % } else { \
>> % __res = *__PCPU_PTR(name); \
>>
>> __PCPU_PTR() seems to be almost unusable, and this is not one of the
>> few cases where it is usable. Here it is used for wide or unusually-sized
>> fields where atomic access is especially problematic, yet it does even
>> less that the above to give atomicity. First consider using it in all
>> cases to replace the above:
>> ...
>> - PCPU_GET/SET(switchtime) in kern_resource.c. The accesses are only
>> proc locked AFAIK. switchtime is uint64_t, so accesses to it are
>> non-atomic on 32-bit arches.
> Note that access to switchtime is covered by the process spinlock.
> Spinlocks disable preemption, so this is fine.
I see. It used to use sched locking, but was optimized.
> Below is the updated patch, with added assert that offset of pc_curthread
> is 0.
> diff --git a/sys/amd64/amd64/vm_machdep.c b/sys/amd64/amd64/vm_machdep.c
> index 103fa0d..070d8c9 100644
> --- a/sys/amd64/amd64/vm_machdep.c
> +++ b/sys/amd64/amd64/vm_machdep.c
> @@ -90,6 +90,10 @@ static u_int cpu_reset_proxyid;
> static volatile u_int cpu_reset_proxy_active;
> #endif
>
> +CTASSERT((struct thread **)OFFSETOF_CURTHREAD ==
> + &((struct pcpu *)NULL)->pc_curthread);
> +CTASSERT((struct pcb **)OFFSETOF_CURPCB == &((struct pcpu *)NULL)->pc_curpcb);
> +
Hmm, I was hoping that the CTASSERT() would be in a header, closer to
the code. Unfortunately, it can't go very close, for the same reason
that we can't just use struct pcpu in machine/pcpu.h. This style seems
to be unpopular -- in <sys> headers, the only CTASSERTs() are in old
disk headers and serial.h.
They should use __offsetof() instead of a home made one.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120720032018.Q3406>