Re: next round: glib thread safety proposal

[Owen Taylor <otaylor redhat com>]

Sebastian Wilhelmi <wilhelmi@ira.uka.de> writes:

> Hi, Owen
> 
> > Re-reading the thread, I see, that the problem is a little
> > different than I presented it above. Essentially, it is:
> > 
> >  1) Thread B reads in some other data, which happens to
> >     be in the same cache line as the contents of the mutex.
> > 
> >  2) Thread A writes the contents of the mutex and the
> >     pointer to the mutex.
> > 
> >  3) Thread B reads in the pointer to the mutex, and uses
> >     it to access the stale cache data for the contents
> >     of the mutex.
> > 
> > This is possible for memory models that allow out-of-order
> > reads.
> 
> ok, I read the thread and if I got it right, it basically said, there
> might be SMP platforms without hardware cache coherence, that might fail
> to work correct for the above. (but nobody actually  named such, apart
> from virtual shared mem systems). So I made it lock the mutex everytime. 

Yeah, I don't know how much of a real problem this is. 
Reading through the Intel hardware specs for MP machines,
it looks like it can't happen there. (Assumming the processor,
as opposed to the cache, never makes speculative reads for
"random" bits of memory)

> But: It is highly desirable to have those StaticMutex'es and to have them
> as fast as possible (because they will most likely be used for glib's
> internal locking, see below).

> So I changed the things to actually use the
> default implementations mutexes statically, and fall back to the 'lock
> everytime and check for existence' method, if a nondefault GMutexFunctions
> is passed to g_thread_init(). (Look at the code in glib.h and the
> generated glibconfig.h, as I can't describe it better.)

I don't quite understand what your code does - you have:

typedef struct _GStaticMutex GStaticMutex;
struct _GStaticMutex 
{
  $g_mutex_default_type default_mutex;
  struct _GMutex* runtime_mutex;
};

Why do we need two mutexes?

We could also do without static mutex altogether for the
purposes of GLIB. We could just make all the mutex global
and initialize them in something called from
g_thread_init. Yes, that's a bit ugly.

> As for the patch, that I'll post to
> ftp://ftp.gtk.org/incoming/glib-wilhelmi-981130-0.patch.*
> 
> Here's the README:
> 
> Hi, 
> 
> This patch consists of two files glib-wilhelmi-981130-0.patch.gz and
> glib-wilhelmi-981130-0.patch.tar.gz, that should be both applied/unpacked
> to the CVS glib.
> 
> this patch aims for laying the grounding for making glib thread-safe.
> Please see the thread: "next round: glib thread safety proposal" on
> gtk-devel-list for rationals and the like. 
> 
> Contained is:
>   - implementations for mutexes and conditions. They can be adjusted at
>     run-time by calling g_thread_init()
>   - implemenations of static mutexes, that can be initialized statically.
>   - convenience macros for using static mutexes, for the usage look at
>     garray.c (Owen: they are of course only syntactic sugar, but nice to
>     use, what do you think)

The g_lock() stuff? Well, something like g_lock_define which
doesn't have the semantics of a function call should definitely
be all uppercase. Other than that 

>   - a whole lot of configure stuff to get things right.
>   - a first shot at making glib threadsafe. I made the files garray.c,
>     gcache.c, gdataset.c, ghash.c and glist.c reentrant. (Owen: does that
>     look sane???, If mutexes are in glib, I'll look at making all files
>     reentrant, but will not commit until I asked the main maintainer of

Yeah, this looks pretty much OK. The list stuff is where the
locks are going to really hurt performance, but doing
better would probably mean using per-thread free lists.

Regards,
                                        Owen