[PATCH] async: fix insert entry in ascending list

[PATCH] async: fix insert entry in ascending list

[Vaughan Cao]

Hi Tejun,

I suppose there is a fault in the patch of https://lkml.org/lkml/2013/1/16/546.
I know you made a new patch for latest kernel which don't move the entry
between pending and running list that remove the code I mentioned, but our
kernel is based on v3.8.13 that has the code.

In my understanding, both pending and running list are sorted ascendingly by
cookie value. To find the correct postion to insert the entry into running
list, we traverse reversely to the head. When a node with a smaller cookie is
found, we break out and add the new entry after it. But the origin code tries 
to find a larger cookie and insert itself before that node, it won't result in
a sorted list in any direction...

I don't know if my understanding about the async mechanism is right, so here 
to have a check with you. Thanks.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 kernel/async.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/async.c b/kernel/async.c
index 6f34904..596c5e7 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -135,9 +135,9 @@ static void async_run_entry_fn(struct work_struct *work)
 	/* 1) move self to the running queue, make sure it stays sorted */
 	spin_lock_irqsave(&async_lock, flags);
 	list_for_each_entry_reverse(pos, &running->domain, list)
-		if (entry->cookie < pos->cookie)
+		if (entry->cookie > pos->cookie)
 			break;
-	list_move_tail(&entry->list, &pos->list);
+	list_move(&entry->list, &pos->list);
 	spin_unlock_irqrestore(&async_lock, flags);
 
 	/* 2) run (and print duration) */
-- 
1.8.3.1

Re: [PATCH] async: fix insert entry in ascending list

[Tejun Heo]

Hello,

On Wed, Dec 18, 2013 at 11:15:23AM +0800, Vaughan Cao wrote:
> I suppose there is a fault in the patch of https://lkml.org/lkml/2013/1/16/546.
> I know you made a new patch for latest kernel which don't move the entry
> between pending and running list that remove the code I mentioned, but our
> kernel is based on v3.8.13 that has the code.
> 
> In my understanding, both pending and running list are sorted ascendingly by
> cookie value. To find the correct postion to insert the entry into running
> list, we traverse reversely to the head. When a node with a smaller cookie is
> found, we break out and add the new entry after it. But the origin code tries 
> to find a larger cookie and insert itself before that node, it won't result in
> a sorted list in any direction...

Yeah, I should have used list_for_each_entry() there.  LOL, I'm an
idiot.

> I don't know if my understanding about the async mechanism is right, so here 
> to have a check with you. Thanks.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
> ---
>  kernel/async.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/async.c b/kernel/async.c
> index 6f34904..596c5e7 100644
> --- a/kernel/async.c
> +++ b/kernel/async.c
> @@ -135,9 +135,9 @@ static void async_run_entry_fn(struct work_struct *work)
>  	/* 1) move self to the running queue, make sure it stays sorted */
>  	spin_lock_irqsave(&async_lock, flags);
>  	list_for_each_entry_reverse(pos, &running->domain, list)
> -		if (entry->cookie < pos->cookie)
> +		if (entry->cookie > pos->cookie)
>  			break;
> -	list_move_tail(&entry->list, &pos->list);
> +	list_move(&entry->list, &pos->list);

Hmmm... sadly, upstream doesn't have the ability to backport this.
The relevant code path is gone and -stable doesn't backport patches
which aren't mainline first.  The only way would be backporting
through distros, I guess.  But, again, this problem shouldn't be
noticeable with modern userland and it has been broekn without anyone
noticing for long enough, so maybe we can just leave it alone?

Thanks.

-- 
tejun

Re: [PATCH] async: fix insert entry in ascending list

[Vaughan Cao]

On 2013年12月18日 20:25, Tejun Heo wrote:
> Hello,
>
> On Wed, Dec 18, 2013 at 11:15:23AM +0800, Vaughan Cao wrote:
>> I suppose there is a fault in the patch of https://lkml.org/lkml/2013/1/16/546.
>> I know you made a new patch for latest kernel which don't move the entry
>> between pending and running list that remove the code I mentioned, but our
>> kernel is based on v3.8.13 that has the code.
>>
>> In my understanding, both pending and running list are sorted ascendingly by
>> cookie value. To find the correct postion to insert the entry into running
>> list, we traverse reversely to the head. When a node with a smaller cookie is
>> found, we break out and add the new entry after it. But the origin code tries
>> to find a larger cookie and insert itself before that node, it won't result in
>> a sorted list in any direction...
> Yeah, I should have used list_for_each_entry() there.  LOL, I'm an
> idiot.
I guess your original intention to use _reverse is that would take less 
steps to find the right position:)

>
>> I don't know if my understanding about the async mechanism is right, so here
>> to have a check with you. Thanks.
>>
>> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
>> ---
>>   kernel/async.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/async.c b/kernel/async.c
>> index 6f34904..596c5e7 100644
>> --- a/kernel/async.c
>> +++ b/kernel/async.c
>> @@ -135,9 +135,9 @@ static void async_run_entry_fn(struct work_struct *work)
>>   	/* 1) move self to the running queue, make sure it stays sorted */
>>   	spin_lock_irqsave(&async_lock, flags);
>>   	list_for_each_entry_reverse(pos, &running->domain, list)
>> -		if (entry->cookie < pos->cookie)
>> +		if (entry->cookie > pos->cookie)
>>   			break;
>> -	list_move_tail(&entry->list, &pos->list);
>> +	list_move(&entry->list, &pos->list);
> Hmmm... sadly, upstream doesn't have the ability to backport this.
> The relevant code path is gone and -stable doesn't backport patches
> which aren't mainline first.  The only way would be backporting
> through distros, I guess.  But, again, this problem shouldn't be
> noticeable with modern userland and it has been broekn without anyone
> noticing for long enough, so maybe we can just leave it alone?
>
> Thanks.
>
Got it. I'll consider pulling your patch of leaving node in pending list 
into our kernel. Thanks.