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: 37840 Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c1Gp3-0002YO-Gt; Mon, 31 Oct 2016 17:56:37 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-8) with esmtp id 1c1Gp0-0004u4-mK; Mon, 31 Oct 2016 18:56:37 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S945290AbcJaRzu (ORCPT + 1 other); Mon, 31 Oct 2016 13:55:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44622 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S945230AbcJaRzp (ORCPT ); Mon, 31 Oct 2016 13:55:45 -0400 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> Cc: linux-mm@kvack.org, Linus Torvalds , Michal Hocko , Jan Kara , Hugh Dickins , Dave Hansen , Rik van Riel , Mel Gorman , Andrew Morton , linux-kernel@vger.kernel.org, linux-cris-kernel@axis.com, linux-ia64@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rdma@vger.kernel.org, kvm@vger.kernel.org, linux-media@vger.kernel.org, devel@driverdev.osuosl.org 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) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.10.31.175117 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, MSGID_ADDED_BY_MTA 0.05, BODY_SIZE_6000_6999 0, BODY_SIZE_7000_LESS 0, IN_REP_TO 0, LEGITIMATE_NEGATE 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, REFERENCES 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CC_NAME 0, __CC_NAME_DIFF_FROM_ACC 0, __CC_REAL_NAMES 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __FORWARDED_MSG 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_VERSION 0, __MOZILLA_USER_AGENT 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' 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 :) > --- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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; }