[PATCH] fb_defio: fix for non-dirty ptes

[PATCH] fb_defio: fix for non-dirty ptes

[Albert Herranz]

This patch fixes a problem observed while using fb_defio with a short
delay on a PowerPC platform.

It is possible that page_mkclean() is invoked in the deferred io work
function _before_ a PTE has been marked dirty. In this case, the page
is removed from the defio pagelist but page_mkclean() does not
write-protect the page again. The end result is that defio ignores all
subsequent writes to the page and the corresponding portions of the
framebuffer never get updated.

The fix consists in keeping track of the pages with non-dirty PTEs,
re-checking them again on the next deferred io work iteration.
Note that those pages are not passed to the defio callback as they are
not written by userspace yet.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
---
 drivers/video/fb_defio.c |   40 ++++++++++++++++++++++++++++++++--------
 1 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 6113c47..1105a59 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -155,25 +155,41 @@ static void fb_deferred_io_work(struct work_struct *work)
 {
 	struct fb_info *info = container_of(work, struct fb_info,
 						deferred_work.work);
-	struct list_head *node, *next;
-	struct page *cur;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct page *page, *tmp_page;
+	struct list_head *node, *tmp_node;
+	struct list_head non_dirty;
+
+	INIT_LIST_HEAD(&non_dirty);
 
 	/* here we mkclean the pages, then do all deferred IO */
 	mutex_lock(&fbdefio->lock);
-	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
-		lock_page(cur);
-		page_mkclean(cur);
-		unlock_page(cur);
+	list_for_each_entry_safe(page, tmp_page, &fbdefio->pagelist, lru) {
+		lock_page(page);
+		/*
+		 * The workqueue callback can be triggered after a
+		 * ->page_mkwrite() call but before the PTE has been marked
+		 * dirty. In this case page_mkclean() won't "rearm" the page.
+		 *
+		 * To avoid this, remove those "non-dirty" pages from the
+		 * pagelist before calling the driver's callback, then add
+		 * them back to get processed on the next work iteration.
+		 * At that time, their PTEs will hopefully be dirty for real.
+		 */
+		if (!page_mkclean(page))
+			list_move_tail(&page->lru, &non_dirty);
+		unlock_page(page);
 	}
 
 	/* driver's callback with pagelist */
 	fbdefio->deferred_io(info, &fbdefio->pagelist);
 
-	/* clear the list */
-	list_for_each_safe(node, next, &fbdefio->pagelist) {
+	/* clear the list... */
+	list_for_each_safe(node, tmp_node, &fbdefio->pagelist) {
 		list_del(node);
 	}
+	/* ... and add back the "non-dirty" pages to the list */
+	list_splice_tail(&non_dirty, &fbdefio->pagelist);
 	mutex_unlock(&fbdefio->lock);
 }
 
@@ -202,6 +218,7 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_open);
 void fb_deferred_io_cleanup(struct fb_info *info)
 {
 	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct list_head *node, *tmp_node;
 	struct page *page;
 	int i;
 
@@ -209,6 +226,13 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	cancel_delayed_work(&info->deferred_work);
 	flush_scheduled_work();
 
+	/*  the list may have still some non-dirty pages at this point */
+	mutex_lock(&fbdefio->lock);
+	list_for_each_safe(node, tmp_node, &fbdefio->pagelist) {
+		list_del(node);
+	}
+	mutex_unlock(&fbdefio->lock);
+
 	/* clear out the mapping that we setup */
 	for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
 		page = fb_deferred_io_page(info, i);
-- 
1.6.3.3

Re: [PATCH] fb_defio: fix for non-dirty ptes

[Jaya Kumar]

On Thu, May 13, 2010 at 4:51 AM, Albert Herranz <albert_herranz@yahoo.es> wrote:
> This patch fixes a problem observed while using fb_defio with a short
> delay on a PowerPC platform.
>
> It is possible that page_mkclean() is invoked in the deferred io work
> function _before_ a PTE has been marked dirty. In this case, the page
> is removed from the defio pagelist but page_mkclean() does not
> write-protect the page again. The end result is that defio ignores all
> subsequent writes to the page and the corresponding portions of the
> framebuffer never get updated.
>
> The fix consists in keeping track of the pages with non-dirty PTEs,
> re-checking them again on the next deferred io work iteration.
> Note that those pages are not passed to the defio callback as they are
> not written by userspace yet.
>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>

Looks good to me. Thanks for the bugfix.

Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>

Re: [PATCH] fb_defio: fix for non-dirty ptes

[Andrew Morton]

On Thu, 13 May 2010 07:34:11 +0800
Jaya Kumar <jayakumar.lkml@gmail.com> wrote:

> On Thu, May 13, 2010 at 4:51 AM, Albert Herranz <albert_herranz@yahoo.es> wrote:
> > This patch fixes a problem observed while using fb_defio with a short
> > delay on a PowerPC platform.
> >
> > It is possible that page_mkclean() is invoked in the deferred io work
> > function _before_ a PTE has been marked dirty. In this case, the page
> > is removed from the defio pagelist but page_mkclean() does not
> > write-protect the page again. The end result is that defio ignores all
> > subsequent writes to the page and the corresponding portions of the
> > framebuffer never get updated.
> >
> > The fix consists in keeping track of the pages with non-dirty PTEs,
> > re-checking them again on the next deferred io work iteration.
> > Note that those pages are not passed to the defio callback as they are
> > not written by userspace yet.
> >
> > Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> 
> Looks good to me. Thanks for the bugfix.
> 
> Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>

For some reason my linux-fbdev feed is being stupid and I don't have a
copy of the patch.  Resend, please?

Re: [PATCH] fb_defio: fix for non-dirty ptes

[Albert Herranz]

Hi,

[CC'ing linux-fbdev]

On 05/19/2010 03:36 PM, Jenkins, Clive wrote:
> Hi Albert
> 
> I recently added deferred I/O to my framebuffer driver, and encountered
> the problem of pages getting stuck in a dirty state. A few days later
> your patch came out, and the problem appeared to be solved. However, I
> still get the problem when doing intensive writing to the FB, using the
> attached program fb_anim that I hacked up.
> 
> After copying a test image to the FB, I have been using the command
> with these arguments:
> 
>  fb_anim -x 13500000 400 480 -1 4 0
> 
> on a 800x480x16-bit FB, which does this:-
> 
>   set initial POSITION to centre screen
> loop:
>   draw a 400x480 rectangle at POSITION by XORing every pixel with -1
>   wait 13.5ms
>   wait 13.5ms
>   draw a 400x480 rectangle at POSITION by XOR (restore)
>   move POSITION by (4,0)
>   if not SIGINT goto loop
> 
> Note that my frame refresh perios is 13.5ms, so the rectangle moves 4
> pixels to the right every 2 frames.
> 
> If I then kill it with ^C, some or all of the rectangle remains XORed
> on the LCD screen, although the data in /dev/fb0 has been restored.
> If I then write a single pixel using
> 
>  echo a >/dev/fb0
> 
> the first page (2.56 lines of pixels) gets restored on the LCD. If I
> do
> 
>  cp /dev/fb0 t
>  cp t /dev/fb0 
> 
> the entire image is restored.
> 

From your description, it may be that the very last deferred io work iteration doesn't take place when your application exits.
This could happen if fb_deferred_io_cleanup() is called when the framebuffer device is closed. Currently, fb_deferred_io_cleanup() doesn't guarantee that a pending deferred io work takes place (the function cancels any pending deferred io work, and the work may get executed or not).

Does your framebuffer driver has a fb_release hook? And do you call fb_deferred_io_cleanup() from there?
If that's the case, does this patch solve the problem?

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -223,8 +223,7 @@ void fb_deferred_io_cleanup(struct fb_info *info)
 	int i;
 
 	BUG_ON(!fbdefio);
-	cancel_delayed_work(&info->deferred_work);
-	flush_scheduled_work();
+	flush_delayed_work(&info->deferred_work);
 
 	/*  the list may have still some non-dirty pages at this point */
 	mutex_lock(&fbdefio->lock);

> I wondered if you could duplicate my findings on whatever platform(s)
> you have. I am using a Freescale MPC8536 (e500v2 core) with a custom
> ASIC (PCI device) containing an LCD controller which has graphics RAM
> attached. [Writing from the CPU through the ASIC into graphics RAM is
> slow, hence the need for FB in system RAM, and defio to graphics RAM
> being done using the 8536's DMA engine, which does efficient burst
> writes.]  I am using a 2.6.27 kernel with your patch applied, HZ%0,
> defio_delay=1 jiffy (4ms). After applying your patch, my normal
> application software worked normally, but my test program still had
> the problem, as described above.
> 

I'm working with a Nintendo Wii which has a non-standard framebuffer format (4:2:2 YCbCr). I'm using fb deferred io to provide a RGB framebuffer abstraction for applications to run unmodified (at the expense of some performance penalty).

I haven't experienced your problem, but I do not call fb_deferred_io_cleanup() until the driver remove hook. And I also use some fb hooks (fillrect, copyarea, imageblit, write) which fbcon uses to update the framebuffer.

> I would be grateful for any help solving this problem.
> 
> Thanks,
> Clive

Cheers,
Albert