lkml.org 
[lkml]   [2020]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] docs: mm/gup: pin_user_pages.rst: add a "case 5"
From
Date
On 2020-06-12 12:24, Matthew Wilcox wrote:
> On Fri, May 29, 2020 at 04:43:08PM -0700, John Hubbard wrote:
>> +CASE 5: Pinning in order to write to the data within the page
>> +-------------------------------------------------------------
>> +Even though neither DMA nor Direct IO is involved, just a simple case of "pin,
>> +access page's data, unpin" can cause a problem. Case 5 may be considered a
>> +superset of Case 1, plus Case 2, plus anything that invokes that pattern. In
>> +other words, if the code is neither Case 1 nor Case 2, it may still require
>> +FOLL_PIN, for patterns like this:
>> +
>> +Correct (uses FOLL_PIN calls):
>> + pin_user_pages()
>> + access the data within the pages
>> + set_page_dirty_lock()
>> + unpin_user_pages()
>> +
>> +INCORRECT (uses FOLL_GET calls):
>> + get_user_pages()
>> + access the data within the pages
>> + set_page_dirty_lock()
>> + put_page()
>
> Why does this case need to pin? Why can't it just do ...
>
> get_user_pages()
> lock_page(page);
> ... modify the data ...
> set_page_dirty(page);
> unlock_page(page);
>

Yes, it could do that. And that would also make a good additional "correct"
example. Especially for the case of just dealing with a single page,
lock_page() has the benefit of completely fixing the problem *today*,
without waiting for the pin_user_pages*() handling improvements to get
implemented.

And it's also another (probably better) way to fix the vhost.c problem, than
commit 690623e1b496 ("vhost: convert get_user_pages() --> pin_user_pages()").

I'm inclined to leave vhost.c alone for now, unless someone really prefers
it to be changed, but to update the Case 5 documentation with your point
above. Sound about right?


thanks,
--
John Hubbard
NVIDIA

\
 
 \ /
  Last update: 2020-06-12 22:04    [W:0.077 / U:0.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site