[vdr] Re: vdr-1.3.17: thread issues with vdr-xine andvdr-osdteletext

[pasi.juppo@iki.fi]

On 19 Dec, Rainer Zocholl wrote:
> Klaus.Schmidinger@cadsoft.de(Klaus Schmidinger)  19.12.04 11:56
> 
>>Reinhard Nissl wrote:
>>> [ ... fixing thread handling ... ]
> 
>>Somehow this is getting a little too complicated for my taste.
>>There's way too much locking and condvar stuff being introduced.
> 
>>I suggest the attached patch against thread.[hc] of VDR 1.3.17.
>>The main changes here are that the child thread no longer
>>accesses childTid at all (it calls pthread_self() directly),
> 
>>and the running state is stored in a simple boolean variable,
>>which requires no extensive locking since it is read/written
>>atomically. 
> 
> You can't "read/write" anything "atomically" 
> with simple C-Code in a mutlitthreaded environment.
> There are CPU specific assembly commands required like
> "lock: CMPXCHG AX,memptr" which are really "atomic",
> but gcc does not allow direct access AFAIK (see atomic_store_rel etc.)
> Too it would be reinventing the wheel, as the thread library
> already offers all required stuff, tested in deep.
> 
> Some examples of real "atomic operations" you can find for example
> here
> http://lists.ximian.com/archives/public/mono-patches/2004-March/032172.html
> 
> Something like:
> 
> __ASM_GLOBAL_FUNC(interlocked_cmpxchg,
>                   "movl 12(%esp),%eax\n\t"
>                   "movl 8(%esp),%ecx\n\t"
>                   "movl 4(%esp),%edx\n\t"
>                   "lock; cmpxchgl %ecx,(%edx)\n\t"
>                   "ret");
> 
> 
> is "atomic", but:

Not sure about x86 environment but denying interrupts for a while would
allow atomic operation but I believe this is high prohibited method when
writing code for consumer OSes such as Linux. In embedded SW this is
sometimes allowed - but should be avoided there also.


> If you do an
> 
> if (running)
>   running=TRUE
> 
> *nothing* will inhibit a taskswitch in the virtual "line" 
> between the "if" and the assignment.
> So the second thread thread see too: "running not set"
> and you have the (false) reentrance you wanted to avoid.
> So the first task will overwrite the second
> tasks values when she's back on CPU...
> That might work, or not. In this case the thread
> might be started twice...
> 
> But: Why bothering with the "running" flag at all if you 
> accept that it might sometimes do not work or
> assume(!) that the function is never ever called from different threads?
> 
> 
> Depending how robust the remaining function is written
> that code might work and will "only" fail very rarely.
> The fails are not reproducible but annoying and
> avoidable and only because the code should look easier???
>
>
> An
> 
> if (running++)
> 
> is too absolutely "no atomic operation" tho it might look so.
> 
> 
> Programming multithreaded *is* complicate.
> That's why it's avoided so often or fails.
> 
> There is not sense in trying to "optimize" "shortcut" 
> the locking calls.
> 
> 
> It's "thread lottery" not protecting *every*
> "critical/atomic resource accesses" by semaphores or mutex etc..
> 
> It may work today flawlessly. But tomorrow there is a new
> plugin or a faster CPU and it may fail.
> BTST, beleave me.
> It is not worth the time you saved in not using a mutex.
> It's begging for long term trouble.

I agree completely. By isolating only parts that require mutex to
dedicated functions/methods it is easy to "hide" the protection and keep
the code clean.

Using tools such as Doxygen can provide class diagrams (useful for plain
C code as well) that can help to make necessary modifications to the
code to get it thread safe.


>>parentTid has been removed, since it was never actually used.
> 
> 
>>Please take a look at this and let me know if you can find a
>>possible loophole there.
> 
> That does not solve the other reentrancy problems too.
> "But here it works" is no anwser in an multithreaded environment 
> with more than one installation and more than one programmer
> (who will act as semaphore...until he forgot the details...).
> 
> BTW:
> Maybe you should describe more what the functions are expected to
> to do and what is assuemd arround and why it is done the way it is done.
> That would be of great help for all who are trying to help
> and would help to avoid to keep "cargo cult code" ;-)

I suggested to use Doxygen here. It generates automatically quite nice
looking documentation. Naturally comments are required (with prefix) as
it cannot invent then documentation. This would be very helpful for
other developers - plugin and patch makers.

-- 
t. Pasi