On 3/6/20 5:25 AM, Claudio Imbrenda wrote: > + /* > + * We need to make the page accessible if and only if we are going > + * to access its content (the FOLL_PIN case). Please see > + * Documentation/core-api/pin_user_pages.rst for details. > + */ > + if (flags & FOLL_PIN) { > + ret = arch_make_page_accessible(page); > + if (ret) { > + unpin_user_page(page); > + page = ERR_PTR(ret); > + goto out; > + } > + } Thanks, Claudio, for a really thorough refresher on this in private mail. But, I think this mechanism probably hooks into the wrong place. I don't doubt that it *functions* on s390, but I think these calls are misplaced. I think the end result is that no other architecture will have a chance to use the same hooks. They're far too s390-specific even for a concept that's not limited to s390. get_user_pages(FOLL_PIN) does *not* mean "the kernel will access this page's contents". The kmap() family is really what we use for that. kmap()s are often *preceded* by get_user_pages(), which is probably why this works for you, though. Yes, the docs do say that FOLL_PIN is for accessing the pages. But, there's a crucial thing that it leaves out: *WHO* will be accessing the pages. For Direct IO, for instance, the CPU isn't touching the page at all. It's always a device. Also, crucially, the page contents are *not* accessible from the CPU's perspective after a gup. They're not accessible until a kmap(). They're also not even accessible for *devices* after a gup. There's a _separate_ mapping process that's requires to make them accessible to the CPU. > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2764,7 +2764,7 @@ int test_clear_page_writeback(struct page *page) > int __test_set_page_writeback(struct page *page, bool keep_write) > { > struct address_space *mapping = page_mapping(page); > - int ret; > + int ret, access_ret; > > lock_page_memcg(page); > if (mapping && mapping_use_writeback_tags(mapping)) { > @@ -2807,6 +2807,13 @@ int __test_set_page_writeback(struct page *page, bool keep_write) > inc_zone_page_state(page, NR_ZONE_WRITE_PENDING); > } > unlock_page_memcg(page); > + access_ret = arch_make_page_accessible(page); > + /* > + * If writeback has been triggered on a page that cannot be made > + * accessible, it is too late to recover here. > + */ > + VM_BUG_ON_PAGE(access_ret != 0, page); > + > return ret; > > } I think this one really shows the cracks in the approach. Pages being swapped *don't* have get_user_pages() done on them since we've already got the physical page at the time writeback and aren't looking at PTEs. They're read by I/O devices sending them out to storage, but also by the CPU if you're doing something like zswap. But, again, critically, accessing page contents won't be done until kmap(). I suspect you saw crashes underneath __swap_writepage()->submit_bio() and looked a few lines up to the set_page_writeback() and decided to hook in there. I think a better spot, again, is to hook into kmap() which is called in the block layer. Why do I care? I was looking at AMD's SEV (Secure Encrypted Virtualization) code which is in the kernel which shares some implementation details with the not-in-the-tree Intel MKTME. SEV currently has a concept of guest pages being encrypted and being gibberish to the host, plus a handshake to share guest-selected pages. Some of the side-effects of exposing the gibberish to the host aren't great (I think it can break cache coherency if a stray write occurs) and it would be nice to get better behavior. But, to get better behavior, the host kernel might need to remove pages from its direct map, making them inaccessible. I was hoping to reuse arch_make_page_accessible() for obvious reasons. But, get_user_pages() is not the right spot to map pages because they might not *ever* be accessed by the CPU, only devices. Anyway, I know it's late feedback, but I'd hate to have core code like this that has no hope of ever getting reused.