[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect
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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |