On 05/23/2012 08:06 PM, Dave Chinner wrote:
> On Wed, May 23, 2012 at 01:48:54PM -0400, Brian Foster wrote:
>> On 05/22/2012 08:58 PM, Dave Chinner wrote: snip
>>>
>>> Finally, rather than calling wake_up_process() in the
>>> xfs_ail_push*() functions, call wake_up(&ailp->xa_idle); There
>>> can only be one thread sleeping on that (the xfsaild) so there
>>> is no need to use the wake_up_all() variant...
>>>
>>> FWIW, you might be able to do this without the idle wait queue
>>> and just use wake_up_process() -
>>>
>>
>> Hi Dave,
>>
>> I have a working version of your suggested algorithm. It looks
>> mostly the same with the exception of a spin_unlock fix. I also
>> have the below version that uses a wait_queue and that I plan to
>> test overnight tonight:
>
> See my previous mail about using an idle queue.
>
Ok, I was a bit curious why you suggested that, but I figured it was for
aesthetic or consistency reasons. ;) No problem.
>> while (!kthread_should_stop()) {
>> if (tout && tout <= 20)
>> state = TASK_KILLABLE;
>> else
>> state = TASK_INTERRUPTIBLE;
>>
>> prepare_to_wait(&ailp->xa_idle, &wait, state);
>>
>> spin_lock(&ailp->xa_lock);
>> /* barrier matches the xa_target update in xfs_ail_push() */
>> smp_rmb();
>> if (!xfs_ail_min(ailp) && (ailp->xa_target == ailp->xa_target_prev)) {
>> /* the ail is empty and no change to the push target - idle */
>> spin_unlock(&ailp->xa_lock);
>> schedule();
>> } else if (tout) {
>> spin_unlock(&ailp->xa_lock);
>> /* more work to do soon */
>> schedule_timeout(msecs_to_jiffies(tout));
>> } else {
>> spin_unlock(&ailp->xa_lock);
>> }
>
> Three separate unlocks? that's a recipe for future disasters. how
> about:
>
FWIW, I started off with two just to fix the double unlock on return
from idle mode, then rearranged that for some reason when I added the
idle queue.
> if (!xfs_ail_min(ailp) && (ailp->xa_target == ailp->xa_target_prev)) {
> /* the ail is empty and no change to the push target - idle */
> spin_unlock(&ailp->xa_lock);
> schedule();
> tout = 0;
> continue;
> }
> spin_unlock(&ailp->xa_lock);
>
> if (tout) {
> /* more work to do soon */
> schedule_timeout(msecs_to_jiffies(tout));
> }
>
> So that we recheck the idle condition on wakeup from idle before
> doing anything. (i.e. handle spurious idle wakeups effectively). By
> setting the tout to zero, we then fall through immediately to
> pushing the AIL if it was a real wakeup that moved the target....
>
That sounds good to me. Thanks again.
Brian
> Cheers,
>
> Dave.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
