| Message ID | 20220210141111.5231-2-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | fbdev: Significantly improve performance of fbdefio mmap | expand |
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>
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 <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 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. 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. Best regards Thomas > > > >> /* 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>
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. + */ + if (!list_empty(&page->lru)) + goto page_already_added; + /* 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); }
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(-)