From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akio Takebe Subject: Re: [Patch] fix xenfb_update_screen bogus rect Date: Wed, 28 Jan 2009 16:31:24 +0900 Message-ID: <498009CC.1000101@jp.fujitsu.com> References: <497E784B.7060003@jp.fujitsu.com> <497ED10A.76E4.0078.0@novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------040809000004070609050404" Return-path: In-Reply-To: <497ED10A.76E4.0078.0@novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------040809000004070609050404 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit 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 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 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: > [] xenfb_thread+0x19b/0x2be > [] try_to_wake_up+0x33b/0x34c > [] __wake_up_common+0x3e/0x68 > [] autoremove_wake_function+0x0/0x2e > [] keventd_create_kthread+0x0/0x61 > [] xenfb_thread+0x0/0x2be > [] keventd_create_kthread+0x0/0x61 > [] kthread+0xd4/0x109 > [] child_rip+0xa/0x12 > [] keventd_create_kthread+0x0/0x61 > [] xen_send_IPI_mask+0x0/0xf5 > [] kthread+0x0/0x109 > [] 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 > > Best Regards, > > Akio Takebe > > --------------040809000004070609050404 Content-Type: text/x-diff; name="fix_pvfb_dirty_lock.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="fix_pvfb_dirty_lock.patch" 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: --------------040809000004070609050404 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------040809000004070609050404--