Dave Airlie wrote:
> On Wed, Apr 22, 2009 at 11:48 AM, Shaohua Li <shaohua...@intel.com> 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
------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensign option that enables unlimited
royalty-free distribution of the report engine for externally facing
server and web deployment.
http://p.sf.net/sfu/businessobjects
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
