[gst-devel] [PATCH] SMP race condition in gstatomic.h

[Jim Thornton]

Wow, great catch!

I have been chasing around a problem for a couple of days that could very
well be the same one: it shows up either as the assertion failure attempting
to unref a buffer although I have also seen cases of other assertion
failures and even SEGV.  In all cases the root problem appears to be a
buffer that has refcount of 0 and NULL data pointer where the buffer should
be fine.

In looking at what you found, the SWAP function/macros only come into play
in gstmemchunk.c which is far removed from the assertion failures.  I'm
finding the code in there rather difficult to follow.  Could you possibly
explain to me how the SWAP problem is causing things to get messed up?

Thanks, it will be a tremendous relief to put this bug to bed, and I'm not
sure how long it would have taken us to find what you discovered...

Jim

> -----Original Message-----
> From: gstreamer-devel-admin at lists.sourceforge.net
> [mailto:gstreamer-devel-admin at lists.sourceforge.net]On Behalf Of Cameron
> Hutchison
> Sent: Thursday, September 05, 2002 1:04 AM
> To: gstreamer developer list
> Subject: [gst-devel] [PATCH] SMP race condition in gstatomic.h
>
>
> I've been banging my head against a bug in gstreamer for the last few
> days (bug #92261). This bug can be triggered by the command:
> $ gst-launch sinesrc ! { queue ! fakesink }
>
> After a random number of iterations an assertion is violated when trying
> to unref a buffer.
>
> The problem is that the SWAP functions/macros in gstatomic.h are not
> being compiled for SMP. That header uses the __SMP__ conditional macro,
> but nothing sets that macro.
>
> Following the method used in <asm/atomic.h>, I've changed __SMP__ to
> CONFIG_SMP (defined in <linux/config.h>). This fixes the problem
> ("lock ;" now gets included in the asm code).
>
> One thing to note about this is that all gstreamer code will be compiled
> for SMP machines anyway, since <linux/config.h> is part of glibc (it
> should never be a symlink to the kernel source), and config.h defines
> CONFIG_SMP to 1. This is as it should be, otherwise binaries built on a
> UP machine will fail to run on a SMP machine. This leads me to believe
> that the part of gstatomic.h that is conditional on __SMP__ (now
> CONFIG_SMP) should be removed and the SMP case should be there for all
> builds. The only reason for the CONFIG_SMP conditional in <asm/atomic.h>
> is that kernels are built for UP or SMP. This should not be the case for
> userspace applications. All applications should be built to work on SMP
> and UP without needing a recompile. Comments?
>
> This should close bug #92261 which I reported a few days ago. Should I
> close this myself or will a core member do it?
>
> BTW. __SMP__ is also used in two tests:
>   tests/bufspeed/gstmempool.c
>   tests/memchunk/gstmemchunk.c
>
> These should probably be changed to make the SMP case the only case.
>
> Patch attached...
>
> PS. Wouldn't you know it, I've just got some work for the next month
> starting tomorrow - there goes most of my time I planned to use to muck
> around with gstreamer.
>
>