Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Apr 2023 14:26:12 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     "zhangpeng (AS)" <zhangpeng362@huawei.com>
Cc:     Vishal Moola <vishal.moola@gmail.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
        willy@infradead.org, sidhartha.kumar@oracle.com,
        muchun.song@linux.dev, wangkefeng.wang@huawei.com,
        sunnanyong@huawei.com
Subject: Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user()
 to copy_folio_from_user()
Message-ID: <20230410212612.GA8315@monkey>
References: <20230331093937.945725-1-zhangpeng362@huawei.com>
 <20230331093937.945725-4-zhangpeng362@huawei.com>
 <CAOzc2pzr7VJRdsx1ud_ceBhbu2XP7Ay72jFETtN8eOt5yR7S=Q@mail.gmail.com>
 <a874c84b-4a83-c12f-e064-eab6a792c1e6@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a874c84b-4a83-c12f-e064-eab6a792c1e6@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NWxHYnFPL0RCaHEwU1RBZEdtc2RLNWhFWldlcUNOaUQydGtpQXFHcjdCd2ps?=
 =?utf-8?B?ajVBL0JIbVR5OEVRTlVPa0JuTUdiaFBTcGZIa3owZEJHQlQ1QWtteURqMmZB?=
 =?utf-8?B?eEVQTklOZFhCaDI1amFKM3ZRUHZMUGZlaW9ZK2RuK2VWQmFVZWV1OStiZEY5?=
 =?utf-8?B?MjNWTGNmNE9YK2tZU3RsMHNJeVZnWTRicU5vM1lZWnFVNWd2aUtzbTFmUHpO?=
 =?utf-8?B?N2FPWGNhTFpFc1Q2SGNnKzRtMDBrT2xWcVVkRWp2N2pEMXYzeUM1RmVMQ0ps?=
 =?utf-8?B?RHBXaE40SCtCRVJhTUtyT3NvWHlNSmZ4aWd1d0tWalhZbDZnazFJSnp6VEQ3?=
 =?utf-8?B?MGlKMnlZSndNT1RVcG4vOVJYZHl3QWlqNXd1SXJPK1VaYllkaWtMajFYY3ln?=
 =?utf-8?B?ZitudXN4ZFBRUEswNU5oZ2NRR0VvMTZGMG15Q0haT292Zi9nQVJHZkpqaU9R?=
 =?utf-8?B?Ly8vY2ZBR3pnZVdZVldDSUI2TDh2aVFGd0srbzZWTzlONFQzOFJ3bTZLRWdG?=
 =?utf-8?B?U0ZqOFNnZitFR2w3MEhud1V4UndoLzBuVEllazFrc0wxUWdidHZ3UGw0Z0Ja?=
 =?utf-8?B?dWJieG5mek5lRzV3WjNnSUJKTWU3MVFLaTd6TFRHQyt4WitDQXZFWHZPSnVp?=
 =?utf-8?B?U2VmTTZFaWVhUlozNXViVnI2cjdTK3BPNlZ1cDQ5N240UjJTdVRRYWhQUmJ4?=
 =?utf-8?B?NFFDNU1QVVk1V29UYUxLSDNtN2pLQUJiWEV6ekgxTVJjUmxYYnZSQ1c4RmNN?=
 =?utf-8?B?cnFsdmEyakxMUngwaHVFa3JvQ05EN1BHREx2d1NjY3R6ZUxUR3I1WjFFQy9B?=
 =?utf-8?B?dEVZdXdYU0N1bFNmZko1ZzhqTzJYWUMyTldtcmRMTHFLK1V1U2NBZHdhREdS?=
 =?utf-8?B?V3dRUm1DTkVYdHhVQVdhMnZUUFo0dXZDSDMzY3ZYMXpGZWozeit4QlRnVGtM?=
 =?utf-8?B?NzBtd0VSMU96Z2dKa0VGRE1rMGZRVDNLR3dIVkZONDM4eXZKL3l1WGRWRHRr?=
 =?utf-8?B?VXBGMkdORE5IaFg4bzFseHNhTkVLS0l0bVR4RWdYSkJIcHdDU3U1TjZiNmtl?=
 =?utf-8?B?dFhWZmFZaTlNL242VjN4dFUwQXByVXVMQ0tXdkhxd3F3V2dSVW1JdWY5SjZl?=
 =?utf-8?B?ZjZ4YmJKNG5nc1lBRFErcytPdUdjVkRSMHlYdzFjYzhtMEwxUUFvQlozdzlk?=
 =?utf-8?B?dmVybFJ4U2Y3QXZwSm8xZXE0N2htWmFKckN1ZGNpTFhUVjY0Q29YcmxsVnVn?=
 =?utf-8?B?bjlTTm5CNTdBZWFWMG9PajJ0QzBMNnNCR3BQdGd1TEJmTlZoM0Z1YTU2OUhz?=
 =?utf-8?B?QkRZSjNDUzdDaXBacnZhNFM3VzgvSWNIWWNIRkJUTXM0elYxbERIMjU0V0dy?=
 =?utf-8?B?S1lzOEZvTkgrMUQwS3JReEk1WHdGWk9jRWw3eGJZNjV5OHZNNEdXSUFWaitl?=
 =?utf-8?B?RmlFV0RaYktsdUFLL1Jyd2IwOUlEc29IV3lYQlBkUmxnb2FWaUpkN0E4WGtV?=
 =?utf-8?B?d0l6N01YTlJ2N2VGditkR2FnQ2VWZHIvMVJzRUFtejhld2VON2ZaU0FsTURr?=
 =?utf-8?B?TE8ybFQyNXZUOEs5TkRRV1FJaWNFMmNzcGt0MlhDd01taWJqNTQyR21vb3ht?=
 =?utf-8?B?S1UrMDJBa2N4bGRlMGVId1V6TEk5R3BFOEhQS3hyUWFCMEtpS0pZMXhzcDhT?=
 =?utf-8?B?YkY4WVhWdEJSMjNNcEhYcnZVeE10Q0JPczhTUTVrWk9VbFl2R2tCajdqank0?=
 =?utf-8?B?dzdJNDljS3ZDd2hSWEFROEJEUjBEQkt4aWNJcjV3ZzJacitkbXBsQzl6SzBE?=
 =?utf-8?B?OG5keUJEVUI5MExWU2dvekJDQk0zNEprRmVPSGl6TEVoUmlHYUJudi9MOEox?=
 =?utf-8?B?OWRDUitCdkhoZ0g0alRrVStnMXQwOW1EUHFKWmlhbE5nZ2JqSHUwR3Y4djBh?=
 =?utf-8?B?YzNYQkpJb2N0YjJ0dUhXMk5lY1k1QlVER2FaZmEyRnczRFdCenlBTHdOSk45?=
 =?utf-8?B?ZmRCV0R0WkRqZGJ6U0FsUUE3dG14R1B5M1F5ZW85aFdYZk1LTDkzdHRneEtE?=
 =?utf-8?B?S29mSGpyaTk3TlVQeXF0UzVKSHhYQXhDSENjWnk1SGJjRXBnUXY3MUUrSS9n?=
 =?utf-8?Q?xbl8OrBrAFSCAhy+fDZHg2uF9?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?RWtncXg4RkRBWDM3YzREcEJuRmJMcWtwL3I1dTFSTDNBRFFPN3ZlaXhyZjNh?=
 =?utf-8?B?NXE1RWZXTGp4MXI3ZmVEM1Q2TFE2Qi9taWNFRkZ6SlQ5WmhjdEdJbndmdjBU?=
 =?utf-8?B?d1ZpMjFJd1BsUW9UN0lWYVZOTHNVUS9yMmRKVGpPSmo1YUJvUXRHb000Q1hD?=
 =?utf-8?B?eUVCQUFVOG5BR0ZUT3g1RUxScXRYYU1HbnlHQlZRSlY4UnU2S2hJRERrbmwy?=
 =?utf-8?B?OGVRUTF1cHVLV0k5RXVYWkE3WFByNXJCalBHemVjTE14Zy9WRUhtYUpBZGNz?=
 =?utf-8?B?U3FobG9YbkUvMmIrbzh0WTlqM25hOVlNaUROZG9vSSt4aTRkWjJibTJHelAv?=
 =?utf-8?B?VzFmNEgzMXFPRmtPTTZ1Y2RXcS9EUU5OcC91Ky90bkdZMUd4U3dLNUxYc3Jp?=
 =?utf-8?B?TWh4cW5zMCszMkdWY2owKzRqQWc4bkplbDd6VzRuZmJ1a3ZEQjFsVnlMVHRE?=
 =?utf-8?B?eWpHQzJrQjlrVmQrY1RGOGtPbk1kTlJ4S2tBYjVta1owcVpxNGR3MnVQSlMr?=
 =?utf-8?B?NFRCVEdPVnpiWk52ZzREbDYwb2tFbk1FZi9WV0RHVjhaVDBtWFNvQ2s3ZXlO?=
 =?utf-8?B?TXF3OG5jRDEwbjdlNGc0dENYYmhKMmNDcEJvc3h3cEY5Q3B5aXdYWk9VTE5k?=
 =?utf-8?B?Sm5jMDdNcG03d0p5SUlUV05iRFhMcUs4KzVFTlhxRjZYMGFVSW5QWDRBKzUy?=
 =?utf-8?B?cWMxTXZuUUJEa3cxb21mNGVkTWpSR2RNU3ozRTVaSlFZYkZhZy85MGRRUThE?=
 =?utf-8?B?SXo4OFhKYVVOOUJBWHFLcWlVQlB5N3JYY0xMVEtDSlhKa1QraW1XZzJGc2dx?=
 =?utf-8?B?d1Z0dmVNOFptVDFYcUtGNmZTSUZ2aXhSbHNQTDRlOXdtSkV6SjFYQTU2eXBv?=
 =?utf-8?B?RW1FRWxrM2duZzhEblh0TUFUazB5NEhIK1diVm9aMkI2ZUx4VGdMSm9HQ0Qy?=
 =?utf-8?B?NFZGTS9iazI3KzhmVGVTMjUzZ2xKZDhubDlFOGhZaDB6TU5uRmdJc0pub3NJ?=
 =?utf-8?B?VEpCK2VnVCtOd1BhQnV0Sk5DRmJHS1Z6RU5CN3J2OTVzcXRUc0lIcFlKRXIv?=
 =?utf-8?B?RTM3L1FDUEFFOWtSOEpYbXZlN1MwaDAwVFFBRTNFeDNXK0tOODRqbDk1WHpH?=
 =?utf-8?B?OHVweW9zc3NoVnpmQ0JBMS83SGozSjQ3Q2xqeWcwSGZueGdSUGRLL2gzM0Z3?=
 =?utf-8?B?aW1raVNhbnlEQlp6UTkrZEJpZ25BbDY1ZzY4UlJoODREY1gyNldoS3BqUGVI?=
 =?utf-8?B?WWhUSWZIYTNoVDB0NTdRQksvWk8rKzZ3NmhEN1hOV3pFbkFZUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8fb8251f-a894-40c6-c79c-08db3a0a36f3
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2023 21:26:15.1079
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Xq6Tuo69ASPO5bUq9CmT14Y6NRM6Vyk4EOayftHS+6ifP8QD+70uNg+NAzxXLAX/8xNEF/J2ZoLsAXpSoXP1jw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4741
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-10_15,2023-04-06_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 suspectscore=0 mlxscore=0 bulkscore=0 malwarescore=0 phishscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2304100186
X-Proofpoint-GUID: ZN4k3_vcdSt58KhXk3n5CZZQSKPZV26W
X-Proofpoint-ORIG-GUID: ZN4k3_vcdSt58KhXk3n5CZZQSKPZV26W
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 04/08/23 12:43, zhangpeng (AS) wrote:
> On 2023/4/7 10:28, Vishal Moola wrote:
> 
> > On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@huawei.com> wrote:
> > > From: ZhangPeng <zhangpeng362@huawei.com>
> > > 
> > > Replace copy_huge_page_from_user() with copy_folio_from_user().
> > > copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> > > takes in a folio instead of a page. Convert page_kaddr to kaddr in
> > > copy_folio_from_user() to do indenting cleanup.
> > > 
> > > Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> > > Reviewed-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> > > ---
> > > -                               bool allow_pagefault)
> > > +long copy_folio_from_user(struct folio *dst_folio,
> > > +                          const void __user *usr_src,
> > > +                          bool allow_pagefault)
> > >   {
> > > -       void *page_kaddr;
> > > +       void *kaddr;
> > >          unsigned long i, rc = 0;
> > > -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
> > > +       unsigned int nr_pages = folio_nr_pages(dst_folio);
> > > +       unsigned long ret_val = nr_pages * PAGE_SIZE;
> > >          struct page *subpage;
> > > 
> > > -       for (i = 0; i < pages_per_huge_page; i++) {
> > > -               subpage = nth_page(dst_page, i);
> > > -               page_kaddr = kmap_local_page(subpage);
> > > +       for (i = 0; i < nr_pages; i++) {
> > > +               subpage = folio_page(dst_folio, i);
> > > +               kaddr = kmap_local_page(subpage);
> > >                  if (!allow_pagefault)
> > >                          pagefault_disable();
> > > -               rc = copy_from_user(page_kaddr,
> > > -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > > +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > >                  if (!allow_pagefault)
> > >                          pagefault_enable();
> > > -               kunmap_local(page_kaddr);
> > > +               kunmap_local(kaddr);
> > > 
> > >                  ret_val -= (PAGE_SIZE - rc);
> > >                  if (rc)
> > >                          break;
> > > 
> > > -               flush_dcache_page(subpage);
> > > -
> > >                  cond_resched();
> > >          }
> > > +       flush_dcache_folio(dst_folio);
> > >          return ret_val;
> > >   }
> > Moving the flush_dcache_page() outside the loop to be
> > flush_dcache_folio() changes the behavior of the function.
> > 
> > Initially, if it fails to copy the entire page, the function breaks out
> > of the loop and returns the number of unwritten bytes without
> > flushing the page from the cache. Now if it fails, it will still flush
> > out the page it failed on, as well as any later pages it may not
> > have gotten to yet.
> 
> Agreed. If it fails, could we just not flush the folio?

I believe that should be OK.  If returning an error, nobody should be
depending on any part of the page being present or not in the cache.
-- 
Mike Kravetz

> Like this:
> long copy_folio_from_user(...)
> {
> 	...
> 	for (i = 0; i < nr_pages; i++) {
> 		...
> 		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> 		...
> 		ret_val -= (PAGE_SIZE - rc);
> 		if (rc)
> -                       break;
> +                       return ret_val;
> 		cond_resched();
> 	}
> 	flush_dcache_folio(dst_folio);
> 	return ret_val;
> }
> 
> Thanks for your review.
> 
> Best Regards,
> Peng
>