| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Feb 2022 15:26:59 +0100
From: Jakub Matěna <matenajakub@...il.com>
To: Liam Howlett <liam.howlett@...cle.com>
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"patches@...ts.linux.dev" <patches@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"vbabka@...e.cz" <vbabka@...e.cz>,
"mhocko@...nel.org" <mhocko@...nel.org>,
"mgorman@...hsingularity.net" <mgorman@...hsingularity.net>,
"willy@...radead.org" <willy@...radead.org>,
"hughd@...gle.com" <hughd@...gle.com>,
"kirill@...temov.name" <kirill@...temov.name>,
"riel@...riel.com" <riel@...riel.com>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"peterz@...radead.org" <peterz@...radead.org>
Subject: Re: [RFC PATCH 1/4] [PATCH 1/4] mm: refactor of vma_merge()
On Fri, Feb 18, 2022 at 8:43 PM Liam Howlett <liam.howlett@...cle.com> wrote:
>
> * Jakub Matěna <matenajakub@...il.com> [220218 07:21]:
> > Refactor vma_merge() to make it shorter, more understandable and
> > suitable for tracing of successful merges made possible by following
> > patches in the series.
> >
> > Signed-off-by: Jakub Matěna <matenajakub@...il.com>
> > ---
> > mm/mmap.c | 81 +++++++++++++++++++++++++++----------------------------
> > 1 file changed, 39 insertions(+), 42 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 1e8fdb0b51ed..b55e11f20571 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1172,6 +1172,9 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
> > pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
> > struct vm_area_struct *area, *next;
> > int err;
> > + int merge_prev = 0;
> > + int merge_both = 0;
> > + int merge_next = 0;
>
> You set these as true, can you please use booleans?
As you mentioned in another email. This corresponds with one of the
following patches,
but you are right, booleans should be used here.
>
> >
> > /*
> > * We later require that vma->vm_flags == vm_flags,
> > @@ -1191,65 +1194,59 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
> > VM_WARN_ON(addr >= end);
> >
> > /*
> > - * Can it merge with the predecessor?
> > + * Can we merge predecessor?
> > */
> > if (prev && prev->vm_end == addr &&
> > mpol_equal(vma_policy(prev), policy) &&
> > can_vma_merge_after(prev, vm_flags,
> > anon_vma, file, pgoff,
> > vm_userfaultfd_ctx, anon_name)) {
> > - /*
> > - * OK, it can. Can we now merge in the successor as well?
> > - */
> > - if (next && end == next->vm_start &&
> > - mpol_equal(policy, vma_policy(next)) &&
> > - can_vma_merge_before(next, vm_flags,
> > - anon_vma, file,
> > - pgoff+pglen,
> > - vm_userfaultfd_ctx, anon_name) &&
> > - is_mergeable_anon_vma(prev->anon_vma,
> > - next->anon_vma, NULL)) {
> > - /* cases 1, 6 */
> > - err = __vma_adjust(prev, prev->vm_start,
> > - next->vm_end, prev->vm_pgoff, NULL,
> > - prev);
> > - } else /* cases 2, 5, 7 */
> > - err = __vma_adjust(prev, prev->vm_start,
> > - end, prev->vm_pgoff, NULL, prev);
> > - if (err)
> > - return NULL;
> > - khugepaged_enter_vma_merge(prev, vm_flags);
> > - return prev;
> > + merge_prev = true;
>
> You could set area = prev here and simplify the if statements below.
Thanks I will.
>
> > }
> > -
> > /*
> > - * Can this new request be merged in front of next?
> > + * Can we merge successor?
> > */
> > if (next && end == next->vm_start &&
> > mpol_equal(policy, vma_policy(next)) &&
> > can_vma_merge_before(next, vm_flags,
> > - anon_vma, file, pgoff+pglen,
> > - vm_userfaultfd_ctx, anon_name)) {
> > + anon_vma, file, pgoff+pglen,
> > + vm_userfaultfd_ctx, anon_name)) {
> > + merge_next = true;
> > + }
> > + /*
> > + * Can we merge both predecessor and successor?
> > + */
> > + if (merge_prev && merge_next)
> > + merge_both = is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL);
> > +
> > + if (merge_both) { /* cases 1, 6 */
> > + err = __vma_adjust(prev, prev->vm_start,
> > + next->vm_end, prev->vm_pgoff, NULL,
> > + prev);
> > + area = prev;
>
> I don't think you need all three booleans since merge_both is only used
> here.
This again corresponds with one of the following patches, but in this
patch merge_both is not needed.
>
> > + } else if (merge_prev) { /* cases 2, 5, 7 */
> > + err = __vma_adjust(prev, prev->vm_start,
> > + end, prev->vm_pgoff, NULL, prev);
> > + area = prev;
> > + } else if (merge_next) {
> > if (prev && addr < prev->vm_end) /* case 4 */
> > err = __vma_adjust(prev, prev->vm_start,
> > - addr, prev->vm_pgoff, NULL, next);
> > - else { /* cases 3, 8 */
> > + addr, prev->vm_pgoff, NULL, next);
> > + else /* cases 3, 8 */
> > err = __vma_adjust(area, addr, next->vm_end,
> > - next->vm_pgoff - pglen, NULL, next);
> > - /*
> > - * In case 3 area is already equal to next and
> > - * this is a noop, but in case 8 "area" has
> > - * been removed and next was expanded over it.
> > - */
>
> I think the above comment is still true?
Yes, it is. It just slipped away.
>
> > - area = next;
> > - }
> > - if (err)
> > - return NULL;
> > - khugepaged_enter_vma_merge(area, vm_flags);
> > - return area;
> > + next->vm_pgoff - pglen, NULL, next);
> > + area = next;
> > + } else {
> > + err = -1;
> > }
>
> If you initialize err to something, you can drop this else.
Thank you, that sounds better.
>
> >
> > - return NULL;
> > + /*
> > + * Cannot merge with predecessor or successor or error in __vma_adjust?
> > + */
> > + if (err)
> > + return NULL;
> > + khugepaged_enter_vma_merge(area, vm_flags);
> > + return area;
> > }
> >
> > /*
> > --
> > 2.34.1
> >
Powered by blists - more mailing lists