amd64/169927: siginfo, si_code for fpe errors when error occurs using the SSE math processor

[Bruce Evans]

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