>> I think the problem is the cmpxchg8b instruction used in >> counter_u64_fetch(), because this machine instruction always writes to >> memory, also when we only want to read and have (EDX:EAX) = (ECX:EBX): >> >> TEMP64 <- DEST >> IF (EDX:EAX = TEMP64) >> THEN >> ZF <- 1 >> DEST <- ECX:EBX >> ELSE >> ZF <- 0 >> EDX:EAX <- TEMP64 >> DEST <- TEMP64 >> FI >> >> If one CPU increments the counter in pf_create_state() and another does >> the fetch, then both CPU's may run the xmpxschg8b at once with the >> chance that both read the same memory value in TEMP64 and the fetching >> CPU is the second CPU that writes and so the increment is lossed. Thats >> what I see without the above patch two or three times a week. > > Please try the following patch. The idea is to make the value to compare > with unlikely to be equal to the memory content, for fetch_one().
During my research I first had the same idea, but it did not work. In the actual coding eax/edx is not well defined before cmpxchg8b is executed, but it does not help for the problem to do so. > Also it fixes a silly bug in zero_one(). > > diff --git a/sys/i386/include/counter.h b/sys/i386/include/counter.h > index 7fd26d2a960..aa20831ba18 100644 > --- a/sys/i386/include/counter.h > +++ b/sys/i386/include/counter.h > @@ -78,6 +78,9 @@ counter_u64_read_one_8b(uint64_t *p) > uint32_t res_lo, res_high; > > __asm __volatile( > + "movl (%0),%%eax\n\t" > + "movl 4(%0),%%edx\n\t" > + "addl $0x10000000,%%edx\n\t" /* avoid write */ > "movl %%eax,%%ebx\n\t" > "movl %%edx,%%ecx\n\t" > "cmpxchg8b (%2)" We can not avoid the write done by cmpxchg8b as can be seen from the microcode given above, we always end up with "DEST <- TEMP". From the Intel instruction reference manual: The destination operand is written back if the comparision fails. (The processor never produces a locked read without also producing a locked write). Maybe it is enough to prefix the cmpxchg8b with LOCK only in function counter_u64_read_one_8b(). > @@ -120,11 +123,11 @@ counter_u64_zero_one_8b(uint64_t *p) > { > __asm __volatile( > +"\n1:\n\t" > "movl (%0),%%eax\n\t" > - "movl 4(%0),%%edx\n" > + "movl 4(%0),%%edx\n\t" > "xorl %%ebx,%%ebx\n\t" > "xorl %%ecx,%%ecx\n\t" > -"1:\n\t" > "cmpxchg8b (%0)\n\t" > "jnz 1b" > : If jnz jumps back the instruction cmpxchg8b has load registers eax/edx with (%0), therefor I do not understand the silly bug. Andreas _______________________________________________ freebsd-pf@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-pf To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"