Re: [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe <takebe_akio@jp.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

Hi, Markus

Markus Armbruster wrote:
> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
> 
>> Hi, Markus
>>
>> Markus Armbruster wrote:
>>> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
> [...]
>>>> diff -r 83b71f4b5cb2 drivers/xen/fbfront/xenfb.c
>>>> --- a/drivers/xen/fbfront/xenfb.c	Tue Jan 20 13:28:35 2009 +0000
>>>> +++ b/drivers/xen/fbfront/xenfb.c	Thu Jan 29 01:24:06 2009 +0900
>>>> @@ -213,17 +213,23 @@
>>> Please use -p with diff.
>>>
>>>>  	if (xenfb_queue_full(info))
>>>>  		return;
>>>>  -	mutex_lock(&info->mm_lock);
>>>> -
>>>>  	spin_lock_irqsave(&info->dirty_lock, flags);
>>>> -	y1 = info->y1;
>>>> -	y2 = info->y2;
>>>> -	x1 = info->x1;
>>>> -	x2 = info->x2;
>>>> -	info->x1 = info->y1 = INT_MAX;
>>>> -	info->x2 = info->y2 = 0;
>>>> +	if (info->dirty){
>>>> +		info->dirty = 0;
>>>> +		y1 = info->y1;
>>>> +		y2 = info->y2;
>>>> +		x1 = info->x1;
>>>> +		x2 = info->x2;
>>>> +		info->x1 = info->y1 = INT_MAX;
>>>> +		info->x2 = info->y2 = 0;
>>>> +	} else {
>>>> +		spin_unlock_irqrestore(&info->dirty_lock, flags);
>>>> +		return;
>>>> +	}
>>>>  	spin_unlock_irqrestore(&info->dirty_lock, flags);
>>>>  +	mutex_lock(&info->mm_lock);
>>>> +	
>>>>  	list_for_each_entry(map, &info->mappings, link) {
>>>>  		if (!map->faults)
>>>>  			continue;
>>> Careful, locking is rather delicate here.  Please read the big comment
>>> "There are three locks:", then explain why moving the locking of
>>> mm_lock is safe.
>>>
>> Thank you for your review.
> 
> My pleasure.  I have to thank for your fix!
> 
>> First, I thought mm_lock just protected the mappings,
>> and the dirty rectangle was protected by the dirty_lock.
>> So I moved the mm_lock.
>> Also when I tested the patch, I didn't get any problem.
> 
> Subtle locking bugs are notoriously hard to demonstrate by testing.
> 
>> Do you mean the narrow point of between unloking dirty_lock and locking mm_lock
>> in xenfb_update_screen() may be not safe?
>> Do you find any critical cases?
>>
>> Best Regards,
>>
>> Akio Takebe
> 
> Your proposed change might be fine, it might be racy, I don't know.
> What I do know is that it invalidates the comment I mentioned.  That's
> a nasty trap for the unwary, and clearly a bug as far as I'm
> concerned.
> 
> I strongly recommend to either revert the locking change, or fix the
> comment to match the new code.
> 
I agree with you. I revert the mm_lock changing.
In my graphic test, I didn't get any problem with this patch.

Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>

Best Regards,

Akio Takebe

[-- Attachment #2: revert_mm_lock.patch --]
[-- Type: text/x-diff, Size: 780 bytes --]

diff -r f90f02049d7e drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c	Thu Jan 29 18:26:47 2009 +0900
+++ b/drivers/xen/fbfront/xenfb.c	Fri Jan 30 10:54:07 2009 +0900
@@ -210,6 +210,8 @@ static void xenfb_update_screen(struct x
 	if (xenfb_queue_full(info))
 		return;
 
+	mutex_lock(&info->mm_lock);
+	
 	spin_lock_irqsave(&info->dirty_lock, flags);
 	if (info->dirty){
 		info->dirty = 0;
@@ -221,12 +223,11 @@ static void xenfb_update_screen(struct x
 		info->x2 = info->y2 = 0;
 	} else {
 		spin_unlock_irqrestore(&info->dirty_lock, flags);
+		mutex_unlock(&info->mm_lock);
 		return;
 	}
 	spin_unlock_irqrestore(&info->dirty_lock, flags);
 
-	mutex_lock(&info->mm_lock);
-	
 	list_for_each_entry(map, &info->mappings, link) {
 		if (!map->faults)
 			continue;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel