From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S376762AbdEAHNN (ORCPT ); Mon, 1 May 2017 03:13:13 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34954 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755193AbdEAHND (ORCPT ); Mon, 1 May 2017 03:13:03 -0400 Date: Mon, 1 May 2017 09:12:59 +0200 From: Ingo Molnar To: Dan Williams Cc: "linux-kernel@vger.kernel.org" , Linux MM , =?iso-8859-1?B?Suly9G1l?= Glisse , Ingo Molnar , Andrew Morton , Logan Gunthorpe , Kirill Shutemov Subject: Re: [PATCH v2] mm, zone_device: replace {get, put}_zone_device_page() with a single reference Message-ID: <20170501071259.5vya524wcdddm42b@gmail.com> References: <20170428063913.iz6xjcxblecofjlq@gmail.com> <149339998297.24933.1129582806028305912.stgit@dwillia2-desk3.amr.corp.intel.com> <20170429141838.tkyfxhldmwypyipz@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Dan Williams wrote: > On Sat, Apr 29, 2017 at 7:18 AM, Ingo Molnar wrote: > > > > * Dan Williams wrote: > > > >> Kirill points out that the calls to {get,put}_dev_pagemap() can be > >> removed from the mm fast path if we take a single get_dev_pagemap() > >> reference to signify that the page is alive and use the final put of the > >> page to drop that reference. > >> > >> This does require some care to make sure that any waits for the > >> percpu_ref to drop to zero occur *after* devm_memremap_page_release(), > >> since it now maintains its own elevated reference. > >> > >> Cc: Ingo Molnar > >> Cc: Jérôme Glisse > >> Cc: Andrew Morton > >> Reviewed-by: Logan Gunthorpe > >> Suggested-by: Kirill Shutemov > >> Tested-by: Kirill Shutemov > >> Signed-off-by: Dan Williams > > > > This changelog is lacking an explanation about how this solves the crashes you > > were seeing. > > Kirill? It wasn't clear to me why the conversion to generic > get_user_pages_fast() caused the reference counts to be off. Ok, the merge window is open and we really need this fix for x86/mm, so this is what I've decoded: The x86 conversion to the generic GUP code included a small change which causes crashes and data corruption in the pmem code - not good. The root cause is that the /dev/pmem driver code implicitly relies on the x86 get_user_pages() implementation doing a get_page() on the page refcount, because get_page() does a get_zone_device_page() which properly refcounts pmem's separate page struct arrays that are not present in the regular page struct structures. (The pmem driver does this because it can cover huge memory areas.) But the x86 conversion to the generic GUP code changed the get_page() to page_cache_get_speculative() which is faster but doesn't do the get_zone_device_page() call the pmem code relies on. One way to solve the regression would be to change the generic GUP code to use get_page(), but that would slow things down a bit and punish other generic-GUP using architectures for an x86-ism they did not care about. (Arguably the pmem driver was probably not working reliably for them: but nvdimm is an Intel feature, so non-x86 exposure is probably still limited.) So restructure the pmem code's interface with the MM instead: get rid of the get/put_zone_device_page() distinction, integrate put_zone_device_page() into __put_page() and and restructure the pmem completion-wait and teardown machinery. This speeds up things while also making the pmem refcounting more robust going forward. ... is this extension to the changelog correct? I'll apply this for the time being - but can still amend the text before sending it to Linus later today. Thanks, Ingo