From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756476AbbKRREs (ORCPT ); Wed, 18 Nov 2015 12:04:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34771 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756385AbbKRREr (ORCPT ); Wed, 18 Nov 2015 12:04:47 -0500 Date: Wed, 18 Nov 2015 18:04:45 +0100 From: Andrea Arcangeli To: Jan Kara Cc: Dave Hansen , linux-kernel@vger.kernel.org, x86@kernel.org, dave.hansen@linux.intel.com, akpm@linux-foundation.org Subject: Re: [PATCH 02/37] mm, frame_vector: do not use get_user_pages_locked() Message-ID: <20151118170445.GO5078@redhat.com> References: <20151117033511.BFFA1440@viggo.jf.intel.com> <20151117033514.B104D3A2@viggo.jf.intel.com> <20151118122938.GB6097@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151118122938.GB6097@quack.suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 18, 2015 at 01:29:38PM +0100, Jan Kara wrote: > On Mon 16-11-15 19:35:14, Dave Hansen wrote: > > > > From: Dave Hansen > > > > get_user_pages_locked() appears to be for use when a caller needs > > to know that its lock on mmap_sem was invalidated by the gup > > call. > > > > But, get_vaddr_frames() is not one of those users. It > > unconditionally locks the mmap_sem and unconditionally unlocks it > > after the gup call. It takes no special action and does not need > > to know whether its lock was invalidated or not. > > > > Replace get_user_pages_locked() with a vanilla get_user_pages() > > and save a few lines of code. > > > > Note that this was the *ONLY* use of get_user_pages_locked() in > > the entire kernel tree. > > I've used get_user_pages_locked() because of a comment before that function > saying: > > * We can leverage the VM_FAULT_RETRY functionality in the page fault > * paths better by using either get_user_pages_locked() or > * get_user_pages_unlocked(). > * > * get_user_pages_locked() is suitable to replace the form: > * > * down_read(&mm->mmap_sem); > * do_something() > * get_user_pages(tsk, mm, ..., pages, NULL); > * up_read(&mm->mmap_sem); > * > * to: > * > * int locked = 1; > * down_read(&mm->mmap_sem); > * do_something() > * get_user_pages_locked(tsk, mm, ..., pages, &locked); > * if (locked) > * up_read(&mm->mmap_sem); > > So I understood it as a way to reduce mmap_sem hold time by doing a try > first. Did I understand that comment wrong? That is correct. get_user_pages_locked should not be downgraded to get_user_pages or it can actually break userfaultfd, as userfaultfd needs to be allowed to drop the mmap_sem within handle_mm_fault, to function correctly. Furthermore get_user_pages_locked allows for higher SMP scalability as it can take advantage of the optimized pagecache code that drops the mmap_sem before waiting for I/O (and then it retries the page fault after the I/O is complete). The only case where get_user_pages without FOLL_FAULT_ALLOW_RETRY makes any sense is the get_dump_page case which is actually not using get_user_pages in the first place, so ideally get_user_pages should be obsoleted and dropped. Thanks, Andrea