Re: "fb-defio: fix page list with concurrent processes"

[Jaya Kumar]

On Mon, Jun 16, 2008 at 3:05 PM, Jeremy Fitzhardinge <jeremy@xxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> ---
> 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 */
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/