Re: [PATCH 1/2] fbdev/defio: Early-out if page is already enlisted

Hi Thomas,

On Thu, Feb 10, 2022 at 03:11:11PM +0100, Thomas Zimmermann wrote:

Return early if a page is already in the list of dirty pages for deferred I/O. This can be detected if the page's list head is not empty. Keep the list head initialized while the page is not enlisted to make this work reliably.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/video/fbdev/core/fb_defio.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c index a591d291b231..3727b1ca87b1 100644 --- a/drivers/video/fbdev/core/fb_defio.c +++ b/drivers/video/fbdev/core/fb_defio.c @@ -59,6 +59,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf) printk(KERN_ERR "no mapping available\n");

BUG_ON(!page->mapping);

• INIT_LIST_HEAD(&page->lru); page->index = vmf->pgoff;

  vmf->page = page;

@@ -122,17 +123,19 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf) */ lock_page(page);

• /*

• * 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.
  

• */
  

When moving this comment it would be prudent to also fix the wording a bit. - Capital in start of sentence and after a period - Spell out process and do not shorten ps

• if (!list_empty(&page->lru))

• goto page_already_added;
  

This check says that if the page already has something in the parge->lru then this is added by defio and thus is already added. This matches your commit description - OK.

Maybe add something like: * Pages added will have their lru set, and it is clered again in the * deferred work handler.

/* 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)
  

• if (cur->index > page->index)
  break;
  

  }

@@ -194,7 +197,7 @@ static void fb_deferred_io_work(struct work_struct *work)

/* clear the list */ list_for_each_safe(node, next, &fbdefio->pagelist) {

• list_del(node);
  

• list_del_init(node);
  

  } mutex_unlock(&fbdefio->lock);

}

With the comment adjusted as you see fit Acked-by: Sam Ravnborg sam@ravnborg.org