Re: [Patch] fix xenfb_update_screen bogus rect

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

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

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.
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

Jan Beulich wrote:
> This looks bogus to me. check_is_dirty() definitely isn't needed - if at all, a
> memory read barrier may need to be added, but since kthread_should_stop()
> is a function call even that ought to be unnecessary.
> 
> I also think the other change is more involved than it needs to be - it'd be
> much simpler to let xenfb_update_screen() check-and-clear the dirty flag
> along with reading the other fields, and bail if the flag was clear.
> 
> Jan
> 
>>>> Akio Takebe <takebe_akio@jp.fujitsu.com> 27.01.09 03:58 >>>
> Hi,
> 
> When I tested pvfb, I got the following warnings.
> It seems to be caused by checking/setting info->dirty without dirty_lock.
> We need to check/set info->dirty safely.
> 
> xenfb_update_screen bogus rect 2147483647 0 2147483647 0
>  BUG: warning at /root/linux-2.6.18-xen.hg/drivers/xen/fbfront/xenfb.c:240/xenfb_update_screen()
> 
>  Call Trace:
>   [<ffffffff8036920e>] xenfb_thread+0x19b/0x2be
>   [<ffffffff8022730a>] try_to_wake_up+0x33b/0x34c
>   [<ffffffff80225c3d>] __wake_up_common+0x3e/0x68
>   [<ffffffff80241e20>] autoremove_wake_function+0x0/0x2e
>   [<ffffffff80241a75>] keventd_create_kthread+0x0/0x61
>   [<ffffffff80369073>] xenfb_thread+0x0/0x2be
>   [<ffffffff80241a75>] keventd_create_kthread+0x0/0x61
>   [<ffffffff80241ceb>] kthread+0xd4/0x109
>   [<ffffffff8020afe0>] child_rip+0xa/0x12
>   [<ffffffff80241a75>] keventd_create_kthread+0x0/0x61
>   [<ffffffff8021477f>] xen_send_IPI_mask+0x0/0xf5
>   [<ffffffff80241c17>] kthread+0x0/0x109
>   [<ffffffff8020afd6>] child_rip+0x0/0x12
> 
> 
> FYI, when I tested it, I used the following shell scripts on PV guest.
> The above warnings occurred in /var/log/messages.
> =======================
> #!/bin/bash
> 
> while true
> do
>     date
> done
> =======================
> 
> The attached patch fixed this warnings.
> How about it?
> 
> Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>
> 
> Best Regards,
> 
> Akio Takebe
> 
> 

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

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 @@
 	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;
@@ -262,10 +268,7 @@
 
 	while (!kthread_should_stop()) {
 		xenfb_handle_resize_dpy(info);
-		if (info->dirty) {
-			info->dirty = 0;
-			xenfb_update_screen(info);
-		}
+		xenfb_update_screen(info);
 		wait_event_interruptible(info->wq,
 			kthread_should_stop() || info->dirty);
 		try_to_freeze();
@@ -666,15 +669,6 @@
 	if (ret < 0)
 		goto error;
 
-	/* FIXME should this be delayed until backend XenbusStateConnected? */
-	info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
-	if (IS_ERR(info->kthread)) {
-		ret = PTR_ERR(info->kthread);
-		info->kthread = NULL;
-		xenbus_dev_fatal(dev, ret, "register_framebuffer");
-		goto error;
-	}
-
 	return 0;
 
  error_nomem:
@@ -839,6 +833,13 @@
 					"feature-resize", "%d", &val) < 0)
 			val = 0;
 		info->feature_resize = val;
+
+		info->kthread = kthread_run(xenfb_thread, info, "xenfb thread");
+		if (IS_ERR(info->kthread)) {
+			info->kthread = NULL;
+			xenbus_dev_fatal(dev, PTR_ERR(info->kthread),
+					"register_framebuffer");
+		}
 		break;
 
 	case XenbusStateClosing:

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

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