Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite
Hi
Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
Hi Thomas.
On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
Refactor the page-write handler for deferred I/O. Drivers use the
function to let fbdev track written pages of mmap'ed framebuffer
memory.
I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).
FYI I'm going to move the locking into the track-page helper, which
makes the code more readable.
Best regards
Thomas
But if you consider it better keep it. Again just my thoughts when
reading the code.
Sam
v2:
* don't export the helper until we have an external caller
Signed-off-by: Thomas Zimmermann
Reviewed-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_defio.c | 68 -
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index a03b9c64fc61..214581ce5840 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasy
}
EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
-/* vm_ops->page_mkwrite handler */
-static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+/*
+ * Adds a page to the dirty list. Requires caller to hold
+ * struct fb_deferred_io.lock. Call this from struct
+ * vm_operations_struct.page_mkwrite.
+ */
+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned
long offset,
+ struct page *page)
{
- struct page *page = vmf->page;
- struct fb_info *info = vmf->vma->vm_private_data;
struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_pageref *pageref;
- unsigned long offset;
vm_fault_t ret;
- offset = (vmf->address - vmf->vma->vm_start);
-
- /* this is a callback we get when userspace first tries to
- write to the page. we schedule a workqueue. that workqueue
- will eventually mkclean the touched pages and execute the
- deferred framebuffer IO. then if userspace touches a page
- again, we repeat the same scheme */
-
- file_update_time(vmf->vma->vm_file);
-
- /* protect against the workqueue changing the page list */
- mutex_lock(>lock);
-
/* first write in this cycle, notify the driver */
if (fbdefio->first_io && list_empty(>pagelist))
fbdefio->first_io(info);
@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
*/
lock_page(pageref->page);
- mutex_unlock(>lock);
-
/* come back after delay to process the deferred IO */
schedule_delayed_work(>deferred_work, fbdefio->delay);
return VM_FAULT_LOCKED;
@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
return ret;
}
+/*
+ * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
+ * @fb_info: The fbdev info structure
+ * @vmf: The VM fault
+ *
+ * This is a callback we get when userspace first tries to
+ * write to the page. We schedule a workqueue. That workqueue
+ * will eventually mkclean the touched pages and execute the
+ * deferred framebuffer IO. Then if userspace touches a page
+ * again, we repeat the same scheme.
+ *
+ * Returns:
+ * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
+ */
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct
vm_fault *vmf)
+{
+ struct page *page = vmf->page;
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+ unsigned long offset;
+ vm_fault_t ret;
+
+ offset = (vmf->address - vmf->vma->vm_start);
+
+ file_update_time(vmf->vma->vm_file);
+
+ /* protect against the workqueue changing the page list */
+ mutex_lock(>lock);
+ ret = __fb_deferred_io_track_page(info, offset, page);
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+/* vm_ops->page_mkwrite handler */
+static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+{
+ struct fb_info *info = vmf->vma->vm_private_data;
+
+ return fb_deferred_io_page_mkwrite(info, vmf);
+}
+
static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.fault = fb_deferred_io_fault,
.page_mkwrite = fb_deferred_io_mkwrite,
--
2.36.0
--
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
Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite
Hi
Am 25.04.22 um 20:24 schrieb Sam Ravnborg:
Hi Thomas.
On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
Refactor the page-write handler for deferred I/O. Drivers use the
function to let fbdev track written pages of mmap'ed framebuffer
memory.
I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).
But if you consider it better keep it. Again just my thoughts when
reading the code.
The comes from patch of the larger GEM refactoring patches. [1]
fb_deferred_io_page_mkwrite() is later supposed to be exported for use
with DRM.
The patch isn't strictly necessary, but it doesn't do any harm either. I
added it to the patchset, so that I don't have to deal with potential
bit rot later on.
Best regards
Thomas
[1]
https://lore.kernel.org/dri-devel/20220303205839.28484-5-tzimmerm...@suse.de/
Sam
v2:
* don't export the helper until we have an external caller
Signed-off-by: Thomas Zimmermann
Reviewed-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_defio.c | 68 -
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index a03b9c64fc61..214581ce5840 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasy
}
EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
-/* vm_ops->page_mkwrite handler */
-static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+/*
+ * Adds a page to the dirty list. Requires caller to hold
+ * struct fb_deferred_io.lock. Call this from struct
+ * vm_operations_struct.page_mkwrite.
+ */
+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned
long offset,
+ struct page *page)
{
- struct page *page = vmf->page;
- struct fb_info *info = vmf->vma->vm_private_data;
struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_pageref *pageref;
- unsigned long offset;
vm_fault_t ret;
- offset = (vmf->address - vmf->vma->vm_start);
-
- /* this is a callback we get when userspace first tries to
- write to the page. we schedule a workqueue. that workqueue
- will eventually mkclean the touched pages and execute the
- deferred framebuffer IO. then if userspace touches a page
- again, we repeat the same scheme */
-
- file_update_time(vmf->vma->vm_file);
-
- /* protect against the workqueue changing the page list */
- mutex_lock(>lock);
-
/* first write in this cycle, notify the driver */
if (fbdefio->first_io && list_empty(>pagelist))
fbdefio->first_io(info);
@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
*/
lock_page(pageref->page);
- mutex_unlock(>lock);
-
/* come back after delay to process the deferred IO */
schedule_delayed_work(>deferred_work, fbdefio->delay);
return VM_FAULT_LOCKED;
@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
return ret;
}
+/*
+ * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
+ * @fb_info: The fbdev info structure
+ * @vmf: The VM fault
+ *
+ * This is a callback we get when userspace first tries to
+ * write to the page. We schedule a workqueue. That workqueue
+ * will eventually mkclean the touched pages and execute the
+ * deferred framebuffer IO. Then if userspace touches a page
+ * again, we repeat the same scheme.
+ *
+ * Returns:
+ * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
+ */
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct
vm_fault *vmf)
+{
+ struct page *page = vmf->page;
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+ unsigned long offset;
+ vm_fault_t ret;
+
+ offset = (vmf->address - vmf->vma->vm_start);
+
+ file_update_time(vmf->vma->vm_file);
+
+ /* protect against the workqueue changing the page list */
+ mutex_lock(>lock);
+ ret = __fb_deferred_io_track_page(info, offset, page);
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+/* vm_ops->page_mkwrite handler */
+static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+{
+ struct fb_info *info = vmf->vma->vm_private_data;
+
+ return fb_deferred_io_page_mkwrite(info, vmf);
+}
+
static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.fault = fb_deferred_io_fault,
.page_mkwrite = fb_deferred_io_mkwrite,
--
2.36.0
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite
Hi Thomas.
On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
> Refactor the page-write handler for deferred I/O. Drivers use the
> function to let fbdev track written pages of mmap'ed framebuffer
> memory.
I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).
But if you consider it better keep it. Again just my thoughts when
reading the code.
Sam
>
> v2:
> * don't export the helper until we have an external caller
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/video/fbdev/core/fb_defio.c | 68 -
> 1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index a03b9c64fc61..214581ce5840 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t
> start, loff_t end, int datasy
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>
> -/* vm_ops->page_mkwrite handler */
> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +/*
> + * Adds a page to the dirty list. Requires caller to hold
> + * struct fb_deferred_io.lock. Call this from struct
> + * vm_operations_struct.page_mkwrite.
> + */
> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned
> long offset,
> + struct page *page)
> {
> - struct page *page = vmf->page;
> - struct fb_info *info = vmf->vma->vm_private_data;
> struct fb_deferred_io *fbdefio = info->fbdefio;
> struct fb_deferred_io_pageref *pageref;
> - unsigned long offset;
> vm_fault_t ret;
>
> - offset = (vmf->address - vmf->vma->vm_start);
> -
> - /* this is a callback we get when userspace first tries to
> - write to the page. we schedule a workqueue. that workqueue
> - will eventually mkclean the touched pages and execute the
> - deferred framebuffer IO. then if userspace touches a page
> - again, we repeat the same scheme */
> -
> - file_update_time(vmf->vma->vm_file);
> -
> - /* protect against the workqueue changing the page list */
> - mutex_lock(>lock);
> -
> /* first write in this cycle, notify the driver */
> if (fbdefio->first_io && list_empty(>pagelist))
> fbdefio->first_io(info);
> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
> *vmf)
>*/
> lock_page(pageref->page);
>
> - mutex_unlock(>lock);
> -
> /* come back after delay to process the deferred IO */
> schedule_delayed_work(>deferred_work, fbdefio->delay);
> return VM_FAULT_LOCKED;
> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
> *vmf)
> return ret;
> }
>
> +/*
> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
> + * @fb_info: The fbdev info structure
> + * @vmf: The VM fault
> + *
> + * This is a callback we get when userspace first tries to
> + * write to the page. We schedule a workqueue. That workqueue
> + * will eventually mkclean the touched pages and execute the
> + * deferred framebuffer IO. Then if userspace touches a page
> + * again, we repeat the same scheme.
> + *
> + * Returns:
> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
> + */
> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct
> vm_fault *vmf)
> +{
> + struct page *page = vmf->page;
> + struct fb_deferred_io *fbdefio = info->fbdefio;
> + unsigned long offset;
> + vm_fault_t ret;
> +
> + offset = (vmf->address - vmf->vma->vm_start);
> +
> + file_update_time(vmf->vma->vm_file);
> +
> + /* protect against the workqueue changing the page list */
> + mutex_lock(>lock);
> + ret = __fb_deferred_io_track_page(info, offset, page);
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
> +/* vm_ops->page_mkwrite handler */
> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +{
> + struct fb_info *info = vmf->vma->vm_private_data;
> +
> + return fb_deferred_io_page_mkwrite(info, vmf);
> +}
> +
> static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> .fault = fb_deferred_io_fault,
> .page_mkwrite = fb_deferred_io_mkwrite,
> --
> 2.36.0
[PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite
Refactor the page-write handler for deferred I/O. Drivers use the
function to let fbdev track written pages of mmap'ed framebuffer
memory.
v2:
* don't export the helper until we have an external caller
Signed-off-by: Thomas Zimmermann
Reviewed-by: Javier Martinez Canillas
---
drivers/video/fbdev/core/fb_defio.c | 68 -
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index a03b9c64fc61..214581ce5840 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasy
}
EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
-/* vm_ops->page_mkwrite handler */
-static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+/*
+ * Adds a page to the dirty list. Requires caller to hold
+ * struct fb_deferred_io.lock. Call this from struct
+ * vm_operations_struct.page_mkwrite.
+ */
+static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned
long offset,
+ struct page *page)
{
- struct page *page = vmf->page;
- struct fb_info *info = vmf->vma->vm_private_data;
struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_pageref *pageref;
- unsigned long offset;
vm_fault_t ret;
- offset = (vmf->address - vmf->vma->vm_start);
-
- /* this is a callback we get when userspace first tries to
- write to the page. we schedule a workqueue. that workqueue
- will eventually mkclean the touched pages and execute the
- deferred framebuffer IO. then if userspace touches a page
- again, we repeat the same scheme */
-
- file_update_time(vmf->vma->vm_file);
-
- /* protect against the workqueue changing the page list */
- mutex_lock(>lock);
-
/* first write in this cycle, notify the driver */
if (fbdefio->first_io && list_empty(>pagelist))
fbdefio->first_io(info);
@@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
*/
lock_page(pageref->page);
- mutex_unlock(>lock);
-
/* come back after delay to process the deferred IO */
schedule_delayed_work(>deferred_work, fbdefio->delay);
return VM_FAULT_LOCKED;
@@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
*vmf)
return ret;
}
+/*
+ * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
+ * @fb_info: The fbdev info structure
+ * @vmf: The VM fault
+ *
+ * This is a callback we get when userspace first tries to
+ * write to the page. We schedule a workqueue. That workqueue
+ * will eventually mkclean the touched pages and execute the
+ * deferred framebuffer IO. Then if userspace touches a page
+ * again, we repeat the same scheme.
+ *
+ * Returns:
+ * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
+ */
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct
vm_fault *vmf)
+{
+ struct page *page = vmf->page;
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+ unsigned long offset;
+ vm_fault_t ret;
+
+ offset = (vmf->address - vmf->vma->vm_start);
+
+ file_update_time(vmf->vma->vm_file);
+
+ /* protect against the workqueue changing the page list */
+ mutex_lock(>lock);
+ ret = __fb_deferred_io_track_page(info, offset, page);
+ mutex_unlock(>lock);
+
+ return ret;
+}
+
+/* vm_ops->page_mkwrite handler */
+static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
+{
+ struct fb_info *info = vmf->vma->vm_private_data;
+
+ return fb_deferred_io_page_mkwrite(info, vmf);
+}
+
static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.fault = fb_deferred_io_fault,
.page_mkwrite = fb_deferred_io_mkwrite,
--
2.36.0
