[Patch] fix xenfb_update_screen bogus rect

[Patch] fix xenfb_update_screen bogus rect

[Akio Takebe]

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

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: 1152 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	Tue Jan 27 20:09:58 2009 +0900
@@ -255,19 +255,36 @@
 	}
 	spin_unlock_irqrestore(&info->resize_lock, flags);
 }
+static int check_is_dirty(struct xenfb_info *info)
+{
+	int is_dirty;
+	unsigned long flags;
+	spin_lock_irqsave(&info->dirty_lock, flags);
+	is_dirty = info->dirty;
+	spin_unlock_irqrestore(&info->dirty_lock, flags);
+
+	return is_dirty;
+}
 
 static int xenfb_thread(void *data)
 {
 	struct xenfb_info *info = data;
+	unsigned long flags;
+	int is_dirty;
 
 	while (!kthread_should_stop()) {
 		xenfb_handle_resize_dpy(info);
-		if (info->dirty) {
+		spin_lock_irqsave(&info->dirty_lock, flags);
+		is_dirty = info->dirty;
+		if (is_dirty) {
 			info->dirty = 0;
+			spin_unlock_irqrestore(&info->dirty_lock, flags);
 			xenfb_update_screen(info);
+		} else {
+			spin_unlock_irqrestore(&info->dirty_lock, flags);
 		}
 		wait_event_interruptible(info->wq,
-			kthread_should_stop() || info->dirty);
+			kthread_should_stop() || check_is_dirty(info));
 		try_to_freeze();
 	}
 	return 0;

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

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Jan Beulich]

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe]

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Jan Beulich]

Generally this looks okay (assuming it works), but couldn't the update_wanted
field then go away altogether?

Jan

>>> Akio Takebe <takebe_akio@jp.fujitsu.com> 28.01.09 08:31 >>>
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
> 
> 

Re: [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster]

"Jan Beulich" <jbeulich@novell.com> writes:

> Generally this looks okay (assuming it works), but couldn't the update_wanted
> field then go away altogether?
>
> Jan

I think so, yes.  Start the kthread only when request-update.

Re: [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe]

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

Hi, Markus and Jan

Markus Armbruster wrote:
> "Jan Beulich" <jbeulich@novell.com> writes:
> 
>> Generally this looks okay (assuming it works), but couldn't the update_wanted
>> field then go away altogether?
>>
>> Jan
> 
> I think so, yes.  Start the kthread only when request-update.
> 
Thank you for your review.
The patch was commited already.
So I made an additional patch which eliminate the update_wanted field.

Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>

Best Regards,

Akio Takebe

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

diff -r 51decc39e5e7 drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c	Wed Jan 28 13:42:09 2009 +0000
+++ b/drivers/xen/fbfront/xenfb.c	Thu Jan 29 18:04:36 2009 +0900
@@ -61,7 +61,6 @@ struct xenfb_info
 	int			irq;
 	struct xenfb_page	*page;
 	unsigned long 		*mfns;
-	int			update_wanted; /* XENFB_TYPE_UPDATE wanted */
 	int			feature_resize; /* Backend has resize feature */
 	struct xenfb_resize	resize;
 	int			resize_dpy;
@@ -208,8 +207,6 @@ static void xenfb_update_screen(struct x
 	int y1, y2, x1, x2;
 	struct xenfb_mapping *map;
 
-	if (!info->update_wanted)
-		return;
 	if (xenfb_queue_full(info))
 		return;
 
@@ -823,22 +820,24 @@ static void xenfb_backend_changed(struct
 		if (dev->state != XenbusStateConnected)
 			goto InitWait; /* no InitWait seen yet, fudge it */
 
-		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-				 "request-update", "%d", &val) < 0)
-			val = 0;
-		if (val)
-			info->update_wanted = 1;
 
 		if (xenbus_scanf(XBT_NIL, dev->otherend,
 					"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");
+		if (xenbus_scanf(XBT_NIL, info->xbdev->otherend,
+				 "request-update", "%d", &val) < 0)
+			val = 0;
+
+		if (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;
 

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

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster]

Akio Takebe <takebe_akio@jp.fujitsu.com> writes:

> Hi, Markus and Jan
>
> Markus Armbruster wrote:
>> "Jan Beulich" <jbeulich@novell.com> writes:
>>
>>> Generally this looks okay (assuming it works), but couldn't the update_wanted
>>> field then go away altogether?
>>>
>>> Jan
>>
>> I think so, yes.  Start the kthread only when request-update.
>>
> Thank you for your review.
> The patch was commited already.
> So I made an additional patch which eliminate the update_wanted field.
>
> Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>
>
> Best Regards,
>
> Akio Takebe

Looks good to me.

A possible reason not to take this cleanup is the fact that this
version of the driver is a dead end (the living end lives in current
upstream kernels).  Perhaps only true fixes with minimal structural
impact are desired because of that.  Not my call to make.

Re: [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster]

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.

[...]

Re: [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe]

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster]

Akio Takebe <takebe_akio@jp.fujitsu.com> writes:

