Re: [RFC PATCH 0/3] page count lock for simpler put_page

[Minchan Kim <minchan.kim@gmail.com>]

On Fri, Aug 12, 2011 at 03:35:25PM -0700, Michel Lespinasse wrote:
> On Tue, Aug 9, 2011 at 3:22 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> > On Tue, Aug 9, 2011 at 8:04 PM, Michel Lespinasse <walken@google.com> wrote:
> >> On Sun, Aug 7, 2011 at 7:25 AM, Minchan Kim <minchan.kim@gmail.com> wrote:
> >>> On Thu, Aug 04, 2011 at 11:39:19PM -0700, Michel Lespinasse wrote:
> >>>> I can see that the page_cache_get_speculative comment in
> >>>> include/linux/pagemap.h maps out one way to prevent the issue. If
> >>>> thread T continually held an rcu read lock from the time it finds the
> >>>> pointer to P until the time it calls get_page_unless_zero on that
> >>>> page, AND there was a synchronize_rcu() call somewhere between the
> >>>> time a THP page gets allocated and the time __split_huge_page_refcount
> >>>> might first get called on that page, then things would be safe.
> >>>> However, that does not seem to be true today: I could not find a
> >>>> synchronize_rcu() call before __split_huge_page_refcount(), AND there
> >>>> are also places (such as deactivate_page() for example) that call
> >>>> get_page_unless_zero without being within an rcu read locked section
> >>>> (or holding the zone lru lock to provide exclusion against
> >>>> __split_huge_page_refcount).
> >>
> >> Going forward, I can see several possible solutions:
> >> - Use my proposed page count lock in order to avoid the race. One
> >> would have to convert all get_page_unless_zero() sites to use it. I
> >> expect the cost would be low but still measurable.
> >
> > It's not necessary to apply it on *all* get_page_unless_zero sites.
> > Because deactivate_page does it on file pages while THP handles only anon pages.
> > So the race should not a problem.
> 
> But it doesn't matter what kind of page the get_page_unless_zero call
> site hopes to get a reference on - if it doesn't already hold a
> reference on the page (either directly as a reference, or if a known
> mapping points to that page and the page table lock is taken or
> interrupts are disabled in order to guarantee the mapping won't get
> yanked), then the page can get yanked and a THP page could show up
> there before the call site gets a reference.

As I said, the caller of deactivate hold a reference at now so it should be okay.
But I admit deactivate_page doesn't have to call get_page_unless_zero but get_page
is enough if caller makes sure to hold a reference on the page like current situation.
I will add such comment on the deactivate_page and change get_page_unless_zero with
get_page for easy use in future.

I have't notice that.
Thanks for giving the chance to think of it, Michel.

> 
> >> - Protect all get_page_unless_zero call sites with rcu read lock or
> >> lru lock (page_cache_get_speculative already has it, but there are
> >> others to consider), and add a synchronize_rcu() before splitting huge
> >> pages.
> >
> > I think it can't be a solution.
> > If we don't have any lock for protect write-side, page_count could be
> > unstable again while we peek page->count in
> > __split_huge_page_refcount after calling synchronize_rcu.
> > Do I miss something?
> 
> The tail page count would be unstable for at most one rcu grace period
> after the page got allocated. This is guaranteed by making all
> get_page_unless_zero call sites make sure they somehow determine the
> page is not a THP tail page (for example because they found it in
> radix tree) before calling get_page_unless_zero and having an rcu read
> lock wrapping these two together. This is basically the protocol
> described in the comment for page_cache_get_speculative() in pagemap.h

Absolutely

> 
> -- 
> Michel "Walken" Lespinasse
> A program is never fully debugged until the last user dies.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>