Hi, Markus
Markus Armbruster wrote:
Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> writes:
Hi, Markus
Markus Armbruster wrote:
Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
Best Regards,
Akio Takebe
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home