Re: [PATCH v4 1/1] mm: introduce put_user_page*(), placeholder versions

[John Hubbard]

On 3/19/19 8:36 AM, Jan Kara wrote:
> On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote:
>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote:
>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote:
>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote:
>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@xxxxxxxxx wrote:
>>>>>> From: John Hubbard <jhubbard@xxxxxxxxxx>
>>>> [...]
>>> Forgot to mention one thing, we had a discussion with Andrea and Jan
>>> about set_page_dirty() and Andrea had the good idea of maybe doing
>>> the set_page_dirty() at GUP time (when GUP with write) not when the
>>> GUP user calls put_page(). We can do that by setting the dirty bit
>>> in the pte for instance. They are few bonus of doing things that way:
>>>     - amortize the cost of calling set_page_dirty() (ie one call for
>>>       GUP and page_mkclean()
>>>     - it is always safe to do so at GUP time (ie the pte has write
>>>       permission and thus the page is in correct state)
>>>     - safe from truncate race
>>>     - no need to ever lock the page
>>>
>>> Extra bonus from my point of view, it simplify thing for my generic
>>> page protection patchset (KSM for file back page).
>>>
>>> So maybe we should explore that ? It would also be a lot less code.
>>
>> Yes, please. It sounds more sensible to me to dirty the page on get, not
>> on put.
> 
> I fully agree this is a desirable final state of affairs. And with changes
> to how we treat pinned pages during writeback there won't have to be any
> explicit dirtying at all in the end because the page is guaranteed to be
> dirty after a write page fault and pin would make sure it stays dirty until
> unpinned. However initially I want the helpers to be as close to code they
> are replacing as possible. Because it will be hard to catch all the bugs
> due to driver conversions even in that situation. So I still think that
> these helpers as they are a good first step. Then we need to convert
> GUP users to use them and then it is much easier to modify the behavior
> since it is no longer opencoded in two hudred or how many places...
> 
> 								Honza

In fact, we had this very same question come up last month [1]: I was also
wondering if we should just jump directly to the final step, and not
do the dirtying call, but it is true that during the conversion process,
(which effectively wraps put_page(), without changing anything else),
it's safer to avoid changing things. 

The whole system is fragile because it's running something that has some 
latent bugs in this area, so probably best to do it the way Jan says, and 
avoid causing any new instances of reproducing this problem, even though 
there is a bit more churn involved.


[1] https://lore.kernel.org/r/20190205112107.GB3872@xxxxxxxxxxxxxx

thanks,
-- 
John Hubbard
NVIDIA