On 27.02.20 06:58, John Hubbard wrote: > On 2/26/20 7:11 PM, Stephen Rothwell wrote: >> Hi all, >> >> Today's linux-next merge of the akpm-current tree got a conflict in: >> >> mm/gup.c >> >> between commit: >> >> 732b80e677b8 ("mm/gup/writeback: add callbacks for inaccessible pages") >> >> from the kvms390 tree and commit: >> >> 9947ea2c1e60 ("mm/gup: track FOLL_PIN pages") >> >> from the akpm-current tree. >> >> I fixed it up (see below - maybe not optimally) and can carry the fix as >> necessary. This is now fixed as far as linux-next is concerned, but any >> non trivial conflicts should be mentioned to your upstream maintainer >> when your tree is submitted for merging. You may also want to consider >> cooperating with the maintainer of the conflicting tree to minimise any >> particularly complex conflicts. >> > > Yes. Changes to mm/gup.c really should normally go through linux-mm and > Andrew's tree, if at all possible. This would have been caught, and figured out > on linux-mm, had that been done--instead of leaving the linux-next maintainer > trying to guess at how to resolve the conflict. Yes. This patch should go via Andrew. Claudio is going to provide a fixed up version that takes care of the new semantics. This patch was posted several times on linux-mm (also before rc1) and I will drop it from my tree due to the conflict. > > +Cc David Hildenbrand, who I see looked at the kvms390 proposed patch a bit. > Maybe he has some opinions, especially about my questions below. > > The fix-up below may (or may not) need some changes: > > > diff --cc mm/gup.c > index 354bcfbd844b,f589299b0d4a..000000000000 > --- a/mm/gup.c > +++ b/mm/gup.c > @@@ -269,18 -470,11 +468,19 @@@ retry > goto retry; > } > > + /* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */ > + if (unlikely(!try_grab_page(page, flags))) { > + page = ERR_PTR(-ENOMEM); > + goto out; > + } > + if (flags & FOLL_GET) { > > > If I'm reading the diff correctly, I believe that line should *maybe* be changed to: > > if (flags & (FOLL_GET | FOLL_PIN)) { > > ...because each of those flags has a similar effect: pinned pages for DMA or RDMA > use. So either flag will require a call to arch_make_page_accessible()...except that > I'm not sure that's what you want. Would the absence of a call to > arch_make_page_accessible() cause things like pin_user_pages() to not work correctly? > Seems like it would, to me. > > (I'm pretty unhappy that we have to ask this at the linux-next level.) > > Also below... > > > - if (unlikely(!try_get_page(page))) { > - page = ERR_PTR(-ENOMEM); > - goto out; > - } > + ret = arch_make_page_accessible(page); > + if (ret) { > + put_page(page); > > > put_page() only works with FOLL_GET. So if we do allow to get here via either FOLL_GET or > FOLL_PIN, the we need to do an unpin_user_page(), like this: > > if (flags & FOLL_PIN) > unpin_user_page(page); > else > put_page(page); > > > > + page = ERR_PTR(ret); > + goto out; > + } > + } > if (flags & FOLL_TOUCH) { > if ((flags & FOLL_WRITE) && > !pte_dirty(pte) && !PageDirty(page)) > > thanks, >