Created attachment 188352 [details] [review]
here is a patch

Created attachment 188353 [details] [review]
optimize bitlocks

I really don't like the _exchange_ in the name here since these operations don't exchange anything at all.

I think we're better off calling them one of:

  g_atomic_int_fetch_and_and
  g_atomic_int_get_and_and
  g_atomic_int_and

The last variant has the advantage of not seeing 'and_and' and 'and_or' in function names.

Note that the macro version of g_atomic_int_add is already:

#define g_atomic_int_add(atomic,val) \
  __sync_fetch_and_add((atomic),(val))

so g_atomic_int_add is already 'fetch and add' (at least sometimes...)

implied: we would deprecate g_atomic_int_exchange_and_add

the short names would be ok for me

g_atomic_int_{add,sub,and,or,xor,nand}

I'm not super-keen on deprecating...

g_atomic_int_add already exists, returning void rather than the previous value.

g_atomic_int_exchange_and_and seems to be declared twice in the header.

Also:

gint  g_atomic_int_exchange_and_and (volatile gint G_GNUC_MAY_ALIAS *atomic,
                                     gint               val);

#define g_atomic_int_exchange_and_add(atomic,val) \
  __sync_fetch_and_add((atomic),(val))

I worry a bit about the types here. If you hit the function then your arguments will be auto-converted to the right integer size type. But if you hit the macro it will instead cause a different size be used for the atomic due to the macro magic in the gcc things. I think we should add some manual casts here.

Oh, no i misread and_add for one of the and_and:s :)

Also:

+gint
+gint
+g_atomic_int_exchange_and_nand (volatile gint G_GNUC_MAY_ALIAS *atomic,

gatomic-gcc.c: In function 'g_atomic_int_exchange_and_nand':
gatomic-gcc.c:166:32: note: '__sync_fetch_and_nand' changed semantics in GCC 4.4


Sounds scary. Do we really need nand?

My initial impression was also that 'nand' would not be particularly helpful and that I wouldn't intuitively know how I expected it to work.  Seems that the GCC developers didn't know either...

We should resolve bug 650935 (one way or the other) before committing anything here.

(In reply to comment #7)
> I worry a bit about the types here. If you hit the function then your arguments
> will be auto-converted to the right integer size type. But if you hit the macro
> it will instead cause a different size be used for the atomic due to the macro
> magic in the gcc things. I think we should add some manual casts here.

This is not just a problem with the input types but also the return types.  In the pointer case the atomic operations are used on gsize* and people still expect gpointer to be returned (since they expect that it is a function).

Created attachment 188570 [details] [review]
Add pointer sized atomic ops

These are nice since they can be used for bitlocks and gdatalist flags.

Created attachment 188571 [details] [review]
Implement pointer sized bitlocks

pointer atomics and bitlocks needed for the optimizations in bug 650458

I should not that i'm slightly unsure about the details of accessing some memory atomically at different sizes. I.e. on 64bit arches all updates of the pointer bitlock are using atomic 64bit writes (and reads), but the read in the futex code is 32bit.

Does a 64bit atomic write guarantee atomicity of a 32bit atomic read of the same data?

Random atomic/bitlock question, how does the ARM spinlock protected atomic primitives work wrt the atomic read in the kernel in the futex syscall. The kernel can hardly be using the userspace spinlock...

In fact, some googling around makes me worry that ARM SMP futex support is not really all implemented yet, and we should handle ENOSYS from the futex calls at runtime...

I think making sense of all this (atomics, bitlocks, etc) on arm requires someone with interest in that platform and available hw to jump in...

commit 83821352657a9481dbff6ab04e8ae60566c17d5e
Author: Ryan Lortie <desrt@desrt.ca>
Date:   Sat May 28 15:59:18 2011 -0400

    glib: Rewrite gatomic.[ch]
    
     - remove all inline assembly versions
    
     - implement the atomic operations using either GCC intrinsics, the
       Windows interlocked API or a mutex-based fallback
    
     - drop gatomic-gcc.c since these are now defined in the header file.
       Adjust Makefile.am accordingly.
    
     - expand the set of operations: support 'get', 'set', 'compare and
       exchange', 'add', 'or', and 'xor' for both integers and pointers
    
     - deprecate g_atomic_int_exchange_and_add since g_atomic_int_add (as
       with all the new arithmetic operations) now returns the prior value
    
     - unify the use of macros: all functions are now wrapped in macros that
       perform the proper casts and checks
    
     - remove G_GNUC_MAY_ALIAS use; it was never required for the integer
       operations (since casting between pointers that only vary in
       signedness of the target is explicitly permitted) and we avoid the
       need for the pointer operations by using simple 'void *' instead of
       'gpointer *' (which caused the 'type-punned pointer' warning)
    
     - provide function implementations of g_atomic_int_inc and
       g_atomic_int_dec_and_test: these were strictly macros before
    
     - improve the documentation to make it very clear exactly which types
       of pointers these operations may be used with
    
     - remove a few uses of the now-deprecated g_atomic_int_exchange_and_add
    
     - drop initialisation of gatomic from gthread (by using a GStaticMutex
       instead of a GMutex)
    
     - update glib.symbols and documentation sections files
    
    Closes #650823 and #650935

Review of attachment 188571 [details] [review]:

::: glib/gbitlock.c
@@ +350,3 @@
+    {
+      int_v = pointer_int_part (v, lock_bit);
+      int_address = address_of_int_part (address, lock_bit);

I wonder why we need two 'icky' functions here. Can't we figure out the address first, and then simply do

int_v = *int_address 

?

@@ +386,3 @@
+
+/**
+ * g_pointer_bit_tryunlock:

Obvious copy-paste accident here

Review of attachment 188571 [details] [review]:

::: glib/gbitlock.c
@@ +350,3 @@
+    {
+      int_v = pointer_int_part (v, lock_bit);
+      int_address = address_of_int_part (address, lock_bit);

Eh? But those are not the same. int_v is the old value returned from before the atomic or. *int_address is the new value that futex() checks to see if its the same as the passed in value (i.e. int_v).

I fixed up the copy-paste accident and moved this patch to bug 651467.