[vdr] Re: vdr[3330]: ERROR: can't record MPEG1! - possible cause

[Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de>]

Andreas Schultz wrote:
> 
> Hi,
> 
> Klaus Schmidinger wrote:
> 
> [...]
> 
> >>The race is in the if condition. Consider this: execution switches from
> >>thread A to thread B, after thread A has executed the first part of the
> >>condition (getpid() != lockingPid), but not the second part, at this
> >>point thread B could execute the full statement above, setting locked to
> >>1, if at this point execution switches back to thread A, the second part
> >>of the condition will return true, making thread A belive that he is
> >>holding the lock, and therefore doesn't need to aquire it, where in
> >>truth thread B is holding the lock.
> >>
> >
> > Meanwhile I am beginning to doubt that there can be a race condition here
> > at all. 'lockingPid' is 0 at startup, so any thread trying to lock the mutex
> > will see the first part of the 'if' as 'true' and will directly go to calling
> > 'pthread_mutex_lock()'. Of course, only one thread will return from this call
> > and actually "have" the lock. This thread will set 'lockingPid' to its very own
> > PID and increment 'locked'.
> 
> >
> > At this point, again, any other thread trying to to set the lock will see the
> > first part of the 'if' as true and directly go to calling 'pthread_mutex_lock()'.
> > The only thread that will ever actually see the first part of the 'if' as 'false'
> > is the thread that currently "has" the lock. And for that thread it is absolutely
> 
> Or that "had" the lock before. lockingPid is not cleared when the lock
> is released. Therefor a thread could see it's own pid in lockingPid
> without holding the lock.
> The solution is of course to clear lockingPid right before the lock is
> released.

Yes, you got a point there.
I have now changed the Lock() and Unlock() functions to look like this:

void cMutex::Lock(void)
{
  if (getpid() != lockingPid || !locked) {
     pthread_mutex_lock(&mutex);
     lockingPid = getpid();
     }
  locked++;
}

void cMutex::Unlock(void)
{
 if (!--locked) {
    lockingPid = 0;
    pthread_mutex_unlock(&mutex);
    }
}

> If you are interrested, i have also a solution that works without the
> double check.

I would be interested in _any_ version that actually works, and improves
the overall behaviour. However, with any version that uses "recursive mutexes"
so far I have had hangups in channel switching. So for the time being I'll
stay with the existing code, with the above small changes.

> > safe to check the second part of the 'if' - it will see that 'locked' is not 0
> > and will _not_ call 'pthread_mutex_lock()'.
> >
> > Since the only thread that can see the first part of the 'if' as false is the
> > one actually holding the lock, no two threads can see that part of the condition
> > as true at the same time.
> >
> > What _could_ be changed is the setting of 'lockingPid = getpid()', which would make
> > sense to be done only inside the 'if' branch. But then agian, I don't think that
> > that would make any difference.
> >
> > Please correct me if I'm wrong, but as far as I can see there is no race condition
> > here, so I guess using recursive mutexes won't get us any further (except that
> > channel switching will hang).
> 
> Meanwhile, i believe that it is very unlikely that we would hit this
> race under normal circumstances.

Me too. As far as I can see the only case where a recursive use of the mutexes
is made is in fact in channel switching. So during normal recording or replay
this shouldn't matter at all.

Klaus
-- 
_______________________________________________________________

Klaus Schmidinger                       Phone: +49-8635-6989-10
CadSoft Computer GmbH                   Fax:   +49-8635-6989-40
Hofmark 2                               Email:   kls@cadsoft.de
D-84568 Pleiskirchen, Germany           URL:     www.cadsoft.de
_______________________________________________________________