diff mbox series

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

Message ID 20220210141111.5231-2-tzimmermann@suse.de
State New
Headers show
Series fbdev: Significantly improve performance of fbdefio mmap | expand

Commit Message

Thomas Zimmermann Feb. 10, 2022, 2:11 p.m. UTC
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(-)

Comments

Sam Ravnborg Feb. 10, 2022, 9 p.m. UTC | #1
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>
Thomas Zimmermann Feb. 11, 2022, 9:12 a.m. UTC | #2
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 mbox series

Patch

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);
 }