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: Jan Beulich <jbeulich@xxxxxxxxxx>
Subject: Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect
From: Akio Takebe <takebe_akio@xxxxxxxxxxxxxx>
Date: Wed, 28 Jan 2009 16:31:24 +0900
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Tue, 27 Jan 2009 23:32:09 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <497ED10A.76E4.0078.0@xxxxxxxxxx>
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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird 2.0.0.19 (X11/20090105)
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@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx> 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@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 @@
        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:
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel