Matthew Wilcox wrote:
The original code and your rewrite both access memory twice in the loop. Why don't we do it with one memory reference per loop instead? { CMPXCHG_BUGCHECK_DECL u32 *m = (u32 *)addr + (nr >> 5); u32 bit = 1 << (nr & 31); u32 old = *m; while (!(old & bit)) { u32 new = old | bit; u32 prev = cmpxchg_acq(m, old, new); CMPXCHG_BUGCHECK(m); if (prev == old) return 1; old = prev; } return 0; } Looking at the disassembly of grab_block() in fs/ext2/balloc.c, I don't see much difference. The ld4.acq turns into a regular ld4 (because 'm' is no longer tagged as volatile), and is hoisted out of the loop. Interestingly, gcc chooses to reorder the tests, and make the loop four bundles long instead of three, but will 'goto repeat' in two bundles instead of four. Using likely()/unlikely() doesn't persuade gcc to change the order of the two branches, so I assume it actually is better to do it this way.
I like this code with the following slight modifications: - let's keep "m" as pointer to volatile - let's keep on using "__u32" types - not sure we need "new" - return the old bit volatile __u32 *m = (volatile __u32 *)addr + (nr >> 5); __u32 bit = 1 << (nr & 31); __u32 old = *m; while (!(old & bit)) { __u32 prev = cmpxchg_acq(m, old, old | bit); CMPXCHG_BUGCHECK(m); if (prev == old) return 0; old = prev; } return 1; It compiles to: 0: and r15=31,r32 6: mov r14=1 c: extr r32=r32,5,27;; 10: shladd r18=r32,2,r33;; 16: ld4.acq r16=[r18] 1c: shl r17=r14,r15;; 20: and r14=r17,r16;; 26: nop.m 0x0 2c: cmp4.eq p7,p6=0,r14 30: nop.b 0x0 36: nop.b 0x0 3c: (p06) br.cond.dpnt.few a0 <+0xa0> 40: nop.m 0x0 46: zxt4 r14=r16 4c: nop.b 0x0;; 50: mov.m ar.ccv=r14;; 56: nop.m 0x0 5c: or r14=r17,r16 60: nop.m 0x0;; 66: cmpxchg4.acq r14=[r18],r14,ar.ccv 6c: nop.i 0x0;; 70: cmp4.eq p7,p6=r14,r16 76: nop.f 0x0 7c: and r15=r17,r14 80: mov r16=r14 86: nop.f 0x0 8c: (p07) br.cond.dpnt.few b0 <+0xb0>;; 90: nop.m 0x0 96: cmp4.eq p7,p6=0,r15 9c: (p07) br.cond.dptk.few 40 <+0x40> a0: nop.m 0x0 a6: mov r8=1 ac: br.ret.sptk.many b0;; b0: nop.m 0x0 b6: mov r8=r0 bc: br.ret.sptk.many b0;; It seems to be o.k., thanks. Zoltán Menyhárt - To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |