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 tzimmermann@suse.de Reviewed-by: Javier Martinez Canillas javierm@redhat.com

---

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(&fbdefio->lock);

• /* first write in this cycle, notify the driver */ if (fbdefio->first_io && list_empty(&fbdefio->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(&fbdefio->lock);
• /* come back after delay to process the deferred IO */ schedule_delayed_work(&info->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(&fbdefio->lock);
• ret = __fb_deferred_io_track_page(info, offset, page);
• mutex_unlock(&fbdefio->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