From mboxrd@z Thu Jan  1 00:00:00 1970
From: Akio Takebe <takebe_akio@jp.fujitsu.com>
Subject: Re: [Patch] fix xenfb_update_screen bogus rect
Date: Thu, 29 Jan 2009 21:54:37 +0900
Message-ID: <4981A70D.1010905@jp.fujitsu.com>
References: <497E784B.7060003@jp.fujitsu.com>	<497ED10A.76E4.0078.0@novell.com>
	<498009CC.1000101@jp.fujitsu.com>
	<871vun33jp.fsf@pike.pond.sub.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <871vun33jp.fsf@pike.pond.sub.org>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Markus Armbruster <armbru@redhat.com>
Cc: xen-devel <xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org

Hi, Markus

Markus Armbruster wrote:
> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
> 
>> Hi, Jan
>>
>> Thank you for your review.
>> I remade it simpler.
>> But Just moving check-and-clear dirty flag to xenfb_update_screen(),
>> a guest with vcpu=1 could not boot.
>> It is caused by info->update_wanted = 0.
> 
> Thanks for debugging this!
> 
>> So I moved kthread_run() into XenbusStateConnected handling.
>> How about it?
>>
>> Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>
>>
>> Best Regards,
>>
>> Akio Takebe
> [...]
>> 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.
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.
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