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

[Thomas Zimmermann]

Hi Sam

Am 10.02.22 um 22:00 schrieb Sam Ravnborg:

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 <tzimmerm...@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");

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

@@ -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.
+        */

BUG_ON(!page->mapping); vmf->page = page; + /*

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

Ok.

+       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.

Best regards
Thomas

I'll add a related TODO to the comment because we actually want to remove the use of the lru field. It's owned by the page cache. I already have a prototype patch that implements the page tracking with a separate list.

        /* 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;
        }

        list_for_each_safe(node, next, &fbdefio->pagelist) {
-               list_del(node);
+               list_del_init(node);
        }
        mutex_unlock(&fbdefio->lock);
  }

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

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

OpenPGP_signature
Description: OpenPGP digital signature