Re: [PATCH 1/2] sched/wait: add round robin wakeup mode

[Jason Baron <jbaron@akamai.com>]

On 02/10/2015 04:03 AM, Peter Zijlstra wrote:
> On Mon, Feb 09, 2015 at 11:06:17PM -0500, Jason Baron wrote:
>> On 02/09/2015 04:50 PM, Peter Zijlstra wrote:
>>> On Mon, Feb 09, 2015 at 08:05:57PM +0000, Jason Baron wrote:
>>>> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
>>>> index 852143a..17d1039 100644
>>>> --- a/kernel/sched/wait.c
>>>> +++ b/kernel/sched/wait.c
>>>> @@ -71,8 +71,11 @@ static void __wake_up_common(wait_queue_head_t *q, unsigned int mode,
>>>>  		unsigned flags = curr->flags;
>>>>  
>>>>  		if (curr->func(curr, mode, wake_flags, key) &&
>>>> -				(flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
>>>> +			       (flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive) {
>>>> +			if (flags & WQ_FLAG_ROUND_ROBIN)
>>>> +				list_move_tail(&curr->task_list, &q->task_list);
>>>>  			break;
>>>> +		}
>>>>  	}
>>>>  }
>>> I think you meant to write something like:
>>>
>>> 	if (curr->func(curr, mode, wake_flags, key) &&
>>> 	    (flags & WQ_FLAG_EXCLUSIVE)) {
>>> 		if (flag & WQ_FLAG_ROUND_ROBIN)
>>> 			list_move_tail(&curr->task_list, &q->task_list);
>>> 		if (!--nr_exclusive)
>>> 			break;
>>> 	}
>>>
>>> Otherwise can only work for nr_exclusive==1.
>> Indeed. I'm also wondering if its worth avoiding the list_move_tail()
>> for the case where nr_exclusive is initially 0. IE the wake all case,
>> where we are just going to end up doing a bunch of list_move_tail()
>> calls, but end up in the same state.
> After writing this email, it occurred to me that you could probably do
> this with a custom wake function.
>
> Where autoremove_wake_function() does a list_del_init() you could do a
> rotate_wake_function() that does list_move_tail().
>
> That would avoid the entire WQ flag muckery.

hmmm...but don't we need the head/tail of the list to add it back too?

Further, we can't just append to tail while walking the list b/c
otherwise it can result in multiple wakeups to the same item. So I could
add to a local list, for example, in __wake_up_common(). And then just
add that to the tail once the list_for_each() finishes.

In terms of the flag, maybe another option would be to have the
wait_queue_func_t return a 'ROTATE_ME' value instead
of 1, since I think we currently only make use of 0 and 1?

Thanks,

-Jason