Re: [RFC] drm: drm_lock possible error

[Thomas H. <the...@vm...>]

Dave Airlie wrote:
> On Wed, Apr 22, 2009 at 11:48 AM, Shaohua Li <sha...@in...> wrote:
>   
>> drm_lock() does:
>>        for (;;) {
>>                __set_current_state(TASK_INTERRUPTIBLE);
>>                ...
>>                if (drm_lock_take(&master->lock, lock->context)) {
>>                        <==== schedule() here
>>                        master->lock.file_priv = file_priv;
>>                        master->lock.lock_time = jiffies;
>>                        atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
>>                        break;  /* Got lock */
>>                }
>>                ...
>>        }
>> If a preempt occurs in marked line, the task already holds the lock but
>> set to interruptible, then nobody can wakeup the task (except signal) and
>> other tasks can't get the lock again. Am I missing anything?
>>     
>
> Thomas you seem to have the best understanding of this code, can you
> take a look and ack this?
>
> Dave.
>   
At a first glance this looks like a sane patch.
In essence what's said is that a TASK_INTERRUPTIBLE task must never be 
preempted, because it might be that nobody's there to wake it up.
But we would've most likely hit the consequences by now, wouldn't we?

Also looking at similar code (for example __wait_event_interruptible) in 
<linux/wait.h> there's no
preempt_disable.

Before we adopt this patch we'd need to understand why that is. Could it 
be that the scheduler is
smart enough never to put (!TASK_RUNNING) processes to sleep when 
they're preempted?

That said, we should fix wait code like this to use prepare_to_wait / 
finish_wait to mimic what's in linux/wait.h. In this case it should even 
be possible to use wait_event_interruptible(). There seems to be some 
hairy memory_barrier things that we currently don't get right on 
processors that reorder write operations. See comments on 
set_current_state in <linux/sched.h>

So I won't ack this until we have an explanation why <linux/wait.h> 
isn't doing the same thing.

/Thomas