Re: [PATCH 02/02] libata: implement ata_eh_wait()

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +
>> +    while (ap->flags & (ATA_FLAG_EH_PENDING | 
>> ATA_FLAG_EH_IN_PROGRESS)) {
>> +        prepare_to_wait(&ap->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE);
>> +        spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +        schedule();
>> +        spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    }
> 
> 
> Two comments:
> 
> * why not use completions?

Mainly because there can be more than one waiters and also because the 
waiting doesn't really fit completion semantics.  In some places, 
eh_wait is used just to flush EH without actually scheduling EH.  In 
such cases, there's no synchronization point to start waiting for 
completion.

> * don't use schedule().  If there's nothing to schedule, it IMO chews up 
> too much CPU busy-waiting.  schedule_timeout(1) will at least wait for 
> the next timer tick.

I don't really understand your point here.  All that schedule_timeout() 
does is adding a timer to wake it up after the given jiffies pass.  It 
only adds wake up condition.  ie. it only makes sleeps shorter not 
longer.  e.g. both of the following code snippets result in busy 
sleep/wake up loop.

__set_current_state(TASK_RUNNING);
while (some_condition)
	schedule();

-------

__set_current_state(TASK_RUNNING);
while (some_condition)
	schedule_timeout(1);

The only behavior difference between above two code snippets is that the 
latter registers and unregisters timer every iteration which probably 
never expires as the thread would usually get scheduled again before a 
full tick passes.

AFAICS, schedule_timeout() is to be used when the wait should be 'timed 
out' when the thread has other wake up condition it's waiting for but it 
doesn't want to wait forever.  As a special case, when the condition is 
nil, it works as unconditional sleep.

Whether a thread gets rescheduled or not is determined by 
current->task_state.  If it's RUNNING, it will get rescheduled 
immediately.  If it's UNINTERRUPTIBLE or INTERRUPTIBLE, it will wait 
till gets woken up.  schedule() simply means the caller is done with the 
CPU at the moment and giving it away.

In eh_wait(), prepare_to_wait() is called w/ TASK_UNINTERRUPTIBLE before 
calling schedule, which sets current->task_state to UNINTERRUPTIBLE. 
The thread won't be scheduled again until the wait condition triggers 
and restores task_state to RUNNING.

Oops, I forgot calling finish_wait() after exiting the loop.  I'll 
submit a patch as soon as #upstream is updated.  Sorry.

Anyways, please read workqueue.c::flush_cpu_workqueue() and 
sched.c::wait_for_completion().  These two functions use similar waiting 
loops.

Thanks.

-- 
tejun