WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect

To: Markus Armbruster <armbru@xxxxxxxxxx>
Subject: Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect
From: Akio Takebe <takebe_akio@xxxxxxxxxxxxxx>
Date: Thu, 29 Jan 2009 21:54:37 +0900
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 29 Jan 2009 04:55:08 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <871vun33jp.fsf@xxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <497E784B.7060003@xxxxxxxxxxxxxx> <497ED10A.76E4.0078.0@xxxxxxxxxx> <498009CC.1000101@xxxxxxxxxxxxxx> <871vun33jp.fsf@xxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird 2.0.0.19 (X11/20090105)
Hi, Markus

Markus Armbruster wrote:
Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>

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

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