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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-amd64
To unsubscribe, send any mail to "[email protected]"
