From patchwork Mon Oct 31 17:55:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 9406179 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 05F9960721 for ; Mon, 31 Oct 2016 17:55:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EE52C29380 for ; Mon, 31 Oct 2016 17:55:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E0EC829385; Mon, 31 Oct 2016 17:55:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 3526429380 for ; Mon, 31 Oct 2016 17:55:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7210C6E1CA; Mon, 31 Oct 2016 17:55:47 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5D4576E1CA for ; Mon, 31 Oct 2016 17:55:45 +0000 (UTC) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5442B4E4DE; Mon, 31 Oct 2016 17:55:44 +0000 (UTC) Received: from [10.36.112.17] (ovpn-112-17.ams2.redhat.com [10.36.112.17]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9VHtYgx010800 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 31 Oct 2016 13:55:36 -0400 Subject: Re: [PATCH 2/2] mm: remove get_user_pages_locked() To: Lorenzo Stoakes References: <20161031100228.17917-1-lstoakes@gmail.com> <20161031100228.17917-3-lstoakes@gmail.com> <20161031134849.GA13609@lucifer> From: Paolo Bonzini Message-ID: Date: Mon, 31 Oct 2016 18:55:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161031134849.GA13609@lucifer> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 31 Oct 2016 17:55:44 +0000 (UTC) Cc: devel@driverdev.osuosl.org, Rik van Riel , Jan Kara , kvm@vger.kernel.org, linux-cris-kernel@axis.com, linux-rdma@vger.kernel.org, Mel Gorman , Dave Hansen , Hugh Dickins , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Michal Hocko , linux-mm@kvack.org, linux-ia64@vger.kernel.org, Andrew Morton , Linus Torvalds , linux-media@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 31/10/2016 14:48, Lorenzo Stoakes wrote: > On Mon, Oct 31, 2016 at 12:45:36PM +0100, Paolo Bonzini wrote: >> >> >> On 31/10/2016 11:02, Lorenzo Stoakes wrote: >>> - * >>> - * get_user_pages should be phased out in favor of >>> - * get_user_pages_locked|unlocked or get_user_pages_fast. Nothing >>> - * should use get_user_pages because it cannot pass >>> - * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault. >> >> This comment should be preserved in some way. In addition, removing > > Could you clarify what you think should be retained? > > The comment seems to me to be largely rendered redundant by this change - > get_user_pages() now offers identical behaviour, and of course the latter part > of the comment ('because it cannot pass FAULT_FLAG_ALLOW_RETRY') is rendered > incorrect by this change. > > It could be replaced with a recommendation to make use of VM_FAULT_RETRY logic > if possible. Yes, exactly. locked == NULL should be avoided whenever mmap_sem can be dropped, and the comment indirectly said so. Now most of those cases actually are those where you can just call get_user_pages_unlocked. >> get_user_pages_locked() makes it harder (compared to a simple "git grep >> -w") to identify callers that lack allow-retry functionality). So I'm >> not sure about the benefits of these patches. > > That's a fair point, and certainly this cleanup series is less obviously helpful > than previous ones. > > However, there are a few points in its favour: > > 1. get_user_pages_remote() was the mirror of get_user_pages() prior to adding an > int *locked parameter to the former (necessary to allow for the unexport of > __get_user_pages_unlocked()), differing only in task/mm being specified and > FOLL_REMOTE being set. This patch series keeps these functions 'synchronised' > in this respect. > > 2. There is currently only one caller of get_user_pages_locked() in > mm/frame_vector.c which seems to suggest this function isn't widely > used/known. Or not widely necessary. :) > 3. This change results in all slow-path get_user_pages*() functions having the > ability to use VM_FAULT_RETRY logic rather than 'defaulting' to using > get_user_pages() that doesn't let you do this even if you wanted to. This is only true if someone does the work though. From a quick look at your series, the following files can be trivially changed to use get_user_pages_unlocked: - drivers/gpu/drm/via/via_dmablit.c - drivers/platform/goldfish/goldfish_pipe.c - drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c - drivers/rapidio/devices/rio_mport_cdev.c - drivers/virt/fsl_hypervisor.c Also, videobuf_dma_init_user could be changed to use retry by adding a *locked argument to videobuf_dma_init_user_locked, prototype patch after my signature. Everything else is probably best kept using get_user_pages. > 4. It's somewhat confusing/redundant having both get_user_pages_locked() and > get_user_pages() functions which both require mmap_sem to be held (i.e. both > are 'locked' versions.) > >> If all callers were changed, then sure removing the _locked suffix would >> be a good idea. > > It seems many callers cannot release mmap_sem so VM_FAULT_RETRY logic couldn't > happen anyway in this cases and in these cases we couldn't change the caller. But then get_user_pages_locked remains a less-common case, so perhaps it's a good thing to give it a longer name! > Overall, an alternative here might be to remove get_user_pages() and update > get_user_pages_locked() to add a 'vmas' parameter, however doing this renders > get_user_pages_unlocked() asymmetric as it would lack an vmas parameter (adding > one there would make no sense as VM_FAULT_RETRY logic invalidates VMAs) though > perhaps not such a big issue as it makes sense as to why this is the case. Adding the 'vmas' parameter to get_user_pages_locked would make little sense. Since VM_FAULT_RETRY invalidates it and g_u_p_locked can and does retry, it would generally not be useful. So I think we have the right API now: - do not have lock? Use get_user_pages_unlocked, get retry for free, no need to handle mmap_sem and the locked argument; cannot get back vmas. - have and cannot drop lock? User get_user_pages, no need to pass NULL for the locked argument; can get back vmas. - have but can drop lock (rare case)? Use get_user_pages_locked, cannot get back vams. Paolo > get_user_pages_unlocked() definitely seems to be a useful helper and therefore > makes sense to retain. > Of course another alternative is to leave things be :) > diff --git a/drivers/media/v4l2-core/videobuf-dma-sg.c b/drivers/media/v4l2-core/videobuf-dma-sg.c index 1db0af6c7f94..dae4eb8e9d5b 100644 --- a/drivers/media/v4l2-core/videobuf-dma-sg.c +++ b/drivers/media/v4l2-core/videobuf-dma-sg.c @@ -152,7 +152,8 @@ static void videobuf_dma_init(struct videobuf_dmabuf *dma) } static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma, - int direction, unsigned long data, unsigned long size) + int direction, unsigned long data, unsigned long size, + int *locked) { unsigned long first, last; int err, rw = 0; @@ -185,8 +186,17 @@ static int videobuf_dma_init_user_locked(struct videobuf_dmabuf *dma, dprintk(1, "init user [0x%lx+0x%lx => %d pages]\n", data, size, dma->nr_pages); - err = get_user_pages(data & PAGE_MASK, dma->nr_pages, - flags, dma->pages, NULL); + if (locked && !*locked) { + down_read(¤t->mm->mmap_sem); + *locked = 1; + } + + /* + * If the caller cannot have mmap_sem dropped, it passes locked == NULL + * so get_user_pages_locked will not release it. + */ + err = get_user_pages_locked(data & PAGE_MASK, dma->nr_pages, + flags, dma->pages, locked); if (err != dma->nr_pages) { dma->nr_pages = (err >= 0) ? err : 0; @@ -200,10 +210,11 @@ static int videobuf_dma_init_user(struct videobuf_dmabuf *dma, int direction, unsigned long data, unsigned long size) { int ret; + int locked = 0; - down_read(¤t->mm->mmap_sem); - ret = videobuf_dma_init_user_locked(dma, direction, data, size); - up_read(¤t->mm->mmap_sem); + ret = videobuf_dma_init_user_locked(dma, direction, data, size, &locked); + if (locked) + up_read(¤t->mm->mmap_sem); return ret; } @@ -540,7 +551,7 @@ static int __videobuf_iolock(struct videobuf_queue *q, err = videobuf_dma_init_user_locked(&mem->dma, DMA_FROM_DEVICE, - vb->baddr, vb->bsize); + vb->baddr, vb->bsize, NULL); if (0 != err) return err; }