Re: GAtomic int finals (#63621)

[Sebastian Wilhelmi <seppi seppi de>]

Hi Tim,

> on the technical side, i have one reamining issue. there're a couple
> discussions on the net that seem to suggest we won't be able to ignore
> memory barrier issues forever in the API we introduce:
> 
> http://www.ussg.iu.edu/hypermail/linux/kernel/0110.1/0725.html
>         (alpha can reorder and coalesce read and write requests through
>         several cache layers)
> http://www.ussg.iu.edu/hypermail/linux/kernel/0110.1/0737.html
>         (linus on needing read memory barriers)

Yeah, sure, this is all quite complicated and non-obvious.

> so if we run into issues due to lack of memory barriers, there're two options:
> 1) we add rmb/wmb/mb to the API and expect people to use them appropriately

I don't think, that would be good idea. It will only confuse people...

> 2) we add the barriers to out atomic primitives

That has already happened. E.g. on alpha has the "mb" assembler memory
barrier instruction before and after the functions. However, pretty much
all you are allowed to do after reading a pointer, that is concurrently
accessed by g_atomic_pointer_compare_and_swap, is using is as an oldval
argument to g_atomic_pointer_compare_and_swap.

Thinking about it (I always seem to do it quite late) that means, we
should either replace g_atomic_pointer_compare_and_swap by
g_atomic_pointer_swap (atomic, newval), which returns oldval, or adding
memory barrier protected getters.

> (1) is unlikely to work out properly (complexity, portability, hard to debug,
> requires knowledge about multiple architectures),
> (2) requires an ordinary getter that is used exclusively for access, so
> an rmb can be added if required by a platform.

Ah, I see, you get to the same conclusion. Actually it might be wise to
export a memory barrier (a full one, further optimizing in read/write
seems to be too much) for glib's internal use. We should however not
export it. That would allow to replace the current lightly crude "double
checked locking" check in configure.in by proper memory barriers in
gthread.c.

> since a getter can be a simple inlined read, it doesn't cost us anything
> now (except API and mandatory use), so i strongly opt for adding it.

Ok, new patch on the way.

> on to the API names.

Now it gets interesting. You can't argue taste.... 

> > I've renamed g_atomic_int_exchange_and_add to
> > g_atomic_int_get_and_add. It's shorter and adheres more to the GLib
> > naming scheme and is more correct (Nothing is exchanged here).
> 
> i don't think g_atomic_int_get_and_add() is an improvement. exchange_and_add()
> is a pretty established name, we should stick with that. and in contrast to
> get_and_add(), it tells me that the value returned is the int *before* the
> add, not after. (so in effect, *i* do see what's exchanged here ;).

Doesn't sound very convincing. We need an arbiter. Let Owen decide.

> the API involved with atomic operations should be kept as small as possible,
> that way it stays simple and avoids confusion about what primitives to use.
> that is, we shouldn't provide g_atomic_int_inc() and g_atomic_int_dec_and_test().

I would really like to keep both, even if they are one-liners. They make
the reference counting code easier to read.

> with the above 3 issues sorted out, the patch is ready to go into CVS, making
> way for a variety of other glib improvements/optimizations ;)

Making way for convoluted ways to shoot in our feet.

Bye,
Sebastian
-- 
Sebastian Wilhelmi                 |            här ovanför alla molnen
mailto:seppi seppi de              |     är himmlen så förunderligt blå
http://seppi.de                    |