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 BFB90C7EE2F for ; Wed, 7 Jun 2023 16:53:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3E3D46B0072; Wed, 7 Jun 2023 12:53:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3938A8E0001; Wed, 7 Jun 2023 12:53:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 233D86B0075; Wed, 7 Jun 2023 12:53:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 117736B0072 for ; Wed, 7 Jun 2023 12:53:00 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B30F91603E2 for ; Wed, 7 Jun 2023 16:52:59 +0000 (UTC) X-FDA: 80876546478.12.A1E4BB9 Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by imf13.hostedemail.com (Postfix) with ESMTP id 4293D20008 for ; Wed, 7 Jun 2023 16:52:54 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=iwJUggxm; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf13.hostedemail.com: domain of emmir@google.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=emmir@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686156775; 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=kDPu/l1jQkL1v/hJOBAOUnasvA9zu2nCkrjHp2QAgLM=; b=wVakvbGscF0SPUg+/I04YHtOf/PE5tnI822hJsmio0Tt0XakQh3QXR1pp2TS6079K14jli AUtAPEU2q3DulI1NuBj7XOvR0mz85802djB/CHtmNl/K4BQrmSjPpITBXfpp2i4xzsuJco AA4tMb1StNVwo3OJLJpWv+PVTL/yITU= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=iwJUggxm; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf13.hostedemail.com: domain of emmir@google.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=emmir@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686156775; a=rsa-sha256; cv=none; b=itNgC03oX1tsOSTkmtoj7gLZTIN0puX4zHxqfqA0hj4Yqeyz0idEmHyERsiumfEgy+RBRM C4VP8QX85+vnncH9QZl4QP/FKj7l8kfV45RTDJc9QLjLVEBBevofFI07SMFQX2MRF6Ata5 I68/zKzEZleGagC35qWqa5QXFQ22I5U= Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-516500163b2so133a12.1 for ; Wed, 07 Jun 2023 09:52:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1686156773; x=1688748773; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=kDPu/l1jQkL1v/hJOBAOUnasvA9zu2nCkrjHp2QAgLM=; b=iwJUggxmjMfoM27ioWLRFV88/qcJLp1Oygu73i7qkmVtRPdM1GTkdOV0mLDei7dJou Tszz7TvhbzC6Jz9+7gHV5V7QAiGdiFnO9+fnSBTwFhURNAJJuLDSDAAZ+adkLbgyuI5V JUZLetMRYQNt4iOSw+EUjZvDw3+c3kYqbMM74nOemu+u1R+OUzEKQ7fkMCNzfaLN4z0k 5KzopE35KJ+BqV0b3RincLl3QesUZZCGe9+dpyv3edUGB+UtXniS/bwJNFFmjVyh3IW9 pgxm2oNOf16IWersIYGfY62yQfxgwwLwLHKTkOk5bdBm4oDUibP6dV0/sPt7XlsFEBXV XyZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686156773; x=1688748773; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=kDPu/l1jQkL1v/hJOBAOUnasvA9zu2nCkrjHp2QAgLM=; b=gnXnrlzQYkCLHt4Hgx9oKajyoAqgZeuLIocmGwTm5mwXt1bLS/p7tsiKwAQqa93jjf RE+GCyjgYPArSjJLw1mn27RlfKFLffkTbJs2oqBa6L9mPqc7qoz8U1aDPLcwWesKrY6Y Gn6qSP2OMgx3tFDqIAhAMIjiO8EnbZk0doFIC80CeH+umhJ833Jt9c4/TTg/jRHFyeHv S2AUfn2v1KPUQKdjuyTgXZIKBYfbhNBuD/Gq78JC4S/wLmE/s6fgKHDyht8MhoWDamyt 1wj1iPBFJfp8p7w9wETRsiogAeAKoBEmbDL7nphZmX89uBSZI5GBl8d6uN07k/+WjSwP 65fg== X-Gm-Message-State: AC+VfDzzH7iXu02j/+rMeMzdYJt9N9EiskTmByik84Fat1NIn57aKZn4 Zfw0OHlL7qUAU48PBtwnmLFcZGCXdM72y+y/RXwtDw== X-Google-Smtp-Source: ACHHUZ6yw6A9LT5KQ0IM2TL4O+4LveUckjZMTIyQKCOBlFMXKIWAT/Vq7U6GCPlCLWZN9p98qgeOkVVJjCH+qSayDa4= X-Received: by 2002:a50:bb2a:0:b0:502:7e99:a1a7 with SMTP id y39-20020a50bb2a000000b005027e99a1a7mr130646ede.1.1686156773093; Wed, 07 Jun 2023 09:52:53 -0700 (PDT) MIME-Version: 1.0 References: <20230606060822.1065182-1-usama.anjum@collabora.com> <20230606060822.1065182-3-usama.anjum@collabora.com> <0b8b19e7-fffa-aa1f-8479-e5a338780f7a@collabora.com> In-Reply-To: <0b8b19e7-fffa-aa1f-8479-e5a338780f7a@collabora.com> From: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Date: Wed, 7 Jun 2023 18:52:41 +0200 Message-ID: Subject: Re: [PATCH v17 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs To: Muhammad Usama Anjum Cc: Peter Xu , David Hildenbrand , Andrew Morton , Andrei Vagin , Danylo Mocherniuk , Paul Gofman , Cyrill Gorcunov , Mike Rapoport , Nadav Amit , Alexander Viro , Shuah Khan , Christian Brauner , Yang Shi , Vlastimil Babka , "Liam R . Howlett" , Yun Zhou , Suren Baghdasaryan , Alex Sierra , Matthew Wilcox , Pasha Tatashin , Axel Rasmussen , "Gustavo A . R . Silva" , Dan Williams , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, Greg KH , kernel@collabora.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 4293D20008 X-Stat-Signature: upgdmo4wgnzmarpkys7eajyqabqh7gd4 X-HE-Tag: 1686156774-871843 X-HE-Meta: U2FsdGVkX18TgG7s13y7lgsgQKMAuiFzBspX6un1Z4ER4XqWQ6iDcR7jYCqnVXSfPbGPzfAYYHkJjVl4KKAmuPB4k+VpLDiT/zJ0ldqiyVJmzL1LLphyVLJ057REkMCb3QCc06qayJBSmUX+2XCXyuTuzQjp+YVK+w48NESxKJZotAnjGHirwzm28G8yZ83uGRKJaCMl5W2AU6ogdPS7UHlWx1Gx5FZO006xYMnqraMwlj4mDaem8O19LuC09a5hNw2T+BYu1QctNfx97XTo+YTp/CTXzpxBif4csueYBAGc8UFqAWcMkQy2s629gqvAI5m2lmrdys0YOtNbmaOMudxHqTdg3YRaJrw6Oxh6srWWR4FFc9ibj7tQwuM39QAqD9Vo/wJfZzzKhbrESpBy4PLnHfZRG5ED2Euro7jUGadf2LrzrMpTpJWVSQ80hoSNmAXQ/LiTMWXT3fzRKd9Dr/t0YHQOORBYrkr8HFLpcG0yi8IHFDHxgAHJuqgFID6+08UZG+zfvdGu9UR/MtgRDMVyPDWSWAQ7sqbHB+sMDEGY6oPc+Y83tAgiYNmVxPwoJMDNdO5fAn5SwA6AetJgALoKXFC7Rxsl9EJSxOHHpLPC0+/WpT/wOF3RMyZWDj5olXRdouXPYTvJ2D776HUJ4g9Xov8PKSo0TGZ2YAhFSeA8+XZ2AaHD23W+2tnnCu0+rwU/j13UJ7dJ/x115nRp7Dx74xMKgC+mYDNWrkMesa/1FE7RL5y0EpKJMA+0uGXmQ9FrtSTCoFreX77YWB4qn2CsCyvC64Xu5PrUzvGNWSFxiUa+buSdg98qa55hIqtdUX9m+3kufTRrpkG88OEmTGMyIlUZFeRzHHT/pknFNv0o+nncEPAC/YyNGKChev52KqC2S/5zRd7owlpgK8nRCZCMPSUllPKyvIBlpSM0qJiECPTkeBPmoFkq2fcCWSnkN37FWhhPA24aMfP2c3v VG0BPAPu H2IEJTIzb1xyKCK1o9rsxfw/dDSkAbqBIPIwf0Jz/5bTRZ4YckPlJRvx4PSoqttUoCnniGmXWE18HSeQEC616LGW/NPnG5UtLBG6+6ZL1iOnxxHI1gYQm0nmcpsJj/OvYPvbZQABmv8sW92oUF88+EgmxdBV0abTFDWSMJjeaz0stIyx087ySh6WHB/YM4XzQeQ2EUP5fJw5CYHiR17LrYCrYYzsbGXSj6/LOBFdAB2HQT95NQahzrxcKYsK8n0Y8nDjFLXfy+Ac8OeExz2O5UnBYZxlThaDoxi6k/1Y1c2Whpj8FI10IYXVb7d+W9RzCxiqjPpOthnLxgeqLCueZTqvEqm9G9ov1LVdXlX4Pj/WCSBA= 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 Wed, 7 Jun 2023 at 18:13, Muhammad Usama Anjum wrote: > > Hi Micha=C5=82, > > Thank you for taking time to review! > > On 6/7/23 7:52=E2=80=AFPM, Micha=C5=82 Miros=C5=82aw wrote: > > On Tue, 6 Jun 2023 at 08:08, Muhammad Usama Anjum > > wrote: > >> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or cle= ar > >> the info about page table entries. The following operations are suppor= ted > >> in this ioctl: > >> - Get the information if the pages have been written-to (PAGE_IS_WRITT= EN), > >> file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped > >> (PAGE_IS_SWAPPED). > >> - Find pages which have been written-to and/or write protect the pages > >> (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) > >> > >> This IOCTL can be extended to get information about more PTE bits. > > [...] > >> --- a/fs/proc/task_mmu.c > >> +++ b/fs/proc/task_mmu.c [...] > >> +static inline bool pagemap_scan_check_page_written(struct pagemap_sca= n_private *p) > >> +{ > >> + return (p->required_mask | p->anyof_mask | p->excluded_mask) & > >> + PAGE_IS_WRITTEN; > >> +} > > > > This could be precalculated and put as a flag into > > pagemap_scan_private - it is kernel-private structure and there are a > > few spare bits in `flags` if you'd prefer not to add an explicit > > boolean. > This inline function is only being used at one spot. I can remove the > function altogether. I don't like putting it in flags. It'll bring some > complexity. The difference at the call site will be function call vs field access. Do you mean that moving the function to where the struct is initialized would add complexity? Why is that? > > [...] > >> +static int pagemap_scan_output(bool wt, bool file, bool pres, bool sw= ap, > >> + struct pagemap_scan_private *p, > >> + unsigned long addr, unsigned int n_page= s) > >> +{ > >> + unsigned long bitmap =3D PM_SCAN_BITMAP(wt, file, pres, swap); > >> + struct page_region *cur =3D &p->cur; > >> + > >> + if (!n_pages) > >> + return -EINVAL; > >> + > >> + if ((p->required_mask & bitmap) !=3D p->required_mask) > >> + return 0; > >> + if (p->anyof_mask && !(p->anyof_mask & bitmap)) > >> + return 0; > >> + if (p->excluded_mask & bitmap) > >> + return 0; > >> + > >> + bitmap &=3D p->return_mask; > >> + if (!bitmap) > >> + return 0; > >> + > >> + if (cur->bitmap =3D=3D bitmap && > >> + cur->start + cur->len * PAGE_SIZE =3D=3D addr) { > >> + cur->len +=3D n_pages; > >> + p->found_pages +=3D n_pages; > >> + } else { > >> + /* > >> + * All data is copied to cur first. When more data is = found, we > >> + * push cur to vec and copy new data to cur. The vec_i= ndex > >> + * represents the current index of vec array. We add 1= to the > >> + * vec_index while performing checks to account for da= ta in cur. > >> + */ > >> + if (cur->len && (p->vec_index + 1) >=3D p->vec_len) > >> + return -ENOSPC; > >> + > >> + if (cur->len) { > >> + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->= vec)); > >> + p->vec_index++; > >> + } > >> + > >> + cur->start =3D addr; > >> + cur->len =3D n_pages; > >> + cur->bitmap =3D bitmap; > >> + p->found_pages +=3D n_pages; > >> + } > >> + > >> + if (p->max_pages && (p->found_pages =3D=3D p->max_pages)) > >> + return PM_SCAN_FOUND_MAX_PAGES; > >> + > >> + return 0; > >> +} > >> + > >> +static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start, > >> + unsigned long end, struct mm_walk *w= alk) > >> +{ > >> + struct pagemap_scan_private *p =3D walk->private; > >> + struct vm_area_struct *vma =3D walk->vma; > >> + unsigned long addr =3D end; > >> + pte_t *pte, *orig_pte; > >> + spinlock_t *ptl; > >> + bool is_written; > >> + int ret =3D 0; > >> + > >> + arch_enter_lazy_mmu_mode(); > >> + > >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > >> + ptl =3D pmd_trans_huge_lock(pmd, vma); > >> + if (ptl) { > >> + unsigned long n_pages =3D (end - start)/PAGE_SIZE; > >> + > >> + if (p->max_pages && n_pages > p->max_pages - p->found_= pages) > >> + n_pages =3D p->max_pages - p->found_pages; > > > > Since p->found_pages is only ever increased in `pagemap_scan_output()` > > and that function is only called for GET or GET+WP operations, maybe > > the logic could be folded to pagemap_scan_output() to avoid > > duplication? > > In this function the calculation is used only when WP op is done to > > split the HP if n_pages limit would be hit, but if using plain WP > > (without GET) it doesn't make sense to use the limit. > The n_pages is needed to decide if THP need to be broken down and it is > used in pagemap_scan_output(). I've brought this condition out of > pagemap_scan_output() to cater this former condition. If I move it to > pagemap_scan_output(), I'll have to write same condition to find out if I > need to breakt he THP. This seems like repetition, but we have same use > case for tlbhuge page. My point is that you need to split the THP only if doing a GET+WP operation. If you only do GET, then the worst case would be for the process to report a spurious WRITTEN bit if an earlier-visited part of THP was modified and the scan restarted in the middle of a THP. >> > (pagemap_scan_output() is trivial enough so I think it could be pulled > > inside the spinlocked region.) > It is already in spinlocked region. Spin lock is being released after tlb > flush. Ah, I was thinking about calling pagemap_scan_output() before checking split_huge_pmd() case - and at that use the pagemap_scan_output()'s return value to do the check. > >> + is_written =3D !is_pmd_uffd_wp(*pmd); > >> + > >> + /* > >> + * Break huge page into small pages if the WP operatio= n need to > >> + * be performed is on a portion of the huge page. > >> + */ > >> + if (is_written && IS_PM_SCAN_WP(p->flags) && > >> + n_pages < HPAGE_SIZE/PAGE_SIZE) { > >> + spin_unlock(ptl); > >> + > >> + split_huge_pmd(vma, pmd, start); > >> + goto process_smaller_pages; > >> + } > >> + > >> + if (IS_PM_SCAN_GET(p->flags)) > >> + ret =3D pagemap_scan_output(is_written, vma->v= m_file, > >> + pmd_present(*pmd), > >> + is_swap_pmd(*pmd), > >> + p, start, n_pages); > >> + > >> + if (ret >=3D 0 && is_written && IS_PM_SCAN_WP(p->flags= )) > >> + make_uffd_wp_pmd(vma, addr, pmd); > >> + > >> + if (IS_PM_SCAN_WP(p->flags)) > > > > Why `is_written` is not checked? If is_written is false, then the WP > > op should be a no-op and so won't need TLB flushing, will it? [Same > > for the PTE case below.] > It can be done for THP. But for ptes we cannot trust is_written as > is_written only represent last pte state. Ok, so the PTE case could use a flag recording whether any PTE had WP applied instead of `is_written`. > >> + flush_tlb_range(vma, start, end); > >> + > > [...] > >> + if (IS_PM_SCAN_WP(p->flags)) > >> + flush_tlb_range(vma, start, addr); > >> + > >> + pte_unmap_unlock(orig_pte, ptl); > >> + arch_leave_lazy_mmu_mode(); > >> + > >> + cond_resched(); > >> + return ret; > >> +} > >> + > >> +#ifdef CONFIG_HUGETLB_PAGE > >> +static int pagemap_scan_hugetlb_entry(pte_t *ptep, unsigned long hmas= k, > >> + unsigned long start, unsigned lo= ng end, > >> + struct mm_walk *walk) > >> +{ > >> + unsigned long n_pages =3D (end - start)/PAGE_SIZE; > >> + struct pagemap_scan_private *p =3D walk->private; > >> + struct vm_area_struct *vma =3D walk->vma; > >> + struct hstate *h =3D hstate_vma(vma); > >> + spinlock_t *ptl; > >> + bool is_written; > >> + int ret =3D 0; > >> + pte_t pte; > >> + > >> + if (p->max_pages && n_pages > p->max_pages - p->found_pages) > >> + n_pages =3D p->max_pages - p->found_pages; > >> + > >> + if (IS_PM_SCAN_WP(p->flags)) { > >> + i_mmap_lock_write(vma->vm_file->f_mapping); > >> + ptl =3D huge_pte_lock(h, vma->vm_mm, ptep); > >> + } > >> + > >> + pte =3D huge_ptep_get(ptep); > >> + is_written =3D !is_huge_pte_uffd_wp(pte); > >> + > >> + /* > >> + * Partial hugetlb page clear isn't supported > >> + */ > >> + if (is_written && IS_PM_SCAN_WP(p->flags) && > >> + n_pages < HPAGE_SIZE/PAGE_SIZE) { > >> + ret =3D -EPERM; > > > > Shouldn't this be ENOSPC, conveying that the operation would overflow > > the n_pages limit? > We are testing here is user has asked us to engage WP on a part of the > hugetlb or we can only perform WP on a part of the engage as user buffer = is > full. We cannot judge this has happened because of the former or later > condition. So I'm assuming that user's parameters aren't solid enough and > returning -EPERM. It seemed more suitable to me. But I can return -ENOSPC > as well, if you say? Those two cases can be differentiated when checked before truncating n_pages. If a user requests partial WP for a hugetlb page wouldn't EINVAL (or other error - as this can't ever work) be more appropriate (this check could happen only at the start of scan)? If the request is due to max_pages limit (with found_pages > 0), then I'd return ENOSPC and expect the user to restart the scan with a new buffer. Our discussion here makes me wonder: what is the expected return value for the ioctl WP (without GET) operation? If it would return e.g. the number of 4K-pages successfully scanned, then the caller would be able to detect the partial tail hugepage case and act accordingly. > >> +static int pagemap_scan_pte_hole(unsigned long addr, unsigned long en= d, > >> + int depth, struct mm_walk *walk) > >> +{ > >> + unsigned long n_pages =3D (end - addr)/PAGE_SIZE; > >> + struct pagemap_scan_private *p =3D walk->private; > >> + struct vm_area_struct *vma =3D walk->vma; > >> + int ret =3D 0; > >> + > >> + if (!vma || !IS_PM_SCAN_GET(p->flags)) > >> + return 0; > >> + > >> + if (p->max_pages && n_pages > p->max_pages - p->found_pages) > >> + n_pages =3D p->max_pages - p->found_pages; > > > > Nit: If the page flags don't match (wouldn't be output), the limit > > would not be hit and the calculation is unnecessary. But if it was > > done in pagemap_scan_output() instead after all the flags checks... > Correct for this use case. But moving to pagemap_scan_output() would make > me do duplicate calculation for other 2 cases as explained above. (responded below the cases above) > >> + ret =3D pagemap_scan_output(false, vma->vm_file, false, false,= p, addr, > >> + n_pages); > >> + > >> + return ret; > >> +} > > [...] > >> +static long do_pagemap_scan(struct mm_struct *mm, > >> + struct pm_scan_arg __user *uarg) > >> +{ > >> + unsigned long start, end, walk_start, walk_end; > >> + unsigned long empty_slots, vec_index =3D 0; > >> + struct mmu_notifier_range range; > >> + struct page_region __user *vec; > >> + struct pagemap_scan_private p; > >> + struct pm_scan_arg arg; > >> + int ret =3D 0; > >> + > >> + if (copy_from_user(&arg, uarg, sizeof(arg))) > >> + return -EFAULT; > >> + > >> + start =3D untagged_addr((unsigned long)arg.start); > >> + vec =3D (struct page_region *)untagged_addr((unsigned long)arg= .vec); > >> + > >> + ret =3D pagemap_scan_args_valid(&arg, start, vec); > >> + if (ret) > >> + return ret; > >> + > >> + end =3D start + arg.len; > >> + p.max_pages =3D arg.max_pages; > >> + p.found_pages =3D 0; > >> + p.flags =3D arg.flags; > >> + p.required_mask =3D arg.required_mask; > >> + p.anyof_mask =3D arg.anyof_mask; > >> + p.excluded_mask =3D arg.excluded_mask; > >> + p.return_mask =3D arg.return_mask; > >> + p.cur.start =3D p.cur.len =3D p.cur.bitmap =3D 0; > >> + p.vec =3D NULL; > >> + p.vec_len =3D PAGEMAP_WALK_SIZE >> PAGE_SHIFT; > > > > If p.vec_len would not count the entry held in `cur` (IOW: vec_len =3D > > WALK_SIZE - 1), then pagemap_scan_output() wouldn't need the big > > comment about adding or subtracting 1 when checking for overflow. The > > output vector needs to have space for at least one entrry to make GET > > useful. Maybe `cur` could be renamed or annotated to express that it > > always holds the last entry? > Ohhh.. This can be done by doing subtracting 1 from empty_slots. But I've > explored the idea. I don't see any benefit. If we do this, then I'll have > to put a comment why subtracting 1 is needed. Seems like same problem: > > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1909,7 +1909,7 @@ static int pagemap_scan_output(bool wt, bool file, > bool pres, bool swap, > * represents the current index of vec array. We add 1 to= the > * vec_index while performing checks to account for data = in > cur. > */ > - if (cur->len && (p->vec_index + 1) >=3D p->vec_len) > + if (cur->len && p->vec_index >=3D p->vec_len) > return -ENOSPC; > > if (cur->len) { > @@ -2202,7 +2202,7 @@ static long do_pagemap_scan(struct mm_struct *mm, > if (IS_PM_SCAN_GET(p.flags)) { > p.vec_index =3D 0; > > - empty_slots =3D arg.vec_len - vec_index; > + empty_slots =3D arg.vec_len - 1 - vec_index; > p.vec_len =3D min(p.vec_len, empty_slots); > } > > Lets leave it as it is. I can change `cur` to `last` or any other name. > Please suggest. The difference is that you have the subtraction only once per the outer page_walk loop iteration, but in the current version the addition has to happen every pagemap_scan_output() call after a hole. >From the readability perspective, "if (next_index >=3D vec_len)" is short and self-documenting. Also I'd use "p.vec_len =3D min(p.vec_len, empty_slots - 1)" as it also conveys the intent better (in that `vec` is holding all but the last entry, but arg.vec_len holds the final output buffer length). If we're picking colors, then maybe make `arg.vec_len` have a different name than `p.vec_len` (same for `vec_index`) so that there is less confusion possible as to what they refer to. Maybe keep `arg.vec_len`, but have `p.vec_buf`, `p.vec_buf_len`, and `p.next_buf_index`? (Note: you'd also need to decrement p.vec_len where it is first assigned.) [...] Best Regards Micha=C5=82 Miros=C5=82aw