RE: [PATCH] staging: kpc2000: Convert put_page() to put_user_page*()

[Matt Sickler]

It looks like Outlook is going to absolutely trash this email.  Hopefully it comes through okay.

>> There have been issues with get_user_pages and filesystem writeback.
>> The issues are better described in [1].
>>
>> The solution being proposed wants to keep track of gup_pinned pages
>which will allow to take furthur steps to coordinate between subsystems
>using gup.
>>
>> put_user_page() simply calls put_page inside for now. But the
>implementation will change once all call sites of put_page() are converted.
>>
>> I currently do not have the driver to test. Could I have some suggestions to
>test this code? The solution is currently implemented in [2] and
>> it would be great if we could apply the patch on top of [2] and run some
>tests to check if any regressions occur.
>
>Because this is a common pattern, and because the code here doesn't likely
>need to set page dirty before the dma_unmap_sg call, I think the following
>would be better (it's untested), instead of the above diff hunk:
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index 48ca88bc6b0b..d486f9866449 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -211,16 +211,13 @@ void  transfer_complete_cb(struct aio_cb_data
>*acd, size_t xfr_count, u32 flags)
>        BUG_ON(acd->ldev == NULL);
>        BUG_ON(acd->ldev->pldev == NULL);
>
>-       for (i = 0 ; i < acd->page_count ; i++) {
>-               if (!PageReserved(acd->user_pages[i])) {
>-                       set_page_dirty(acd->user_pages[i]);
>-               }
>-       }
>-
>        dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
>
>        for (i = 0 ; i < acd->page_count ; i++) {
>-               put_page(acd->user_pages[i]);
>+               if (!PageReserved(acd->user_pages[i])) {
>+                       put_user_pages_dirty(&acd->user_pages[i], 1);
>+               else
>+                       put_user_page(acd->user_pages[i]);
>        }
>
>        sg_free_table(&acd->sgt);

I don't think I ever really knew the right way to do this. 

The changes Bharath suggested look okay to me.  I'm not sure about the check for PageReserved(), though.  At first glance it appears to be equivalent to what was there before, but maybe I should learn what that Reserved page flag really means.
From [1], the only comment that seems applicable is
* - MMIO/DMA pages. Some architectures don't allow to ioremap pages that are
 *   not marked PG_reserved (as they might be in use by somebody else who does
 *   not respect the caching strategy).

These pages should be coming from anonymous (RAM, not file backed) memory in userspace.  Sometimes it comes from hugepage backed memory, though I don't think that makes a difference.  I should note that transfer_complete_cb handles both RAM to device and device to RAM DMAs, if that matters.

[1] https://elixir.bootlin.com/linux/v5.2/source/include/linux/page-flags.h#L17