[MPlayer-users] lockless cache2 looks buggy

[Denis Vlasenko]

Hello mplayer coders,

I was looking into cache2.c. It says that it is a lockless
cache implementation.

I see s few issues with the code. First issue is the most serious,
since it looks like real bug.

(1) Non-atomic updated of metadata.

Main thread reds cached data using cache_read(), which uses
off_t variables s->min_filepos, s->max_filepos etc.
They are 64-bit variables on most systems:

    newb=s->max_filepos-s->read_filepos; // new bytes in the buffer
    pos=s->read_filepos - s->offset;
    if(pos<0) pos+=s->buffer_size; else
    if(pos>=s->buffer_size) pos-=s->buffer_size;
    if(newb>s->buffer_size-pos) newb=s->buffer_size-pos; // handle wrap...
    if(newb>size) newb=size;
    memcpy(buf,&s->buffer[pos],newb);

However, caching thread reads new data into circular buffer
and updates those same variables in cache_fill():

  back2=s->buffer_size-(space+newb); // max back size
  if(s->min_filepos<(read-back2)) s->min_filepos=read-back2;
  len=stream_read(s->stream,&s->buffer[pos],space);
  if(!len) s->eof=1;
  s->max_filepos+=len;
  if(pos+len>=s->buffer_size){
      // wrap...
      s->offset+=s->buffer_size;
  }

These updates are not atomic on 32-bit architectures (i386).
Therefore caching thread can update lower half of a value, then it
may get rescheduled with CPU given to main thread, which then
will use bogus value of half-updated variable.

(2) fork() failure is not checked

if((stream->cache_pid=fork())){

The above if() thinks that non-zero return means that we are the parent.
However, -1 is possible too (fork() may fail).

(3) Suboptimal underrun handling.

Cache underrun handling looks suboptimal to me:

  while(size>0){
    int pos,newb,len;

    if(s->read_filepos>=s->max_filepos || s->read_filepos<s->min_filepos){
        // eof?
        if(s->eof) break;
        // waiting for buffer fill...
        usec_sleep(READ_USLEEP_TIME); // 10ms
        continue; // try again...
    }
  ...
  }

If you have an underrun, it is not that good an idea to sleep.
Isn't it better to just schedule (man sched_yield), giving
cache thread a chance to do its work? In the above code,
you can easily add 10 ms more latency for the already bad case
of cache underrun.

(4) Jumbledtogethercoditionalexpressions

while(s->read_filepos<s->min_filepos || s->max_filepos-s->read_filepos<min){

This code has readability problems. Do you hate whitespace? ;)


I am willing to work on this code if it is acceptable to start
with reformatting cache2.c into more readable form first.
Would such patch be accepted?

Best regards,
--
vda