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 1DFC8C433EF for ; Thu, 3 Feb 2022 13:31:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6F27D8D014A; Thu, 3 Feb 2022 08:31:29 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6A1138D0124; Thu, 3 Feb 2022 08:31:29 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5418F8D014A; Thu, 3 Feb 2022 08:31:29 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0161.hostedemail.com [216.40.44.161]) by kanga.kvack.org (Postfix) with ESMTP id 40EF08D0124 for ; Thu, 3 Feb 2022 08:31:29 -0500 (EST) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id E96659095A for ; Thu, 3 Feb 2022 13:31:28 +0000 (UTC) X-FDA: 79101555456.07.D619BAC Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by imf12.hostedemail.com (Postfix) with ESMTP id 72CF540009 for ; Thu, 3 Feb 2022 13:31:28 +0000 (UTC) Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 213BbJf4003414; Thu, 3 Feb 2022 13:31:27 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=date : from : to : cc : subject : message-id : in-reply-to : references : mime-version : content-type : content-transfer-encoding; s=pp1; bh=HitBZKpE6KMyaGTf1UTC72zyQwZjkU4C57dlGY5K7Hg=; b=l+KNiCXhgU5K3W2XlsfZtj5Zps7pfV72BrA/MUmNUTT7yKtZrxXTTHjIJ4tVmT/SE3Pf 9YKnieQZb1s6zugMEa8yFEFcEPYmdFIuHDjvOFf3FQhg+G3YyWYVxeE39LgzOXmC5qH/ xPQ2/vG25s59lYJXO7hFBNCxAACubt60kBSNj8aph23eBIYsS3LRzEQMhXvGR2U7SViG fRs+xbGMTUEZ9LKtrGM2yZlzqZLuqK4kCz6K41PAd4hAlx5gsX6lLpTJMJ27SCAwgIIs v9rXsVPk5+T/vTU9BQ8yk+21srHXKbX1qAtSAEbQhxkQo8qduqdBw3qJXIk/+oiFGcfD 3Q== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3e04m6598b-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 03 Feb 2022 13:31:27 +0000 Received: from m0098413.ppops.net (m0098413.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 213Crl3I006696; Thu, 3 Feb 2022 13:31:27 GMT Received: from ppma03fra.de.ibm.com (6b.4a.5195.ip4.static.sl-reverse.com [149.81.74.107]) by mx0b-001b2d01.pphosted.com with ESMTP id 3e04m6597h-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 03 Feb 2022 13:31:26 +0000 Received: from pps.filterd (ppma03fra.de.ibm.com [127.0.0.1]) by ppma03fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 213DCfw9029778; Thu, 3 Feb 2022 13:31:25 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma03fra.de.ibm.com with ESMTP id 3dvw7a4gc1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 03 Feb 2022 13:31:25 +0000 Received: from b06wcsmtp001.portsmouth.uk.ibm.com (b06wcsmtp001.portsmouth.uk.ibm.com [9.149.105.160]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 213DLTrR48234958 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 3 Feb 2022 13:21:29 GMT Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B9B66A405F; Thu, 3 Feb 2022 13:31:22 +0000 (GMT) Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 07356A4054; Thu, 3 Feb 2022 13:31:22 +0000 (GMT) Received: from p-imbrenda (unknown [9.145.1.135]) by b06wcsmtp001.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 3 Feb 2022 13:31:21 +0000 (GMT) Date: Thu, 3 Feb 2022 14:31:14 +0100 From: Claudio Imbrenda To: John Hubbard Cc: Andrew Morton , Peter Xu , Jason Gunthorpe , David Hildenbrand , Lukas Bulwahn , Jan Kara , "Kirill A . Shutemov" , Alex Williamson , Andrea Arcangeli , LKML , , Jason Gunthorpe Subject: Re: [PATCH v3 2/4] mm/gup: clean up follow_pfn_pte() slightly Message-ID: <20220203143114.42d59bbe@p-imbrenda> In-Reply-To: <20220203093232.572380-3-jhubbard@nvidia.com> References: <20220203093232.572380-1-jhubbard@nvidia.com> <20220203093232.572380-3-jhubbard@nvidia.com> Organization: IBM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: VYOFWgMDHQCNwsAsCcSS0XWBQztY54HN X-Proofpoint-GUID: zWyyqxjiLkSW97wRorv6O7TwdRSjyZLc X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2022-02-03_03,2022-02-03_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 mlxscore=0 malwarescore=0 phishscore=0 mlxlogscore=999 priorityscore=1501 spamscore=0 adultscore=0 suspectscore=0 clxscore=1011 bulkscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000 definitions=main-2202030082 X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 72CF540009 X-Stat-Signature: 84bytbpqkkhf57hrriz3d8t14pqes46b X-Rspam-User: nil Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=ibm.com header.s=pp1 header.b=l+KNiCXh; spf=pass (imf12.hostedemail.com: domain of imbrenda@linux.ibm.com designates 148.163.158.5 as permitted sender) smtp.mailfrom=imbrenda@linux.ibm.com; dmarc=pass (policy=none) header.from=ibm.com X-HE-Tag: 1643895088-524383 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 Thu, 3 Feb 2022 01:32:30 -0800 John Hubbard wrote: > Regardless of any FOLL_* flags, get_user_pages() and its variants should > handle PFN-only entries by stopping early, if the caller expected > **pages to be filled in. > > This makes for a more reliable API, as compared to the previous approach > of skipping over such entries (and thus leaving them silently > unwritten). > > Cc: Peter Xu > Cc: Lukas Bulwahn > Suggested-by: Jason Gunthorpe > Reviewed-by: Jason Gunthorpe > Signed-off-by: John Hubbard > --- > mm/gup.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 65575ae3602f..cad3f28492e3 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -439,10 +439,6 @@ static struct page *no_page_table(struct vm_area_struct *vma, > static int follow_pfn_pte(struct vm_area_struct *vma, unsigned long address, > pte_t *pte, unsigned int flags) > { > - /* No page to get reference */ > - if (flags & (FOLL_GET | FOLL_PIN)) > - return -EFAULT; > - > if (flags & FOLL_TOUCH) { > pte_t entry = *pte; > > @@ -1180,8 +1176,13 @@ static long __get_user_pages(struct mm_struct *mm, > } else if (PTR_ERR(page) == -EEXIST) { > /* > * Proper page table entry exists, but no corresponding > - * struct page. > + * struct page. If the caller expects **pages to be > + * filled in, bail out now, because that can't be done > + * for this page. > */ > + if (pages) > + goto out; > + > goto next_page; > } else if (IS_ERR(page)) { > ret = PTR_ERR(page); I'm not an expert, can you explain why this is better, and why it does not cause new issues? If I understand correctly, the problem you are trying to solve is that in some cases you might try to get n pages, but you only get m < n pages instead, because some don't have an associated struct page, and the missing pages might even be in the middle. The `pages` array would contain the list of pages actually pinned (getted?), but this won't tell which of the requested pages have been pinned (e.g. if some pages in the middle of the run were skipped) With your patch you will stop at the first page without a struct page, meaning that if the caller tries again, it will get 0 pages. Why won't this cause issues? Why will this not cause problems when the `pages` parameter is NULL? sorry for the dumb questions, but this seems a rather important change, and I think in these circumstances you can't have too much documentation.