> Hi, Markus
>
> Markus Armbruster wrote:
>> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
[...]
>>> 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.

My pleasure.  I have to thank for your fix!

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

Subtle locking bugs are notoriously hard to demonstrate by testing.

> 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

Your proposed change might be fine, it might be racy, I don't know.
What I do know is that it invalidates the comment I mentioned.  That's
a nasty trap for the unwary, and clearly a bug as far as I'm
concerned.

I strongly recommend to either revert the locking change, or fix the
comment to match the new code.

Re: [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe]

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

Hi, Markus

Markus Armbruster wrote:
> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
> 
>> Hi, Markus
>>
>> Markus Armbruster wrote:
>>> Akio Takebe <takebe_akio@jp.fujitsu.com> writes:
> [...]
>>>> 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.
> 
> My pleasure.  I have to thank for your fix!
> 
>> 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.
> 
> Subtle locking bugs are notoriously hard to demonstrate by testing.
> 
>> 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
> 
> Your proposed change might be fine, it might be racy, I don't know.
> What I do know is that it invalidates the comment I mentioned.  That's
> a nasty trap for the unwary, and clearly a bug as far as I'm
> concerned.
> 
> I strongly recommend to either revert the locking change, or fix the
> comment to match the new code.
> 
I agree with you. I revert the mm_lock changing.
In my graphic test, I didn't get any problem with this patch.

Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>

Best Regards,

Akio Takebe

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

diff -r f90f02049d7e drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c	Thu Jan 29 18:26:47 2009 +0900
+++ b/drivers/xen/fbfront/xenfb.c	Fri Jan 30 10:54:07 2009 +0900
@@ -210,6 +210,8 @@ static void xenfb_update_screen(struct x
 	if (xenfb_queue_full(info))
 		return;
 
+	mutex_lock(&info->mm_lock);
+	
 	spin_lock_irqsave(&info->dirty_lock, flags);
 	if (info->dirty){
 		info->dirty = 0;
@@ -221,12 +223,11 @@ static void xenfb_update_screen(struct x
 		info->x2 = info->y2 = 0;
 	} else {
 		spin_unlock_irqrestore(&info->dirty_lock, flags);
+		mutex_unlock(&info->mm_lock);
 		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;

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

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

Re: [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster]

Akio Takebe <takebe_akio@jp.fujitsu.com> writes:

> Hi, Markus
>
> Markus Armbruster wrote:
[...]
>> I strongly recommend to either revert the locking change, or fix the
>> comment to match the new code.
>>
> I agree with you. I revert the mm_lock changing.
> In my graphic test, I didn't get any problem with this patch.
>
> Signed-off-by: Akio Takebe <takebe_akio@jp.fujitsu.com>
>
> Best Regards,
>
> Akio Takebe

Looks good to me.  Thanks again!

[PATCH] fbfront: Improve diagnostics when kthread_run() fails

[Markus Armbruster]

Failure is reported with xenbus_dev_fatal(..."register_framebuffer"),
which was already suboptimal before it got moved away from
register_framebuffer(), and is outright misleading now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>

diff -r 26ddc59c674d drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c	Fri Jan 30 10:54:10 2009 +0000
+++ b/drivers/xen/fbfront/xenfb.c	Mon Feb 02 10:12:54 2009 +0100
@@ -211,7 +211,7 @@
 		return;
 
 	mutex_lock(&info->mm_lock);
-	
+
 	spin_lock_irqsave(&info->dirty_lock, flags);
 	if (info->dirty){
 		info->dirty = 0;
@@ -837,7 +837,7 @@
 			if (IS_ERR(info->kthread)) {
 				info->kthread = NULL;
 				xenbus_dev_fatal(dev, PTR_ERR(info->kthread),
-						"register_framebuffer");
+						"xenfb_thread");
 			}
 		}
 		break;

Re: [PATCH] fbfront: Improve diagnostics when kthread_run()fails

[Akio Takebe]

Hi, Markus

Thanks. I agree with you.

Best Regards,

Akio Takebe

>Failure is reported with xenbus_dev_fatal(..."register_framebuffer"),
>which was already suboptimal before it got moved away from
>register_framebuffer(), and is outright misleading now.
>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>diff -r 26ddc59c674d drivers/xen/fbfront/xenfb.c
>--- a/drivers/xen/fbfront/xenfb.c	Fri Jan 30 10:54:10 2009 +0000
>+++ b/drivers/xen/fbfront/xenfb.c	Mon Feb 02 10:12:54 2009 +0100
>@@ -211,7 +211,7 @@
> 		return;
> 
> 	mutex_lock(&info->mm_lock);
>-	
>+
> 	spin_lock_irqsave(&info->dirty_lock, flags);
> 	if (info->dirty){
> 		info->dirty = 0;
>@@ -837,7 +837,7 @@
> 			if (IS_ERR(info->kthread)) {
> 				info->kthread = NULL;
> 				xenbus_dev_fatal(dev, PTR_ERR(info->kthread),
>-						"register_framebuffer");
>+						"xenfb_thread");
> 			}
> 		}
> 		break;
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel