Message ID | 20240111021526.3461825-1-yajun.deng@linux.dev |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2411:b0:101:2151:f287 with SMTP id m17csp1182999dyi; Wed, 10 Jan 2024 18:16:08 -0800 (PST) X-Google-Smtp-Source: AGHT+IFRxOCYYY5dICZ6LdEIlPQVYS5JDK1YQMlf30bxTrNtjXlyClWSXDEAwqJ9lcovO36qDHXB X-Received: by 2002:a05:620a:3621:b0:783:24cf:f426 with SMTP id da33-20020a05620a362100b0078324cff426mr496682qkb.78.1704939368166; Wed, 10 Jan 2024 18:16:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704939368; cv=none; d=google.com; s=arc-20160816; b=Yp4UZSUyQn4l6iRtoVcH9YKgcFffaz93/erqPLnp7K+FoLB3XgiCsUoK5LE3YsJwrC 8hUqkFJNV+pMgUrUf1MtEsCIPm/L6GajESkUIIyhLoTVq/kFZgUiyEMySgzz28KqfiKv jBBqcZBg/SYzR+D+tFFgkOkvt8oj2PWUCt52QmyySHpk2s1+3Ajs6VFrUdlpIjTngyKb iU7OMg39KhIQDM22sh61Z4ZMhGRG6yXhN+jvk5tW5h+3/138r7HlRdVBDzWKRPH9R4ar 0ct18hzP1OU9mbTf5HbuKsF7HVQeqpens8cz1FH8QrEFFsy0pfcwCdvECwU1tVEfPugF GUKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=o0NgjRU2NV7JHGIppMqbyQQ2ZDc1uYffbVtkLp09wOM=; fh=KPJjYJHLC+uLVMTRQa7GdG2CYCH/otyIF0MYaE50dhE=; b=yphqxUDheqnrW3xwdtN8V+6qmesN6pXMRMmLEQsFM0MTS3+legvN4yr0Adlb1uMRO1 zOD70ZoyUCqdxwOflVktLpvFKGa3JuGPLq3ZGlnW02dTCtz9mwQSRUfh5C5oNhwGvcJc rsEhV2s6qEcsK3aihF332B7l4vC/ZSYPTrMJN/f4+V2Ga3OQnYaoNapKQHzmWwF1fGCz v2kxtVKKAqQ9fbUEC7UlIiKnW+510Scueiq5krGEjvY/AT48zJxIlUHc13bXqlQmrI+q zT6r/yckCw7nJb1heNmkRMxa4ozyR/yDss7WhvqGi6whpzFU/COI1SYbdpl+/M3/oo5Z 9QcA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=a46xlnMp; spf=pass (google.com: domain of linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id j27-20020a05620a0a5b00b0078335c34beesi56859qka.595.2024.01.10.18.16.08 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Jan 2024 18:16:08 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@linux.dev header.s=key1 header.b=a46xlnMp; spf=pass (google.com: domain of linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-22961-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linux.dev Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id F0EFD1C2295F for <ouuuleilei@gmail.com>; Thu, 11 Jan 2024 02:16:07 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id D727A1C31; Thu, 11 Jan 2024 02:15:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="a46xlnMp" Received: from out-173.mta0.migadu.com (out-173.mta0.migadu.com [91.218.175.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0E56BEC9 for <linux-kernel@vger.kernel.org>; Thu, 11 Jan 2024 02:15:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1704939339; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=o0NgjRU2NV7JHGIppMqbyQQ2ZDc1uYffbVtkLp09wOM=; b=a46xlnMprMCg8OaJGKL+IpPWkj4c0zSutZ2JLvyGTBnlR0FJekhX1FZmtHMxnd01eYfgr4 Xr3pH+cgzIR422DF5vMG4EZr3DC6jkGwJBD3ZO4E0dR3vTXeYb895kwAPh8U69zp2S+z54 pFwqFTAhAjKCTvYiyKzLQc04FJZh0YA= From: Yajun Deng <yajun.deng@linux.dev> To: akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Yajun Deng <yajun.deng@linux.dev> Subject: [PATCH] mm/mmap: introduce vma_range_init() Date: Thu, 11 Jan 2024 10:15:26 +0800 Message-Id: <20240111021526.3461825-1-yajun.deng@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787758502850622486 X-GMAIL-MSGID: 1787758502850622486 |
Series |
mm/mmap: introduce vma_range_init()
|
|
Commit Message
Yajun Deng
Jan. 11, 2024, 2:15 a.m. UTC
There is a lot of code needs to set the range of vma, introduce
vma_range_init() to initialize the range of vma.
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
include/linux/mm.h | 9 +++++++++
mm/mmap.c | 29 +++++++----------------------
2 files changed, 16 insertions(+), 22 deletions(-)
Comments
* Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: > There is a lot of code needs to set the range of vma, introduce > vma_range_init() to initialize the range of vma. > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > --- > include/linux/mm.h | 9 +++++++++ > mm/mmap.c | 29 +++++++---------------------- > 2 files changed, 16 insertions(+), 22 deletions(-) This isn't a whole lot of code, are there others? We're losing code clarity in favour of saving 6 lines? > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index f5a97dec5169..abb4534be3cc 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, > return (vma && vma->vm_start <= start && end <= vma->vm_end); > } > > +static inline void vma_range_init(struct vm_area_struct *vma, Any reason this can't be in mm/internal.h ? vma_range_set(), vma_set_range(), or just vma_range() might be a better name? My thinking is that some of these are actually modifying the vma and not just initializing it, right? > + unsigned long start, unsigned long end, > + pgoff_t pgoff) > +{ > + vma->vm_start = start; > + vma->vm_end = end; > + vma->vm_pgoff = pgoff; > +} > + > #ifdef CONFIG_MMU > pgprot_t vm_get_page_prot(unsigned long vm_flags); > void vma_set_page_prot(struct vm_area_struct *vma); > diff --git a/mm/mmap.c b/mm/mmap.c > index 06f1f3e88598..0d3f4612d001 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -663,9 +663,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, > > vma_prepare(&vp); > vma_adjust_trans_huge(vma, start, end, 0); > - vma->vm_start = start; > - vma->vm_end = end; > - vma->vm_pgoff = pgoff; > + vma_range_init(vma, start, end, pgoff); > vma_iter_store(vmi, vma); > > vma_complete(&vp, vmi, vma->vm_mm); > @@ -708,9 +706,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, > vma_adjust_trans_huge(vma, start, end, 0); > > vma_iter_clear(vmi); > - vma->vm_start = start; > - vma->vm_end = end; > - vma->vm_pgoff = pgoff; > + vma_range_init(vma, start, end, pgoff); > vma_complete(&vp, vmi, vma->vm_mm); > return 0; > } > @@ -1015,10 +1011,7 @@ static struct vm_area_struct > > vma_prepare(&vp); > vma_adjust_trans_huge(vma, vma_start, vma_end, adj_start); > - > - vma->vm_start = vma_start; > - vma->vm_end = vma_end; > - vma->vm_pgoff = vma_pgoff; > + vma_range_init(vma, vma_start, vma_end, vma_pgoff); > > if (vma_expanded) > vma_iter_store(vmi, vma); > @@ -2811,11 +2804,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > } > > vma_iter_config(&vmi, addr, end); > - vma->vm_start = addr; > - vma->vm_end = end; > + vma_range_init(vma, addr, end, pgoff); > vm_flags_init(vma, vm_flags); > vma->vm_page_prot = vm_get_page_prot(vm_flags); > - vma->vm_pgoff = pgoff; > > if (file) { > vma->vm_file = get_file(file); > @@ -3165,9 +3156,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, > goto unacct_fail; > > vma_set_anonymous(vma); > - vma->vm_start = addr; > - vma->vm_end = addr + len; > - vma->vm_pgoff = addr >> PAGE_SHIFT; > + vma_range_init(vma, addr, addr + len, addr >> PAGE_SHIFT); > vm_flags_init(vma, flags); > vma->vm_page_prot = vm_get_page_prot(flags); > vma_start_write(vma); > @@ -3404,9 +3393,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap, > new_vma = vm_area_dup(vma); > if (!new_vma) > goto out; > - new_vma->vm_start = addr; > - new_vma->vm_end = addr + len; > - new_vma->vm_pgoff = pgoff; > + vma_range_init(new_vma, addr, addr + len, pgoff); > if (vma_dup_policy(vma, new_vma)) > goto out_free_vma; > if (anon_vma_clone(new_vma, vma)) > @@ -3574,9 +3561,7 @@ static struct vm_area_struct *__install_special_mapping( > if (unlikely(vma == NULL)) > return ERR_PTR(-ENOMEM); > > - vma->vm_start = addr; > - vma->vm_end = addr + len; > - > + vma_range_init(vma, addr, addr + len, 0); > vm_flags_init(vma, (vm_flags | mm->def_flags | > VM_DONTEXPAND | VM_SOFTDIRTY) & ~VM_LOCKED_MASK); > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > -- > 2.25.1 > >
On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: > * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: > > There is a lot of code needs to set the range of vma, introduce > > vma_range_init() to initialize the range of vma. > > > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > --- > > include/linux/mm.h | 9 +++++++++ > > mm/mmap.c | 29 +++++++---------------------- > > 2 files changed, 16 insertions(+), 22 deletions(-) > > This isn't a whole lot of code, are there others? We're losing code > clarity in favour of saving 6 lines? > Oh. I thought it was a nice cleanup which made things more clear. > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index f5a97dec5169..abb4534be3cc 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, > > return (vma && vma->vm_start <= start && end <= vma->vm_end); > > } > > > > +static inline void vma_range_init(struct vm_area_struct *vma, > > Any reason this can't be in mm/internal.h ? That would be good. > vma_range_set(), vma_set_range(), or just vma_range() might be a better > name? My thinking is that some of these are actually modifying the vma > and not just initializing it, right? I'd vote for vma_set_range().
* Andrew Morton <akpm@linux-foundation.org> [240122 18:40]: > On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: > > > * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: > > > There is a lot of code needs to set the range of vma, introduce > > > vma_range_init() to initialize the range of vma. > > > > > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > > --- > > > include/linux/mm.h | 9 +++++++++ > > > mm/mmap.c | 29 +++++++---------------------- > > > 2 files changed, 16 insertions(+), 22 deletions(-) > > > > This isn't a whole lot of code, are there others? We're losing code > > clarity in favour of saving 6 lines? > > > > Oh. I thought it was a nice cleanup which made things more clear. I'm not totally against it; that's why I suggested the changes below. I think a name change would go a long way for clarity. It's not as much as I though it would be though. > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index f5a97dec5169..abb4534be3cc 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, > > > return (vma && vma->vm_start <= start && end <= vma->vm_end); > > > } > > > > > > +static inline void vma_range_init(struct vm_area_struct *vma, > > > > Any reason this can't be in mm/internal.h ? > > That would be good. One other thing, do we trust this to be inlined correctly by the compiler or should this be __always_inline? I'd expect it to be okay as it is, but I've been proven wrong in a perf trace before.. > > > vma_range_set(), vma_set_range(), or just vma_range() might be a better > > name? My thinking is that some of these are actually modifying the vma > > and not just initializing it, right? > > I'd vote for vma_set_range(). > Using vma_set_range() leaves vma_range() or vma_size(), which could be added for the calculations of vma->vm_end - vma->vm_start. Davidlohr suggested such a beast a few years ago, but that one would need to live in the include/linux/mm.h as it occurs a lot more. $ git grep "vma->vm_end - vma->vm_start" | wc -l 198 . for just those named vma.
On 2024/1/23 08:18, Liam R. Howlett wrote: > * Andrew Morton <akpm@linux-foundation.org> [240122 18:40]: >> On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: >> >>> * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: >>>> There is a lot of code needs to set the range of vma, introduce >>>> vma_range_init() to initialize the range of vma. >>>> >>>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >>>> --- >>>> include/linux/mm.h | 9 +++++++++ >>>> mm/mmap.c | 29 +++++++---------------------- >>>> 2 files changed, 16 insertions(+), 22 deletions(-) >>> This isn't a whole lot of code, are there others? We're losing code >>> clarity in favour of saving 6 lines? >>> >> Oh. I thought it was a nice cleanup which made things more clear. > I'm not totally against it; that's why I suggested the changes below. I > think a name change would go a long way for clarity. It's not as much as > I though it would be though. > >>>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>>> index f5a97dec5169..abb4534be3cc 100644 >>>> --- a/include/linux/mm.h >>>> +++ b/include/linux/mm.h >>>> @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, >>>> return (vma && vma->vm_start <= start && end <= vma->vm_end); >>>> } >>>> >>>> +static inline void vma_range_init(struct vm_area_struct *vma, >>> Any reason this can't be in mm/internal.h ? >> That would be good. > One other thing, do we trust this to be inlined correctly by the > compiler or should this be __always_inline? I'd expect it to be okay as > it is, but I've been proven wrong in a perf trace before.. > Okay, I would take __always_inline and put it in mm/internal.h in v2. >>> vma_range_set(), vma_set_range(), or just vma_range() might be a better >>> name? My thinking is that some of these are actually modifying the vma >>> and not just initializing it, right? >> I'd vote for vma_set_range(). >> > Using vma_set_range() leaves vma_range() or vma_size(), which could be > added for the calculations of vma->vm_end - vma->vm_start. Davidlohr > suggested such a beast a few years ago, but that one would need to live > in the include/linux/mm.h as it occurs a lot more. > > $ git grep "vma->vm_end - vma->vm_start" | wc -l > 198 > > .. for just those named vma. >
* Yajun Deng <yajun.deng@linux.dev> [240122 21:23]: > > On 2024/1/23 08:18, Liam R. Howlett wrote: > > * Andrew Morton <akpm@linux-foundation.org> [240122 18:40]: > > > On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: > > > > > > > * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: > > > > > There is a lot of code needs to set the range of vma, introduce > > > > > vma_range_init() to initialize the range of vma. > > > > > > > > > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > > > > --- > > > > > include/linux/mm.h | 9 +++++++++ > > > > > mm/mmap.c | 29 +++++++---------------------- > > > > > 2 files changed, 16 insertions(+), 22 deletions(-) > > > > This isn't a whole lot of code, are there others? We're losing code > > > > clarity in favour of saving 6 lines? > > > > > > > Oh. I thought it was a nice cleanup which made things more clear. > > I'm not totally against it; that's why I suggested the changes below. I > > think a name change would go a long way for clarity. It's not as much as > > I though it would be though. > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > > > index f5a97dec5169..abb4534be3cc 100644 > > > > > --- a/include/linux/mm.h > > > > > +++ b/include/linux/mm.h > > > > > @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, > > > > > return (vma && vma->vm_start <= start && end <= vma->vm_end); > > > > > } > > > > > +static inline void vma_range_init(struct vm_area_struct *vma, > > > > Any reason this can't be in mm/internal.h ? > > > That would be good. > > One other thing, do we trust this to be inlined correctly by the > > compiler or should this be __always_inline? I'd expect it to be okay as > > it is, but I've been proven wrong in a perf trace before.. > > > > Okay, I would take __always_inline and put it in mm/internal.h in v2. I'm not confident in this suggestion as the rest. Please rename the function when you move it. > > > > > vma_range_set(), vma_set_range(), or just vma_range() might be a better > > > > name? My thinking is that some of these are actually modifying the vma > > > > and not just initializing it, right? > > > I'd vote for vma_set_range(). ^ This part, use vma_set_range() please. > > Using vma_set_range() leaves vma_range() or vma_size(), which could be > > added for the calculations of vma->vm_end - vma->vm_start. Davidlohr > > suggested such a beast a few years ago, but that one would need to live > > in the include/linux/mm.h as it occurs a lot more. > > > > $ git grep "vma->vm_end - vma->vm_start" | wc -l > > 198 > > > > .. for just those named vma. > > >
On 2024/1/23 11:19, Liam R. Howlett wrote: > * Yajun Deng <yajun.deng@linux.dev> [240122 21:23]: >> On 2024/1/23 08:18, Liam R. Howlett wrote: >>> * Andrew Morton <akpm@linux-foundation.org> [240122 18:40]: >>>> On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: >>>> >>>>> * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: >>>>>> There is a lot of code needs to set the range of vma, introduce >>>>>> vma_range_init() to initialize the range of vma. >>>>>> >>>>>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >>>>>> --- >>>>>> include/linux/mm.h | 9 +++++++++ >>>>>> mm/mmap.c | 29 +++++++---------------------- >>>>>> 2 files changed, 16 insertions(+), 22 deletions(-) >>>>> This isn't a whole lot of code, are there others? We're losing code >>>>> clarity in favour of saving 6 lines? >>>>> >>>> Oh. I thought it was a nice cleanup which made things more clear. >>> I'm not totally against it; that's why I suggested the changes below. I >>> think a name change would go a long way for clarity. It's not as much as >>> I though it would be though. >>> >>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>>>>> index f5a97dec5169..abb4534be3cc 100644 >>>>>> --- a/include/linux/mm.h >>>>>> +++ b/include/linux/mm.h >>>>>> @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, >>>>>> return (vma && vma->vm_start <= start && end <= vma->vm_end); >>>>>> } >>>>>> +static inline void vma_range_init(struct vm_area_struct *vma, >>>>> Any reason this can't be in mm/internal.h ? >>>> That would be good. >>> One other thing, do we trust this to be inlined correctly by the >>> compiler or should this be __always_inline? I'd expect it to be okay as >>> it is, but I've been proven wrong in a perf trace before.. >>> >> Okay, I would take __always_inline and put it in mm/internal.h in v2. > I'm not confident in this suggestion as the rest. > Please rename the function when you move it. inline is a suggestion, __always_inline is mandatory. I think __always_inline is better if we're demanding. >>>>> vma_range_set(), vma_set_range(), or just vma_range() might be a better >>>>> name? My thinking is that some of these are actually modifying the vma >>>>> and not just initializing it, right? >>>> I'd vote for vma_set_range(). > ^ This part, use vma_set_range() please. Okay. By the way, I sent another patch with ("mm/mmap: simplify vma_merge()") subject a few days ago. Please comment if you would like. >>> Using vma_set_range() leaves vma_range() or vma_size(), which could be >>> added for the calculations of vma->vm_end - vma->vm_start. Davidlohr >>> suggested such a beast a few years ago, but that one would need to live >>> in the include/linux/mm.h as it occurs a lot more. >>> >>> $ git grep "vma->vm_end - vma->vm_start" | wc -l >>> 198 >>> >>> .. for just those named vma. >>>
* Yajun Deng <yajun.deng@linux.dev> [240122 22:41]: > > On 2024/1/23 11:19, Liam R. Howlett wrote: > > * Yajun Deng <yajun.deng@linux.dev> [240122 21:23]: > > > On 2024/1/23 08:18, Liam R. Howlett wrote: > > > > * Andrew Morton <akpm@linux-foundation.org> [240122 18:40]: > > > > > On Mon, 22 Jan 2024 17:00:31 -0500 "Liam R. Howlett" <Liam.Howlett@Oracle.com> wrote: > > > > > > > > > > > * Yajun Deng <yajun.deng@linux.dev> [240110 21:15]: > > > > > > > There is a lot of code needs to set the range of vma, introduce > > > > > > > vma_range_init() to initialize the range of vma. > > > > > > > > > > > > > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > > > > > > --- > > > > > > > include/linux/mm.h | 9 +++++++++ > > > > > > > mm/mmap.c | 29 +++++++---------------------- > > > > > > > 2 files changed, 16 insertions(+), 22 deletions(-) > > > > > > This isn't a whole lot of code, are there others? We're losing code > > > > > > clarity in favour of saving 6 lines? > > > > > > > > > > > Oh. I thought it was a nice cleanup which made things more clear. > > > > I'm not totally against it; that's why I suggested the changes below. I > > > > think a name change would go a long way for clarity. It's not as much as > > > > I though it would be though. > > > > > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > > > > > index f5a97dec5169..abb4534be3cc 100644 > > > > > > > --- a/include/linux/mm.h > > > > > > > +++ b/include/linux/mm.h > > > > > > > @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, > > > > > > > return (vma && vma->vm_start <= start && end <= vma->vm_end); > > > > > > > } > > > > > > > +static inline void vma_range_init(struct vm_area_struct *vma, > > > > > > Any reason this can't be in mm/internal.h ? > > > > > That would be good. > > > > One other thing, do we trust this to be inlined correctly by the > > > > compiler or should this be __always_inline? I'd expect it to be okay as > > > > it is, but I've been proven wrong in a perf trace before.. > > > > > > > Okay, I would take __always_inline and put it in mm/internal.h in v2. > > I'm not confident in this suggestion as the rest. > > Please rename the function when you move it. > > inline is a suggestion, __always_inline is mandatory. > I think __always_inline is better if we're demanding. > > > > > > > vma_range_set(), vma_set_range(), or just vma_range() might be a better > > > > > > name? My thinking is that some of these are actually modifying the vma > > > > > > and not just initializing it, right? > > > > > I'd vote for vma_set_range(). > > ^ This part, use vma_set_range() please. > > Okay. > > > By the way, I sent another patch with ("mm/mmap: simplify vma_merge()") > subject a few days ago. > > Please comment if you would like. Oh, okay. Can you please Cc people on these? I almost missed this one and I know several other people will be interested in the vma_merge() patch changes.
diff --git a/include/linux/mm.h b/include/linux/mm.h index f5a97dec5169..abb4534be3cc 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3516,6 +3516,15 @@ static inline bool range_in_vma(struct vm_area_struct *vma, return (vma && vma->vm_start <= start && end <= vma->vm_end); } +static inline void vma_range_init(struct vm_area_struct *vma, + unsigned long start, unsigned long end, + pgoff_t pgoff) +{ + vma->vm_start = start; + vma->vm_end = end; + vma->vm_pgoff = pgoff; +} + #ifdef CONFIG_MMU pgprot_t vm_get_page_prot(unsigned long vm_flags); void vma_set_page_prot(struct vm_area_struct *vma); diff --git a/mm/mmap.c b/mm/mmap.c index 06f1f3e88598..0d3f4612d001 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -663,9 +663,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma, vma_prepare(&vp); vma_adjust_trans_huge(vma, start, end, 0); - vma->vm_start = start; - vma->vm_end = end; - vma->vm_pgoff = pgoff; + vma_range_init(vma, start, end, pgoff); vma_iter_store(vmi, vma); vma_complete(&vp, vmi, vma->vm_mm); @@ -708,9 +706,7 @@ int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma, vma_adjust_trans_huge(vma, start, end, 0); vma_iter_clear(vmi); - vma->vm_start = start; - vma->vm_end = end; - vma->vm_pgoff = pgoff; + vma_range_init(vma, start, end, pgoff); vma_complete(&vp, vmi, vma->vm_mm); return 0; } @@ -1015,10 +1011,7 @@ static struct vm_area_struct vma_prepare(&vp); vma_adjust_trans_huge(vma, vma_start, vma_end, adj_start); - - vma->vm_start = vma_start; - vma->vm_end = vma_end; - vma->vm_pgoff = vma_pgoff; + vma_range_init(vma, vma_start, vma_end, vma_pgoff); if (vma_expanded) vma_iter_store(vmi, vma); @@ -2811,11 +2804,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, } vma_iter_config(&vmi, addr, end); - vma->vm_start = addr; - vma->vm_end = end; + vma_range_init(vma, addr, end, pgoff); vm_flags_init(vma, vm_flags); vma->vm_page_prot = vm_get_page_prot(vm_flags); - vma->vm_pgoff = pgoff; if (file) { vma->vm_file = get_file(file); @@ -3165,9 +3156,7 @@ static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma, goto unacct_fail; vma_set_anonymous(vma); - vma->vm_start = addr; - vma->vm_end = addr + len; - vma->vm_pgoff = addr >> PAGE_SHIFT; + vma_range_init(vma, addr, addr + len, addr >> PAGE_SHIFT); vm_flags_init(vma, flags); vma->vm_page_prot = vm_get_page_prot(flags); vma_start_write(vma); @@ -3404,9 +3393,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap, new_vma = vm_area_dup(vma); if (!new_vma) goto out; - new_vma->vm_start = addr; - new_vma->vm_end = addr + len; - new_vma->vm_pgoff = pgoff; + vma_range_init(new_vma, addr, addr + len, pgoff); if (vma_dup_policy(vma, new_vma)) goto out_free_vma; if (anon_vma_clone(new_vma, vma)) @@ -3574,9 +3561,7 @@ static struct vm_area_struct *__install_special_mapping( if (unlikely(vma == NULL)) return ERR_PTR(-ENOMEM); - vma->vm_start = addr; - vma->vm_end = addr + len; - + vma_range_init(vma, addr, addr + len, 0); vm_flags_init(vma, (vm_flags | mm->def_flags | VM_DONTEXPAND | VM_SOFTDIRTY) & ~VM_LOCKED_MASK); vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);