Sorry, a little mistake in my above mail, it's not TASK_RUNNING, but it still
will run on although it's not TASK_RUNNING, just because the scheduler
won't remove
it from runqueue.
preempt_schedule() is the entry point of in-kernel preemption
preempt_schedule():
add_preempt_count(PREEMPT_ACTIVE);
schedule();
sub_preempt_count(PREEMPT_ACTIVE);
and schedule() calls __schedule(), in __schedule() we have :
if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
if (unlikely(signal_pending_state(prev->state, prev)))
prev->state = TASK_RUNNING;
else
deactivate_task(rq, prev, 1);
switch_count = &prev->nvcsw;
}
That's to say, a TASK_INTERRUPTIBLE prev maybe removed only when
preempt_count() & PREEMPT_ACTIVE == 0,
which is not true since we have add_preempt_count(PREEMPT_ACTIVE) already in
preempt_schedule().
-Zhu Yanhai
2009/4/24 Yanhai Zhu <[email protected]>:
> " If the lock is taken, then
> nobody can take drm lock, so nobody will call drm_unlock() "
>
> No, the lock owner will continue as usual, because __schedule() will
> set it to TASK_RUNNING,
> please take a look at __schedule() for more.
>
> --
> Regards,
> Zhu Yanhai
>
>
> 2009/4/24 Shaohua Li <[email protected]>:
>> On Fri, 2009-04-24 at 16:10 +0800, Thomas Hellstrom wrote:
>>> Dave Airlie wrote:
>>> > On Wed, Apr 22, 2009 at 11:48 AM, Shaohua Li <[email protected]> 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?
>> no. If yes, then the schedule() just below the code will report error.
>> I guess this is case by case. In this case, if no lock is taken, the
>> preempt() in interruptible is ok, because other thread will eventually
>> wake up the thread with a drm_unlock(). If the lock is taken, then
>> nobody can take drm lock, so nobody will call drm_unlock()
>>
>> Thanks,
>> Shaohua
>>
>>
>> ------------------------------------------------------------------------------
>> 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
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/dri-devel
>>
>
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
