Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect

[Akio Takebe <takebe_akio@xxxxxxxxxxxxxx>]

Hi, Markus

Markus Armbruster wrote:
> Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >
> > 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

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