lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 1/1] mm: introduce put_user_page*(), placeholder versions
    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@gmail.com wrote:
    > > > From: John Hubbard <jhubbard@nvidia.com>
    >
    > [...]
    >
    > > > diff --git a/mm/gup.c b/mm/gup.c
    > > > index f84e22685aaa..37085b8163b1 100644
    > > > --- a/mm/gup.c
    > > > +++ b/mm/gup.c
    > > > @@ -28,6 +28,88 @@ struct follow_page_context {
    > > > unsigned int page_mask;
    > > > };
    > > >
    > > > +typedef int (*set_dirty_func_t)(struct page *page);
    > > > +
    > > > +static void __put_user_pages_dirty(struct page **pages,
    > > > + unsigned long npages,
    > > > + set_dirty_func_t sdf)
    > > > +{
    > > > + unsigned long index;
    > > > +
    > > > + for (index = 0; index < npages; index++) {
    > > > + struct page *page = compound_head(pages[index]);
    > > > +
    > > > + if (!PageDirty(page))
    > > > + sdf(page);
    > >
    > > How is this safe? What prevents the page to be cleared under you?
    > >
    > > If it's safe to race clear_page_dirty*() it has to be stated explicitly
    > > with a reason why. It's not very clear to me as it is.
    >
    > The PageDirty() optimization above is fine to race with clear the
    > page flag as it means it is racing after a page_mkclean() and the
    > GUP user is done with the page so page is about to be write back
    > ie if (!PageDirty(page)) see the page as dirty and skip the sdf()
    > call while a split second after TestClearPageDirty() happens then
    > it means the racing clear is about to write back the page so all
    > is fine (the page was dirty and it is being clear for write back).
    >
    > If it does call the sdf() while racing with write back then we
    > just redirtied the page just like clear_page_dirty_for_io() would
    > do if page_mkclean() failed so nothing harmful will come of that
    > neither. Page stays dirty despite write back it just means that
    > the page might be write back twice in a row.

    Fair enough. Should we get it into a comment here?

    > > > +void put_user_pages(struct page **pages, unsigned long npages)
    > > > +{
    > > > + unsigned long index;
    > > > +
    > > > + for (index = 0; index < npages; index++)
    > > > + put_user_page(pages[index]);
    > >
    > > I believe there's an room for improvement for compound pages.
    > >
    > > If there's multiple consequential pages in the array that belong to the
    > > same compound page we can get away with a single atomic operation to
    > > handle them all.
    >
    > Yes maybe just add a comment with that for now and leave this kind of
    > optimization to latter ?

    Sounds good to me.

    --
    Kirill A. Shutemov

    \
     
     \ /
      Last update: 2019-03-19 15:06    [W:2.662 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site