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.

+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,
-- 
John Hubbard
NVIDIA