Messages in this thread | | | Subject | Re: [PATCH 1/2] mm: introduce put_user_page*(), placeholder versions | From | John Hubbard <> | Date | Tue, 15 Jan 2019 13:39:17 -0800 |
| |
On 1/15/19 12:34 AM, Jan Kara wrote: > On Mon 14-01-19 11:09:20, John Hubbard wrote: >> On 1/14/19 9:21 AM, Jerome Glisse wrote: >>>> [...] > >> For example, the following already survives a basic boot to graphics mode. >> It requires a bunch of callsite conversions, and a page flag (neither of which >> is shown here), and may also have "a few" gross conceptual errors, but take a >> peek: > > Thanks for writing this down! Some comments inline. >
I appreciate your taking a look at this, Jan. I'm still pretty new to gup.c, so it's really good to get an early review.
>> +/* >> + * Manages the PG_gup_pinned flag. >> + * >> + * Note that page->_mapcount counting part of managing that flag, because the >> + * _mapcount is used to determine if PG_gup_pinned can be cleared, in >> + * page_mkclean(). >> + */ >> +static void track_gup_page(struct page *page) >> +{ >> + page = compound_head(page); >> + >> + lock_page(page); >> + >> + wait_on_page_writeback(page); > > ^^ I'd use wait_for_stable_page() here. That is the standard waiting > mechanism to use before you allow page modification.
OK, will do. In fact, I initially wanted to use wait_for_stable_page(), but hesitated when I saw that it won't necessarily do wait_on_page_writeback(), and I then I also remembered Dave Chinner recently mentioned that the policy decision needed some thought in the future (maybe something about block device vs. filesystem policy):
void wait_for_stable_page(struct page *page) { if (bdi_cap_stable_pages_required(inode_to_bdi(page->mapping->host))) wait_on_page_writeback(page); }
...but like you say, it's the standard way that fs does this, so we should just use it.
> >> + >> + atomic_inc(&page->_mapcount); >> + SetPageGupPinned(page); >> + >> + unlock_page(page); >> +} >> + >> +/* >> + * A variant of track_gup_page() that returns -EBUSY, instead of waiting. >> + */ >> +static int track_gup_page_atomic(struct page *page) >> +{ >> + page = compound_head(page); >> + >> + if (PageWriteback(page) || !trylock_page(page)) >> + return -EBUSY; >> + >> + if (PageWriteback(page)) { >> + unlock_page(page); >> + return -EBUSY; >> + } > > Here you'd need some helper that would return whether > wait_for_stable_page() is going to wait. Like would_wait_for_stable_page() > but maybe you can come up with a better name.
Yes, in order to wait_for_stable_page(), that seems necessary, I agree.
thanks, -- John Hubbard NVIDIA
| |