Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753977AbaJGNZs (ORCPT ); Tue, 7 Oct 2014 09:25:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26644 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752054AbaJGNZq (ORCPT ); Tue, 7 Oct 2014 09:25:46 -0400 Date: Tue, 7 Oct 2014 15:24:58 +0200 From: Andrea Arcangeli To: "Kirill A. Shutemov" Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-api@vger.kernel.org, Linus Torvalds , Andres Lagar-Cavilla , Dave Hansen , Paolo Bonzini , Rik van Riel , Mel Gorman , Andy Lutomirski , Andrew Morton , Sasha Levin , Hugh Dickins , Peter Feiner , "\\\"Dr. David Alan Gilbert\\\"" , Christopher Covington , Johannes Weiner , Android Kernel Team , Robert Love , Dmitry Adamushko , Neil Brown , Mike Hommey , Taras Glek , Jan Kara , KOSAKI Motohiro , Michel Lespinasse , Minchan Kim , Keith Packard , "Huangpeng (Peter)" , Isaku Yamahata , Anthony Liguori , Stefan Hajnoczi , Wenchao Xia , Andrew Jones , Juan Quintela Subject: Re: [PATCH 08/17] mm: madvise MADV_USERFAULT Message-ID: <20141007132458.GZ2342@redhat.com> References: <1412356087-16115-1-git-send-email-aarcange@redhat.com> <1412356087-16115-9-git-send-email-aarcange@redhat.com> <20141007103645.GB30762@node.dhcp.inet.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141007103645.GB30762@node.dhcp.inet.fi> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kirill, On Tue, Oct 07, 2014 at 01:36:45PM +0300, Kirill A. Shutemov wrote: > On Fri, Oct 03, 2014 at 07:07:58PM +0200, Andrea Arcangeli wrote: > > MADV_USERFAULT is a new madvise flag that will set VM_USERFAULT in the > > vma flags. Whenever VM_USERFAULT is set in an anonymous vma, if > > userland touches a still unmapped virtual address, a sigbus signal is > > sent instead of allocating a new page. The sigbus signal handler will > > then resolve the page fault in userland by calling the > > remap_anon_pages syscall. > > Hm. I wounder if this functionality really fits madvise(2) interface: as > far as I understand it, it provides a way to give a *hint* to kernel which > may or may not trigger an action from kernel side. I don't think an > application will behaive reasonably if kernel ignore the *advise* and will > not send SIGBUS, but allocate memory. > > I would suggest to consider to use some other interface for the > functionality: a new syscall or, perhaps, mprotect(). I didn't feel like adding PROT_USERFAULT to mprotect, which looks hardwired to just these flags: PROT_NONE The memory cannot be accessed at all. PROT_READ The memory can be read. PROT_WRITE The memory can be modified. PROT_EXEC The memory can be executed. Normally mprotect doesn't just alter the vmas but it also alters pte/hugepmds protection bits, that's something that is never needed with VM_USERFAULT so I didn't feel like VM_USERFAULT is a protection change to the VMA. mprotect is also hardwired to mangle only the VM_READ|WRITE|EXEC flags, while madvise is ideal to set arbitrary vma flags. >From an implementation standpoint the perfect place to set a flag in a vma is madvise. This is what MADV_DONTFORK (it sets VM_DONTCOPY) already does too in an identical way to MADV_USERFAULT/VM_USERFAULT. MADV_DONTFORK is as critical as MADV_USERFAULT because people depends on it for example to prevent the O_DIRECT vs fork race condition that results in silent data corruption during I/O with threads that may fork. The other reason why MADV_DONTFORK is critical is that fork() would otherwise fail with OOM unless full overcommit is enabled (i.e. pci hotplug crashes the guest if you forget to set MADV_DONTFORK). Another madvise that would generate a failure if not obeyed by the kernel is MADV_DONTNEED that if it does nothing it could run lead to OOM killing. We don't inflate virt balloons using munmap just to make an example. Various other apps (maybe JVM garbage collection too) makes extensive use of MADV_DONTNEED and depend on it. Said that I can change it to mprotect, the only thing that I don't like is that it'll result in a less clean patch and I can't possibly see a practical risk in keeping it simpler with madvise, as long as we always return -EINVAL whenever we encounter a vma type that cannot raise userfaults yet (that is something I already enforced). Yet another option would be to drop MADV_USERFAULT and vm_flags&VM_USERFAULT entirely and in turn the ability to handle userfaults with SIGBUS, and retain only the userfaultfd. The new userfaultfd protocol requires registering each created userfaultfd into its own private virtual memory ranges (that is to allow an unlimited number of userfaultfd per process). Currently the userfaultfd engages iff the fault address intersects both the MADV_USERFAULT range and the userfaultfd registered ranges. So I could drop MADV_USERFAULT and VM_USERFAULT and just check for vma->vm_userfaultfd_ctx!=NULL to know if the userfaultfd protocol needs to be engaged during the first page fault for a still unmapped virtual address. I just thought it would be more flexibile to also allow SIGBUS without forcing people to use userfaultfd (that's in fact the only reason to still retain madvise(MADV_USERFAULT)!). Volatile pages earlier patches only supported SIGBUS behavior for example.. and I didn't intend to force them to use userfaultfd if they're guaranteed to access the memory with the CPU and never through a kernel syscall (that is something the app can enforce by design). userfaultfd becomes necessary the moment you want to handle userfaults through syscalls/gup etc... qemu obviously requires userfaultfd and it never uses the userfaultfd-less SIGBUS behavior as it touches the memory in all possible ways (first and foremost with the KVM page fault that uses almost all variants of gup..). So here somebody should comment and choose between: 1) set VM_USERFAULT with mprotect(PROT_USERFAULT) instead of the current madvise(MADV_USERFAULT) 2) drop MADV_USERFAULT and VM_USERFAULT and force the usage of the userfaultfd protocol as the only way for userland to catch userfaults (each userfaultfd must already register itself into its own virtual memory ranges so it's a trivial change for userfaultfd users that deletes just 1 or 2 lines of userland code, but it would prevent to use the SIGBUS behavior with info->si_addr=faultaddr for other users) 3) keep things as they are now: use MADV_USERFAULT for SIGBUS userfaults, with optional intersection between the vm_flags&VM_USERFAULT ranges and the userfaultfd registered ranges with vma->vm_userfaultfd_ctx!=NULL to know if to engage the userfaultfd protocol instead of the plain SIGBUS I will update the code accordingly to feedback, so please comment. I implemented 3) because I thought it provided the most flexibility for userland to choose if to engage in the userfaultfd protocol or to stay simple with the SIGBUS if the app doesn't require to access the userfault virtual memory from the kernel code. It also provides the cleanest and simplest implementation to set the VM_USERFAULT flags with madvise. My second choice would be 2). We could always add MADV_USERFAULT later except then we'd be forced to set and clear VM_USERFAULT within the userfaultfd registration to remain backwards compatible. The main cons and the reason I didn't pick 2) is that it wouldn't be a drop in replacement for volatile pages that would then be force to use the userfaultfd protocol too. I don't like 3) very much mostly because the changes to mprotect would just make things more complex on the implementation side with purely conceptual benefits, but then it's possible too and it's feature equivalent to 1) as far as volatile pages are concerned, so I'm overall fine with this change if that's the preferred way. Thanks, Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/