From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 58F4DC004D4 for ; Thu, 19 Jan 2023 11:16:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C981B6B0072; Thu, 19 Jan 2023 06:16:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C48126B0073; Thu, 19 Jan 2023 06:16:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B0F746B0074; Thu, 19 Jan 2023 06:16:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 9EC2C6B0072 for ; Thu, 19 Jan 2023 06:16:39 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 43A13402E9 for ; Thu, 19 Jan 2023 11:16:39 +0000 (UTC) X-FDA: 80371295718.24.61A599A Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf26.hostedemail.com (Postfix) with ESMTP id 936D014000A for ; Thu, 19 Jan 2023 11:16:36 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="ZexZ/bRy"; spf=pass (imf26.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1674126996; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=q0jqQwZd9NFQ5cDKROGrUao6SgJdskLLJ/g0MsZ7haI=; b=zjDgAEHYLSFsykco/ANJk1XQskkoPohtMPXK8y3P7Drkgkq0HkbZJ1fF7T8I6PaA+m//jd TgEOTSD47dgvz3heZLq+oQ2qSOhX6e+rVijGCuFdXjHg0fJOmVAQHh9RbjkqHtaMzn1g+G lprl6mD4bAygf45c1N/3Rl7/Fq6Z3eU= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b="ZexZ/bRy"; spf=pass (imf26.hostedemail.com: domain of rppt@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=rppt@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674126996; a=rsa-sha256; cv=none; b=R2wRJHTO+gx+sO1/RCxFUqp3VR/kC97V44iJF6wbevmwcojuC+xtgaNPwkmyNui7u2FGvZ hx2nMMDCWsG97irkmp8nA0IJ2gJYivEpbAvvdTYLOQTSHSDxzV9p40zyCaHVbyQ0+whtez 0hNA7l7HzWQUXtrbPLjpex7YUShlhwI= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6E50E6150B; Thu, 19 Jan 2023 11:16:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9A246C433EF; Thu, 19 Jan 2023 11:16:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674126994; bh=iXYH708Vv09uHpgq/xbExzDnqXKEmBTrECS2JswF9hM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZexZ/bRy4LRNxirEr8euPRyQUNxFU/1b7mXmm8d1jCYwBSMgM4hJcPt+luBowqUUt W5EXt+vPHpEdL0cATXW6gHjeK3IJUUU9DsMAXeN0TcUqHPDijIJkgVlmzDrHZLZ0DM ii+VXUareTfCA2Yr/A4dGxjA0WM2gEIuCrmpLXsZ4nTv0TpdtCGa+lHalmLMuFUfyT oU3srg/YGTmpf97+xwKrxiDGry24ORPlTKUagyvVjUyzeMdpVkl+hvHPS6DtFjqsoj WbFjnHrYxQ/QIXyyeGSufIlab5FeAXLOYNxWEC/xSWvbkF0cOoN8i7VXHmuKb/pN0g RZm3Dq3cXlmzw== Date: Thu, 19 Jan 2023 13:16:23 +0200 From: Mike Rapoport To: Jason Gunthorpe Cc: Alistair Popple , John Hubbard , linux-mm@kvack.org Subject: Re: [PATCH 1/8] mm/gup: have internal functions get the mmap_read_lock() Message-ID: References: <0-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com> <1-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 936D014000A X-Stat-Signature: zqisw3yrsuwxpgemi4pnxo4oqqsaukws X-HE-Tag: 1674126996-47164 X-HE-Meta: U2FsdGVkX19cUQvbSJl0kJnNfi8FVTSOuDRuh3DKB/yEvzxvDOJiXYV29UmmPSHIlY4mwh87K3kyBLXAVt8rIhtecJPSABo+dYLPLNQDvprtpR9ciWlZzn7WAUGx4K9KH9/3iieRvdriIufh6mtFqj5OFKdM8P4RHjMzq2BP/JlZOsTVe0r+S3aedmZ13QorcH8wOjz0D+8xuoIFTZZqzy4eC2whLc1ysI0MiquFQQm/DvmqT4rRiUqNgkV0XGvuSuZYCM5LcBO5zeJojgJpiXh8AjGkS091A8W8/2/gEC9Nme+VBsi6LYDdVC6LmsQ9dPXxA8DPNYYZfmIv5S5vkVjlh1UEApssnT5jf0hQhRrpAEXFrTYeF5YJiTghqYsxLyrm9Ha6rYR3gDDXQAUpqA07GyMLWNN9WxwSG6KT74oT1MSbrKszyypljfCePTwpXgHB9AqW0hnGmtQF/JgP2/BSlkQdlud0Mth83bKOPloWIChcZKynnVXY29/gft2uJIuAZwN3NT79roDTODYyYYuzwh3/srxmSfGoc+pdJVF9h4S71vgbuYRa/Lieth3oOZx4u1uE8WMLP6MbGCWh1+domI2KrDM5HVjR/ZrmB7s+sjhdAFFDEt6tEPdQRTbjOGv3sEPvzd09mC/NbgttpzbpxUP0GD+LOavC3wT0YNO2g33PElsgw/+tJ5nqt2bhEr/JncMUjMjgkWalPmPCS3F8PYN/97hMQy2JAFpcFJ7XqgWm9mrafeUtvzCBVi8bpXQomGxIRnGhXHu/n3Aww82R3W0ufUMw//a0fhTvA3l2g6KAIHPgDdhWa15pedIhtWQzwBjYK/RiNlgHlpYU9lNp1kI9N0QQ3EJASGh3dxs1VWG0I83CmE7R9FW0X2K2KNr0gcoNnx6z9MRWSntYuZ9cc55uwQt/NykOvv1EuDZxx9fF4Dekbs3S1ZwHePWotSXm3LkOcuJ2VwAO3q+ VOZKvEnP RuEb9q8hXaVSN37FFIcqT4hNml4+V3cxcv8xpELSUVs1qzQ5Ldc5FIe+lma8ErmvsfJTyAifrnfDTOII= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jan 17, 2023 at 11:58:32AM -0400, Jason Gunthorpe wrote: > __get_user_pages_locked() and __gup_longterm_locked() both require the > mmap lock to be held. They have a slightly unusual locked parameter that > is used to allow these functions to unlock and relock the mmap lock and > convey that fact to the caller. > > Several places wrapper these functions with a simple mmap_read_lock() just Nit: ^wrap > so they can follow the optimized locked protocol. > > Consolidate this internally to the functions. Allow internal callers to > set locked = 0 to cause the functions to obtain and release the lock on I'd s/obtain/acquire/g > their own. > > Reorganize __gup_longterm_locked() to use the autolocking in > __get_user_pages_locked(). > > Replace all the places obtaining the mmap_read_lock() just to call > __get_user_pages_locked() with the new mechanism. Replace all the internal > callers of get_user_pages_unlocked() with direct calls to > __gup_longterm_locked() using the new mechanism. > > A following patch will add assertions ensuring the external interface > continues to always pass in locked = 1. > > Signed-off-by: Jason Gunthorpe > --- > mm/gup.c | 92 +++++++++++++++++++++++++++++--------------------------- > 1 file changed, 47 insertions(+), 45 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index f45a3a5be53a48..3a9f764165f50b 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1343,13 +1343,22 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > unsigned int flags) > { > long ret, pages_done; > - bool lock_dropped; > + bool lock_dropped = false; > > if (locked) { > /* if VM_FAULT_RETRY can be returned, vmas become invalid */ > BUG_ON(vmas); > - /* check caller initialized locked */ > - BUG_ON(*locked != 1); > + } > + > + /* > + * The internal caller expects GUP to manage the lock internally and the > + * lock must be released when this returns. > + */ > + if (locked && !*locked) { > + if (mmap_read_lock_killable(mm)) > + return -EAGAIN; > + lock_dropped = true; > + *locked = 1; > } > > if (flags & FOLL_PIN) > @@ -1368,7 +1377,6 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > flags |= FOLL_GET; > > pages_done = 0; > - lock_dropped = false; > for (;;) { > ret = __get_user_pages(mm, start, nr_pages, flags, pages, > vmas, locked); > @@ -1659,9 +1667,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, > unsigned int foll_flags) > { > struct vm_area_struct *vma; > + bool must_unlock = false; > unsigned long vm_flags; > long i; > > + if (!nr_pages) > + return 0; > + > + /* > + * The internal caller expects GUP to manage the lock internally and the > + * lock must be released when this returns. > + */ > + if (locked && !*locked) { > + if (mmap_read_lock_killable(mm)) > + return -EAGAIN; > + must_unlock = true; > + *locked = 1; > + } > + > /* calculate required read or write permissions. > * If FOLL_FORCE is set, we only require the "MAY" flags. > */ > @@ -1673,12 +1696,12 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, > for (i = 0; i < nr_pages; i++) { > vma = find_vma(mm, start); > if (!vma) > - goto finish_or_fault; > + break; > > /* protect what we can, including chardevs */ > if ((vma->vm_flags & (VM_IO | VM_PFNMAP)) || > !(vm_flags & vma->vm_flags)) > - goto finish_or_fault; > + break; > > if (pages) { > pages[i] = virt_to_page((void *)start); > @@ -1690,9 +1713,11 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, > start = (start + PAGE_SIZE) & PAGE_MASK; > } > > - return i; > + if (must_unlock && *locked) { > + mmap_read_unlock(mm); > + *locked = 0; > + } > > -finish_or_fault: > return i ? : -EFAULT; > } > #endif /* !CONFIG_MMU */ > @@ -1861,17 +1886,13 @@ EXPORT_SYMBOL(fault_in_readable); > #ifdef CONFIG_ELF_CORE > struct page *get_dump_page(unsigned long addr) > { > - struct mm_struct *mm = current->mm; > struct page *page; > - int locked = 1; > + int locked = 0; > int ret; > > - if (mmap_read_lock_killable(mm)) > - return NULL; > - ret = __get_user_pages_locked(mm, addr, 1, &page, NULL, &locked, > + ret = __get_user_pages_locked(current->mm, addr, 1, &page, NULL, > + &locked, > FOLL_FORCE | FOLL_DUMP | FOLL_GET); > - if (locked) > - mmap_read_unlock(mm); > return (ret == 1) ? page : NULL; > } > #endif /* CONFIG_ELF_CORE */ > @@ -2047,13 +2068,9 @@ static long __gup_longterm_locked(struct mm_struct *mm, > int *locked, > unsigned int gup_flags) > { > - bool must_unlock = false; > unsigned int flags; > long rc, nr_pinned_pages; > > - if (locked && WARN_ON_ONCE(!*locked)) > - return -EINVAL; > - > if (!(gup_flags & FOLL_LONGTERM)) > return __get_user_pages_locked(mm, start, nr_pages, pages, vmas, > locked, gup_flags); > @@ -2070,11 +2087,6 @@ static long __gup_longterm_locked(struct mm_struct *mm, > return -EINVAL; > flags = memalloc_pin_save(); > do { > - if (locked && !*locked) { > - mmap_read_lock(mm); > - must_unlock = true; > - *locked = 1; > - } > nr_pinned_pages = __get_user_pages_locked(mm, start, nr_pages, > pages, vmas, locked, > gup_flags); > @@ -2085,11 +2097,6 @@ static long __gup_longterm_locked(struct mm_struct *mm, > rc = check_and_migrate_movable_pages(nr_pinned_pages, pages); > } while (rc == -EAGAIN); > memalloc_pin_restore(flags); > - > - if (locked && *locked && must_unlock) { > - mmap_read_unlock(mm); > - *locked = 0; > - } > return rc ? rc : nr_pinned_pages; > } > > @@ -2242,16 +2249,10 @@ EXPORT_SYMBOL(get_user_pages); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags) > { > - struct mm_struct *mm = current->mm; > - int locked = 1; > - long ret; > + int locked = 0; > > - mmap_read_lock(mm); > - ret = __gup_longterm_locked(mm, start, nr_pages, pages, NULL, &locked, > - gup_flags | FOLL_TOUCH); > - if (locked) > - mmap_read_unlock(mm); > - return ret; > + return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL, > + &locked, gup_flags | FOLL_TOUCH); > } > EXPORT_SYMBOL(get_user_pages_unlocked); > > @@ -2904,6 +2905,7 @@ static int internal_get_user_pages_fast(unsigned long start, > { > unsigned long len, end; > unsigned long nr_pinned; > + int locked = 0; > int ret; > > if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM | > @@ -2932,8 +2934,9 @@ static int internal_get_user_pages_fast(unsigned long start, > /* Slow path: try to get the remaining pages with get_user_pages */ > start += nr_pinned << PAGE_SHIFT; > pages += nr_pinned; > - ret = get_user_pages_unlocked(start, nr_pages - nr_pinned, pages, > - gup_flags); > + ret = __gup_longterm_locked(current->mm, start, nr_pages - nr_pinned, > + pages, NULL, &locked, > + gup_flags | FOLL_TOUCH); > if (ret < 0) { > /* > * The caller has to unpin the pages we already pinned so > @@ -3180,14 +3183,13 @@ EXPORT_SYMBOL(pin_user_pages); > long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > struct page **pages, unsigned int gup_flags) > { > - /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > - if (WARN_ON_ONCE(gup_flags & FOLL_GET)) > - return -EINVAL; > + int locked = 0; > > if (WARN_ON_ONCE(!pages)) > return -EINVAL; > > - gup_flags |= FOLL_PIN; > - return get_user_pages_unlocked(start, nr_pages, pages, gup_flags); > + gup_flags |= FOLL_PIN | FOLL_TOUCH; > + return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL, > + &locked, gup_flags); > } > EXPORT_SYMBOL(pin_user_pages_unlocked); > -- > 2.39.0 > > -- Sincerely yours, Mike.