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