Messages in this thread |  | | Date | Mon, 16 Jun 2008 18:11:14 -0700 | From | "Jaya Kumar" <> | Subject | Re: "fb-defio: fix page list with concurrent processes" |
| |
On Mon, Jun 16, 2008 at 3:05 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote: > Your patch "fb-defio: fix page list with concurrent processes" definitely > seems to help with the suspend/resume problem I had with the Xen pvfb > device. Is it queued up anywhere? It seems to be a real bugfix, and should > probably be queued for 2.6.26...
It isn't currently queued. I had intended to improve its performance by taking advantage of Andrew's suggestion of using !list_empty on the page->lru to avoid walking the page list to find the duplicate page, but I ran into trouble since the page starts off being on the lru list. I'll try to take a look at doing this next weekend.
Thanks, jaya
> > J > > fb-defio: fix page list with concurrent processes > > From: Jaya Kumar <jayakumar.lkml@gmail.com> > > Hi Tony, Geert, Andrew, fbdev, > > This patch is a bugfix for how defio handles multiple processes manipulating > the same framebuffer. Thanks to Bernard Blackham for identifying this bug. > It occurs when two applications mmap the same framebuffer and concurrently > write to the same page. Normally, this doesn't occur since only a single > process mmaps the framebuffer. The symptom of the bug is that the mapping > applications will hang. The cause is that defio incorrectly tries to add the > same page twice to the pagelist. The solution I have is to walk the pagelist > and check for a duplicate before adding. Since I needed to walk the > pagelist, I now also keep the pagelist in sorted order. > > Thanks, > jaya > > Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com> > > --- > drivers/video/fb_defio.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > =================================================================== > --- a/drivers/video/fb_defio.c > +++ b/drivers/video/fb_defio.c > @@ -74,6 +74,7 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct > *vma, > { > struct fb_info *info = vma->vm_private_data; > struct fb_deferred_io *fbdefio = info->fbdefio; > + struct page *cur; > > /* this is a callback we get when userspace first tries to > write to the page. we schedule a workqueue. that workqueue > @@ -83,7 +84,24 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct > *vma, > > /* protect against the workqueue changing the page list */ > mutex_lock(&fbdefio->lock); > - list_add(&page->lru, &fbdefio->pagelist); > + > + /* we loop through the pagelist before adding in order > + to keep the pagelist sorted */ > + list_for_each_entry(cur, &fbdefio->pagelist, lru) { > + /* this check is to catch the case where a new > + process could start writing to the same page > + through a new pte. this new access can cause the > + mkwrite even when the original ps's pte is marked > + writable */ > + if (unlikely(cur == page)) > + goto page_already_added; > + else if (cur->index > page->index) > + break; > + } > + > + list_add_tail(&page->lru, &cur->lru); > + > +page_already_added: > mutex_unlock(&fbdefio->lock); > > /* come back after delay to process the deferred IO */ > > >
|  |