Re: [PATCH RFC 16/19] mm/frame-vector: remove FOLL_FORCE usage

[David Hildenbrand]

On 08.11.22 05:45, Tomasz Figa wrote:
> Hi David,

Hi Tomasz,

thanks for looking into this!

> On Tue, Nov 8, 2022 at 1:19 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > FOLL_FORCE is really only for debugger access. According to commit
> > 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are always
> > writable"), the pinned pages are always writable.

As reference, the cover letter of the series:

https://lkml.kernel.org/r/20221107161740.144456-1-david@xxxxxxxxxx

> Actually that patch is only a workaround to temporarily disable
> support for read-only pages as they seemed to suffer from some
> corruption issues in the retrieved user pages. We expect to support
> read-only pages as hardware input after. That said, FOLL_FORCE doesn't
> sound like the right thing even in that case, but I don't know the
> background behind it being added here in the first place. +Hans
> Verkuil +Marek Szyprowski do you happen to remember anything about it?

Maybe I mis-interpreted 707947247e95; re-reading it again, I am not 
quite sure what the actual problem is and how it relates to GUP 
FOLL_WRITE handling. FOLL_FORCE already was in place before 707947247e95 
and should be removed.

What I understood is "Just always call vb2_create_framevec() with 
FOLL_WRITE to always allow writing to the buffers.".


If the pinned page is never written to via the obtained GUP reference:
* FOLL_WRITE should not be set
* FOLL_FORCE should not be set
* We should not dirty the page when unpinning

If the pinned page may be written to via the obtained GUP reference:
* FOLL_WRITE should be set
* FOLL_FORCE should not be set
* We should dirty the page when unpinning


If the function is called for both, we should think about doing it 
conditional based on a "write" variable, like pre-707947247e95 did.

@Hans, any insight?

--
Thanks,

David / dhildenb