Date:   Mon, 14 Jan 2019 12:21:25 -0500
From:   Jerome Glisse <jglisse@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     John Hubbard <jhubbard@...dia.com>,
        Matthew Wilcox <willy@...radead.org>,
        Dave Chinner <david@...morbit.com>,
        Dan Williams <dan.j.williams@...el.com>,
        John Hubbard <john.hubbard@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>, tom@...pey.com,
        Al Viro <viro@...iv.linux.org.uk>, benve@...co.com,
        Christoph Hellwig <hch@...radead.org>,
        Christopher Lameter <cl@...ux.com>,
        "Dalessandro, Dennis" <dennis.dalessandro@...el.com>,
        Doug Ledford <dledford@...hat.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Michal Hocko <mhocko@...nel.org>, mike.marciniszyn@...el.com,
        rcampbell@...dia.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mm: introduce put_user_page*(), placeholder versions

On Mon, Jan 14, 2019 at 03:54:47PM +0100, Jan Kara wrote:
> On Fri 11-01-19 19:06:08, John Hubbard wrote:
> > On 1/11/19 6:46 PM, Jerome Glisse wrote:
> > > On Fri, Jan 11, 2019 at 06:38:44PM -0800, John Hubbard wrote:
> > > [...]
> > > 
> > >>>> The other idea that you and Dan (and maybe others) pointed out was a debug
> > >>>> option, which we'll certainly need in order to safely convert all the call
> > >>>> sites. (Mirror the mappings at a different kernel offset, so that put_page()
> > >>>> and put_user_page() can verify that the right call was made.)  That will be
> > >>>> a separate patchset, as you recommended.
> > >>>>
> > >>>> I'll even go as far as recommending the page lock itself. I realize that this 
> > >>>> adds overhead to gup(), but we *must* hold off page_mkclean(), and I believe
> > >>>> that this (below) has similar overhead to the notes above--but is *much* easier
> > >>>> to verify correct. (If the page lock is unacceptable due to being so widely used,
> > >>>> then I'd recommend using another page bit to do the same thing.)
> > >>>
> > >>> Please page lock is pointless and it will not work for GUP fast. The above
> > >>> scheme do work and is fine. I spend the day again thinking about all memory
> > >>> ordering and i do not see any issues.
> > >>>
> > >>
> > >> Why is it that page lock cannot be used for gup fast, btw?
> > > 
> > > Well it can not happen within the preempt disable section. But after
> > > as a post pass before GUP_fast return and after reenabling preempt then
> > > it is fine like it would be for regular GUP. But locking page for GUP
> > > is also likely to slow down some workload (with direct-IO).
> > > 
> > 
> > Right, and so to crux of the matter: taking an uncontended page lock
> > involves pretty much the same set of operations that your approach does.
> > (If gup ends up contended with the page lock for other reasons than these
> > paths, that seems surprising.) I'd expect very similar performance.
> > 
> > But the page lock approach leads to really dramatically simpler code (and
> > code reviews, let's not forget). Any objection to my going that
> > direction, and keeping this idea as a Plan B? I think the next step will
> > be, once again, to gather some performance metrics, so maybe that will
> > help us decide.
> 
> FWIW I agree that using page lock for protecting page pinning (and thus
> avoid races with page_mkclean()) looks simpler to me as well and I'm not
> convinced there will be measurable difference to the more complex scheme
> with barriers Jerome suggests unless that page lock contended. Jerome is
> right that you cannot just do lock_page() in gup_fast() path. There you
> have to do trylock_page() and if that fails just bail out to the slow gup
> path.
> 
> Regarding places other than page_mkclean() that need to check pinned state:
> Definitely page migration will want to check whether the page is pinned or
> not so that it can deal differently with short-term page references vs
> longer-term pins.
> 
> Also there is one more idea I had how to record number of pins in the page:
> 
> #define PAGE_PIN_BIAS	1024
> 
> get_page_pin()
> 	atomic_add(&page->_refcount, PAGE_PIN_BIAS);
> 
> put_page_pin();
> 	atomic_add(&page->_refcount, -PAGE_PIN_BIAS);
> 
> page_pinned(page)
> 	(atomic_read(&page->_refcount) - page_mapcount(page)) > PAGE_PIN_BIAS
> 
> This is pretty trivial scheme. It still gives us 22-bits for page pins
> which should be plenty (but we should check for that and bail with error if
> it would overflow). Also there will be no false negatives and false
> positives only if there are more than 1024 non-page-table references to the
> page which I expect to be rare (we might want to also subtract
> hpage_nr_pages() for radix tree references to avoid excessive false
> positives for huge pages although at this point I don't think they would
> matter). Thoughts?

Racing PUP are as likely to cause issues:

CPU0                        | CPU1       | CPU2
                            |            |
                            | PUP()      |
    page_pinned(page)       |            |
      (page_count(page) -   |            |
       page_mapcount(page)) |            |
                            |            | GUP()

So here the refcount snap-shot does not include the second GUP and
we can have a false negative ie the page_pinned() will return false
because of the PUP happening just before on CPU1 despite the racing
GUP on CPU2 just after.

I believe only either lock or memory ordering with barrier can
guarantee that we do not miss GUP ie no false negative. Still the
bias idea might be usefull as with it we should not need a flag.

So to make the above safe it would still need the page write back
double check that i described so that GUP back-off if it raced with
page_mkclean,clear_page_dirty_for_io and the fs write page call back
which call test_set_page_writeback() (yes it is very unlikely but
might still happen).


I still need to ponder some more on all the races.


Cheers,
Jérôme

Powered by blists - more mailing lists