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 D0E1AC433FE for ; Fri, 11 Nov 2022 10:10:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6F33C6B0071; Fri, 11 Nov 2022 05:10:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6A2C46B0072; Fri, 11 Nov 2022 05:10:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 592916B0074; Fri, 11 Nov 2022 05:10:23 -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 46C8C6B0071 for ; Fri, 11 Nov 2022 05:10:23 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 117731408A8 for ; Fri, 11 Nov 2022 10:10:23 +0000 (UTC) X-FDA: 80120741526.21.38FBB49 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by imf17.hostedemail.com (Postfix) with ESMTP id 578AC40002 for ; Fri, 11 Nov 2022 10:10:22 +0000 (UTC) Received: from [192.168.10.9] (unknown [39.45.244.84]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: usama.anjum) by madras.collabora.co.uk (Postfix) with ESMTPSA id B18396602A53; Fri, 11 Nov 2022 10:10:07 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1668161420; bh=DB1GesM2rck4OsDAS8g+B+IHbx5lDoa3Do4KleeFlws=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=IDEPl4xDPWN1Mmh4XfPO+WNoKkG1xF8LGiBIMFbdST0NW+6Tb7ppH24M1kWslKPh0 W6uRkBomgqS9FBcRvNy+BWecuwnzTrlmM+iAFwghYXhUOB3JCPlBzX6DdY9Yk/Kweh BmQ6znY1PeIE2+/MNQHEbAEaluhBkUItUwpx9VUFKPKvj0FgHNVMJPV5emU4Myc//6 sOYwBER1RCM2k/4OvboWkxid9qFSuZMQ4cJSbwy0ABzOyE0alr2Vc3UtXznfQPoNad dkXkaDXtMgVXDj5JpHGqAJHbliUcNylbD4ncGcPwPiNvdV5rIeAC/zBiaRuHIKqCDM AFkNsqNFSuyKg== Message-ID: Date: Fri, 11 Nov 2022 15:10:01 +0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Cc: Muhammad Usama Anjum , =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Danylo Mocherniuk , Alexander Viro , Andrew Morton , Suren Baghdasaryan , Greg KH , Christian Brauner , Peter Xu , Yang Shi , Vlastimil Babka , Zach O'Keefe , "Matthew Wilcox (Oracle)" , "Gustavo A. R. Silva" , Dan Williams , kernel@collabora.com, Gabriel Krisman Bertazi , David Hildenbrand , Peter Enderborg , "open list : KERNEL SELFTEST FRAMEWORK" , Shuah Khan , open list , "open list : PROC FILESYSTEM" , "open list : MEMORY MANAGEMENT" , Paul Gofman Subject: Re: [PATCH v6 2/3] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs Content-Language: en-US To: Andrei Vagin References: <20221109102303.851281-1-usama.anjum@collabora.com> <20221109102303.851281-3-usama.anjum@collabora.com> From: Muhammad Usama Anjum In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1668161422; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=UO3fj+pnd8tMssPwGjanqT6AQiJEwPOcvbw+LkJZxog=; b=ChF0xm3uEP4lN6W9R6Obyb5C1TL7X39H1gTVKSCloW8E671kkBhBGu+TmktTnQRE09Yo1O Kw2ZmTljzpcw0aMcYnsDiLHMG2jLG34vBNb1VAsvSf1vpe6novuza610lHC13BUHuXc1mo 9NL7H9u1HroorwdR2Bo+G8DUXmbTesA= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=IDEPl4xD; spf=pass (imf17.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com; dmarc=pass (policy=none) header.from=collabora.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1668161422; a=rsa-sha256; cv=none; b=ZHmLwojGLjdbeceFOQ2wvUqnvJx/pQ0OCz93G3ArXyZbP9S2fE3L4soXS7pdJ/KlLn9uQH oejczSE5Ln+nMXOg8+lrphN7DVsz4L3e1wocNqU48x/Uq29mSlqRWCsmDxEbJpw3EJAD9X FwANOPaU2opUA2tX9MyYmGlSVxrPgfU= X-Rspam-User: X-Stat-Signature: xa4iyrhd5yad1j8cc5fng7auoezdo4mt X-Rspamd-Queue-Id: 578AC40002 Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=collabora.com header.s=mail header.b=IDEPl4xD; spf=pass (imf17.hostedemail.com: domain of usama.anjum@collabora.com designates 46.235.227.172 as permitted sender) smtp.mailfrom=usama.anjum@collabora.com; dmarc=pass (policy=none) header.from=collabora.com X-Rspamd-Server: rspam02 X-HE-Tag: 1668161422-969582 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: Hello Andrei, Thank you for reviewing. On 11/10/22 4:54 AM, Andrei Vagin wrote: [...] >> +static int add_to_out(bool sd, bool file, bool pres, bool swap, struct pagemap_scan_private *p, >> + unsigned long addr, unsigned int len) >> +{ >> + unsigned long bitmap, cur = sd | file << 1 | pres << 2 | swap << 3; > > Should we define contants for each of these bits? I think I can define a macro to hide this dirty bit shifting in the function. > >> + bool cpy = true; >> + >> + if (p->required_mask) >> + cpy = ((p->required_mask & cur) == p->required_mask); >> + if (cpy && p->anyof_mask) >> + cpy = (p->anyof_mask & cur); >> + if (cpy && p->excluded_mask) >> + cpy = !(p->excluded_mask & cur); >> + >> + bitmap = cur & p->return_mask; >> + >> + if (cpy && bitmap) { >> + if ((p->vec_index) && (p->vec[p->vec_index - 1].bitmap == bitmap) && >> + (p->vec[p->vec_index - 1].start + p->vec[p->vec_index - 1].len * PAGE_SIZE == >> + addr)) { > > I think it is better to define a variable for p->vec_index - 1. Will do in the next revision. > nit: len can be in bytes rather than pages. We are considering memory in the page units. The memory given to this IOCTL must have PAGE_SIZE alignment. Oterwise we error out (picked this from mincore()). >> +static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr, >> + unsigned long end, struct mm_walk *walk) >> +{ >> + struct pagemap_scan_private *p = walk->private; >> + struct vm_area_struct *vma = walk->vma; >> + unsigned int len; >> + spinlock_t *ptl; >> + int ret = 0; >> + pte_t *pte; >> + bool dirty_vma = (p->flags & PAGEMAP_NO_REUSED_REGIONS) ? >> + (false) : (vma->vm_flags & VM_SOFTDIRTY); >> + >> + if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages)) >> + return 0; >> + >> + end = min(end, walk->vma->vm_end); >> + >> + ptl = pmd_trans_huge_lock(pmd, vma); >> + if (ptl) { >> + if (dirty_vma || check_soft_dirty_pmd(vma, addr, pmd, false)) { >> + /* >> + * Break huge page into small pages if operation needs to be performed is >> + * on a portion of the huge page or the return buffer cannot store complete >> + * data. >> + */ >> + if ((IS_CLEAR_OP(p) && (end - addr < HPAGE_SIZE))) { >> + spin_unlock(ptl); >> + split_huge_pmd(vma, pmd, addr); >> + goto process_smaller_pages; >> + } >> + >> + if (IS_GET_OP(p)) { >> + len = (end - addr)/PAGE_SIZE; >> + if (p->max_pages && p->found_pages + len > p->max_pages) >> + len = p->max_pages - p->found_pages; >> + >> + ret = add_to_out(dirty_vma || >> + check_soft_dirty_pmd(vma, addr, pmd, false), > > can we reuse a return code of the previous call of check_soft_dirty_pmd? Yes, will do. > >> + vma->vm_file, pmd_present(*pmd), is_swap_pmd(*pmd), >> + p, addr, len); >> + } >> + if (!ret && IS_CLEAR_OP(p)) >> + check_soft_dirty_pmd(vma, addr, pmd, true); > > should we return a error in this case? We need to be sure that: > * we stop waking page tables after this point. I'll update the implementation to return error. It immediately terminates the walk as well. > * return this error to the user-space if we are not able to add anything > in the vector. I'm not returning error to userspace if we found no page matching the masks. The total number of filled page_region are returned from the IOCTL. If IOCTL returns 0, it means no page found has found, but the IOCTL executed successfully. [...] >> +static long do_pagemap_sd_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) >> +{ >> + struct mmu_notifier_range range; >> + unsigned long __user start, end; >> + struct pagemap_scan_private p; >> + int ret; >> + >> + start = (unsigned long)untagged_addr(arg->start); >> + if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len))) >> + return -EINVAL; >> + >> + if (IS_GET_OP(arg) && >> + ((arg->vec_len == 0) || (!access_ok((struct page_region *)arg->vec, arg->vec_len)))) >> + return -ENOMEM; >> + >> +#ifndef CONFIG_MEM_SOFT_DIRTY >> + if (IS_SD_OP(arg) || (arg->required_mask & PAGE_IS_SOFTDIRTY) || >> + (arg->anyof_mask & PAGE_IS_SOFTDIRTY)) >> + return -EINVAL; >> +#endif >> + >> + if ((arg->flags & ~PAGEMAP_SD_FLAGS) || (arg->required_mask & ~PAGEMAP_OP_MASK) || >> + (arg->anyof_mask & ~PAGEMAP_OP_MASK) || (arg->excluded_mask & ~PAGEMAP_OP_MASK) || >> + (arg->return_mask & ~PAGEMAP_OP_MASK)) >> + return -EINVAL; >> + >> + if ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) || !arg->return_mask) >> + return -EINVAL; >> + >> + if (IS_SD_OP(arg) && ((arg->required_mask & PAGEMAP_NONSD_OP_MASK) || >> + (arg->anyof_mask & PAGEMAP_NONSD_OP_MASK))) >> + return -EINVAL; >> + >> + end = start + arg->len; >> + p.max_pages = arg->max_pages; >> + p.found_pages = 0; >> + p.flags = arg->flags; >> + p.required_mask = arg->required_mask; >> + p.anyof_mask = arg->anyof_mask; >> + p.excluded_mask = arg->excluded_mask; >> + p.return_mask = arg->return_mask; >> + p.vec_index = 0; >> + p.vec_len = arg->vec_len; >> + >> + if (IS_GET_OP(arg)) { >> + p.vec = vzalloc(arg->vec_len * sizeof(struct page_region)); > > I think we need to set a reasonable limit for vec_len to avoid large > allocations on the kernel. We can consider to use kmalloc or kvmalloc > here. I'll update to kvzalloc which uses vmalloc if kmalloc fails. It'll use kmalloc for smaller allocations. Thanks for suggesting it. But it'll not limit the memory allocation. > > Thanks, > Andrei -- BR, Muhammad Usama Anjum