| Message ID | 20241118021823.17386-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vma: check retry_merge only for new vma case | expand |
* Wei Yang <richard.weiyang@gmail.com> [241117 21:21]: > Current code logic looks like this: > > __mmap_region() > vma = vma_merge_new_range(&vmg) > if (!vma) > __mmap_new_vma(&map, &vma) > __mmap_new_file_vma(map, vma) > map->retry_merge = xxx --- (1) > if (map.retry_merge) > vma_merge_existing_range(vmg, &map, vma) Please don't quote code in your commit log. We can see the code in the diff section. > > Location (1) is the only place where map.retry_merge is set, this means > it is not necessary to check it if already merged with adjacent vma. > > Let's move the check and following operation into new vma case. This makes sense, but this is a complex block of code. I'm all for optimisations, but there is already a bug in the code that you relocated in your patch, and the backport of these changes isn't even complete. Maybe we can give the existing code some time to soak before optimising? > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Liam R. Howlett <Liam.Howlett@Oracle.com> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > CC: Vlastimil Babka <vbabka@suse.cz> > CC: Jann Horn <jannh@google.com> > --- > mm/vma.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index 8a454a7bbc80..80b1bd404f23 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -2456,14 +2456,14 @@ unsigned long __mmap_region(struct file *file, unsigned long addr, > error = __mmap_new_vma(&map, &vma); > if (error) > goto unacct_error; > - } > > - /* If flags changed, we might be able to merge, so try again. */ > - if (map.retry_merge) { > - VMG_MMAP_STATE(vmg, &map, vma); > + /* If flags changed, we might be able to merge, so try again. */ > + if (map.retry_merge) { > + VMG_MMAP_STATE(vmg, &map, vma); > > - vma_iter_config(map.vmi, map.addr, map.end); > - vma_merge_existing_range(&vmg); > + vma_iter_config(map.vmi, map.addr, map.end); > + vma_merge_existing_range(&vmg); > + } > } > > __mmap_complete(&map, vma); > -- > 2.34.1 >
diff --git a/mm/vma.c b/mm/vma.c index 8a454a7bbc80..80b1bd404f23 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -2456,14 +2456,14 @@ unsigned long __mmap_region(struct file *file, unsigned long addr, error = __mmap_new_vma(&map, &vma); if (error) goto unacct_error; - } - /* If flags changed, we might be able to merge, so try again. */ - if (map.retry_merge) { - VMG_MMAP_STATE(vmg, &map, vma); + /* If flags changed, we might be able to merge, so try again. */ + if (map.retry_merge) { + VMG_MMAP_STATE(vmg, &map, vma); - vma_iter_config(map.vmi, map.addr, map.end); - vma_merge_existing_range(&vmg); + vma_iter_config(map.vmi, map.addr, map.end); + vma_merge_existing_range(&vmg); + } } __mmap_complete(&map, vma);
Current code logic looks like this: __mmap_region() vma = vma_merge_new_range(&vmg) if (!vma) __mmap_new_vma(&map, &vma) __mmap_new_file_vma(map, vma) map->retry_merge = xxx --- (1) if (map.retry_merge) vma_merge_existing_range(vmg, &map, vma) Location (1) is the only place where map.retry_merge is set, this means it is not necessary to check it if already merged with adjacent vma. Let's move the check and following operation into new vma case. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Liam R. Howlett <Liam.Howlett@Oracle.com> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> CC: Vlastimil Babka <vbabka@suse.cz> CC: Jann Horn <jannh@google.com> --- mm/vma.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)