From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akio Takebe Subject: Re: [Patch] fix xenfb_update_screen bogus rect Date: Fri, 30 Jan 2009 11:02:23 +0900 Message-ID: <49825FAF.3050806@jp.fujitsu.com> References: <497E784B.7060003@jp.fujitsu.com> <497ED10A.76E4.0078.0@novell.com> <498009CC.1000101@jp.fujitsu.com> <871vun33jp.fsf@pike.pond.sub.org> <4981A70D.1010905@jp.fujitsu.com> <87y6wuw4ra.fsf@pike.pond.sub.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------000204010509040804070809" Return-path: In-Reply-To: <87y6wuw4ra.fsf@pike.pond.sub.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Markus Armbruster Cc: xen-devel List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------000204010509040804070809 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Hi, Markus Markus Armbruster wrote: > Akio Takebe writes: > >> Hi, Markus >> >> Markus Armbruster wrote: >>> Akio Takebe 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 Best Regards, Akio Takebe --------------000204010509040804070809 Content-Type: text/x-diff; name="revert_mm_lock.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="revert_mm_lock.patch" 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; --------------000204010509040804070809 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 --------------000204010509040804070809--