The same operation already exists in the function __get_vm_area_node,
so delete the duplicate operation to simplify the code.
Signed-off-by: Liu Ye <liuye@kylinos.cn>
---
mm/vmalloc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index dc658d4af181..20d9b9de84b1 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
shift = arch_vmap_pte_supported_shift(size);
align = max(real_align, 1UL << shift);
- size = ALIGN(real_size, 1UL << shift);
}
again:
--
2.25.1On Mon, Mar 03, 2025 at 05:44:07PM +0800, Liu Ye wrote:
> The same operation already exists in the function __get_vm_area_node,
> so delete the duplicate operation to simplify the code.
>
> Signed-off-by: Liu Ye <liuye@kylinos.cn>
> ---
> mm/vmalloc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index dc658d4af181..20d9b9de84b1 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> shift = arch_vmap_pte_supported_shift(size);
>
> align = max(real_align, 1UL << shift);
> - size = ALIGN(real_size, 1UL << shift);
> }
>
> again:
> --
> 2.25.1
>
There is a mess with:
unsigned long real_size = size;
unsigned long real_align = align;
"real_size" and "real_align". Those are useless. What is about:
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 5c88d0e90c20..a381ffee1595 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3771,8 +3771,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
struct vm_struct *area;
void *ret;
kasan_vmalloc_flags_t kasan_flags = KASAN_VMALLOC_NONE;
- unsigned long real_size = size;
- unsigned long real_align = align;
unsigned int shift = PAGE_SHIFT;
if (WARN_ON_ONCE(!size))
@@ -3781,7 +3779,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
if ((size >> PAGE_SHIFT) > totalram_pages()) {
warn_alloc(gfp_mask, NULL,
"vmalloc error: size %lu, exceeds total pages",
- real_size);
+ size);
return NULL;
}
@@ -3798,19 +3796,18 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
else
shift = arch_vmap_pte_supported_shift(size);
- align = max(real_align, 1UL << shift);
- size = ALIGN(real_size, 1UL << shift);
+ align = max(align, 1UL << shift);
}
again:
- area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
+ area = __get_vm_area_node(size, align, shift, VM_ALLOC |
VM_UNINITIALIZED | vm_flags, start, end, node,
gfp_mask, caller);
if (!area) {
bool nofail = gfp_mask & __GFP_NOFAIL;
warn_alloc(gfp_mask, NULL,
"vmalloc error: size %lu, vm_struct allocation failed%s",
- real_size, (nofail) ? ". Retrying." : "");
+ size, (nofail) ? ". Retrying." : "");
if (nofail) {
schedule_timeout_uninterruptible(1);
goto again;
@@ -3860,7 +3857,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
(gfp_mask & __GFP_SKIP_ZERO))
kasan_flags |= KASAN_VMALLOC_INIT;
/* KASAN_VMALLOC_PROT_NORMAL already set if required. */
- area->addr = kasan_unpoison_vmalloc(area->addr, real_size, kasan_flags);
+ area->addr = kasan_unpoison_vmalloc(area->addr, size, kasan_flags);
/*
* In this function, newly allocated vm_struct has VM_UNINITIALIZED
@@ -3878,8 +3875,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
fail:
if (shift > PAGE_SHIFT) {
shift = PAGE_SHIFT;
- align = real_align;
- size = real_size;
goto again;
}
?
--
Uladzislau Rezki
在 2025/3/4 02:30, Uladzislau Rezki 写道:
> On Mon, Mar 03, 2025 at 05:44:07PM +0800, Liu Ye wrote:
>> The same operation already exists in the function __get_vm_area_node,
>> so delete the duplicate operation to simplify the code.
>>
>> Signed-off-by: Liu Ye <liuye@kylinos.cn>
>> ---
>> mm/vmalloc.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index dc658d4af181..20d9b9de84b1 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
>> shift = arch_vmap_pte_supported_shift(size);
>>
>> align = max(real_align, 1UL << shift);
>> - size = ALIGN(real_size, 1UL << shift);
>> }
>>
>> again:
>> --
>> 2.25.1
>>
> There is a mess with:
>
> unsigned long real_size = size;
> unsigned long real_align = align;
>
> "real_size" and "real_align". Those are useless. What is about:
Sorry, the order of the patches may be misleading.
The correct order is as follows：
PATCH1. mm/vmalloc: Size should be used instead of real_size "
PATCH2. mm/vmalloc: Remove unnecessary size ALIGN in __vmalloc_node_range_noprof
PATCH3. mm/vmalloc: Remove the real_size variable to simplify the code "
PATCH4. mm/vmalloc: Rename the variable real_align to original_align to prevent misunderstanding
If PATCH1 is the correct fix, then consider PATCH2, PATCH3, and PATCH4.
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 5c88d0e90c20..a381ffee1595 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3771,8 +3771,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> struct vm_struct *area;
> void *ret;
> kasan_vmalloc_flags_t kasan_flags = KASAN_VMALLOC_NONE;
> - unsigned long real_size = size;
> - unsigned long real_align = align;
> unsigned int shift = PAGE_SHIFT;
>
> if (WARN_ON_ONCE(!size))
> @@ -3781,7 +3779,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> if ((size >> PAGE_SHIFT) > totalram_pages()) {
> warn_alloc(gfp_mask, NULL,
> "vmalloc error: size %lu, exceeds total pages",
> - real_size);
> + size);
> return NULL;
> }
>
> @@ -3798,19 +3796,18 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> else
> shift = arch_vmap_pte_supported_shift(size);
>
> - align = max(real_align, 1UL << shift);
> - size = ALIGN(real_size, 1UL << shift);
> + align = max(align, 1UL << shift);
> }
>
> again:
> - area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
> + area = __get_vm_area_node(size, align, shift, VM_ALLOC |
> VM_UNINITIALIZED | vm_flags, start, end, node,
> gfp_mask, caller);
> if (!area) {
> bool nofail = gfp_mask & __GFP_NOFAIL;
> warn_alloc(gfp_mask, NULL,
> "vmalloc error: size %lu, vm_struct allocation failed%s",
> - real_size, (nofail) ? ". Retrying." : "");
> + size, (nofail) ? ". Retrying." : "");
> if (nofail) {
> schedule_timeout_uninterruptible(1);
> goto again;
> @@ -3860,7 +3857,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> (gfp_mask & __GFP_SKIP_ZERO))
> kasan_flags |= KASAN_VMALLOC_INIT;
> /* KASAN_VMALLOC_PROT_NORMAL already set if required. */
> - area->addr = kasan_unpoison_vmalloc(area->addr, real_size, kasan_flags);
> + area->addr = kasan_unpoison_vmalloc(area->addr, size, kasan_flags);
>
> /*
> * In this function, newly allocated vm_struct has VM_UNINITIALIZED
> @@ -3878,8 +3875,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> fail:
> if (shift > PAGE_SHIFT) {
> shift = PAGE_SHIFT;
> - align = real_align;
> - size = real_size;
> goto again;
> }
>
> ?
>
> --
> Uladzislau Rezki
On 03/05/25 at 09:46am, liuye wrote:
>
> 在 2025/3/4 02:30, Uladzislau Rezki 写道:
> > On Mon, Mar 03, 2025 at 05:44:07PM +0800, Liu Ye wrote:
> >> The same operation already exists in the function __get_vm_area_node,
> >> so delete the duplicate operation to simplify the code.
> >>
> >> Signed-off-by: Liu Ye <liuye@kylinos.cn>
> >> ---
> >> mm/vmalloc.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >> index dc658d4af181..20d9b9de84b1 100644
> >> --- a/mm/vmalloc.c
> >> +++ b/mm/vmalloc.c
> >> @@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> >> shift = arch_vmap_pte_supported_shift(size);
> >>
> >> align = max(real_align, 1UL << shift);
> >> - size = ALIGN(real_size, 1UL << shift);
> >> }
> >>
> >> again:
> >> --
> >> 2.25.1
> >>
> > There is a mess with:
> >
> > unsigned long real_size = size;
> > unsigned long real_align = align;
> >
> > "real_size" and "real_align". Those are useless. What is about:
>
> Sorry, the order of the patches may be misleading.
>
> The correct order is as follows：
>
> PATCH1. mm/vmalloc: Size should be used instead of real_size "
> PATCH2. mm/vmalloc: Remove unnecessary size ALIGN in __vmalloc_node_range_noprof
> PATCH3. mm/vmalloc: Remove the real_size variable to simplify the code "
> PATCH4. mm/vmalloc: Rename the variable real_align to original_align to prevent misunderstanding
>
> If PATCH1 is the correct fix, then consider PATCH2, PATCH3, and PATCH4.
Well, seems the patch split is done too subtly. It's only about the
size/align inside one function, maybe one patch is enough in this case.
My personal opinion.
>
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 5c88d0e90c20..a381ffee1595 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -3771,8 +3771,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> > struct vm_struct *area;
> > void *ret;
> > kasan_vmalloc_flags_t kasan_flags = KASAN_VMALLOC_NONE;
> > - unsigned long real_size = size;
> > - unsigned long real_align = align;
> > unsigned int shift = PAGE_SHIFT;
> >
> > if (WARN_ON_ONCE(!size))
> > @@ -3781,7 +3779,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> > if ((size >> PAGE_SHIFT) > totalram_pages()) {
> > warn_alloc(gfp_mask, NULL,
> > "vmalloc error: size %lu, exceeds total pages",
> > - real_size);
> > + size);
> > return NULL;
> > }
> >
> > @@ -3798,19 +3796,18 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> > else
> > shift = arch_vmap_pte_supported_shift(size);
> >
> > - align = max(real_align, 1UL << shift);
> > - size = ALIGN(real_size, 1UL << shift);
> > + align = max(align, 1UL << shift);
> > }
> >
> > again:
> > - area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
> > + area = __get_vm_area_node(size, align, shift, VM_ALLOC |
> > VM_UNINITIALIZED | vm_flags, start, end, node,
> > gfp_mask, caller);
> > if (!area) {
> > bool nofail = gfp_mask & __GFP_NOFAIL;
> > warn_alloc(gfp_mask, NULL,
> > "vmalloc error: size %lu, vm_struct allocation failed%s",
> > - real_size, (nofail) ? ". Retrying." : "");
> > + size, (nofail) ? ". Retrying." : "");
> > if (nofail) {
> > schedule_timeout_uninterruptible(1);
> > goto again;
> > @@ -3860,7 +3857,7 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> > (gfp_mask & __GFP_SKIP_ZERO))
> > kasan_flags |= KASAN_VMALLOC_INIT;
> > /* KASAN_VMALLOC_PROT_NORMAL already set if required. */
> > - area->addr = kasan_unpoison_vmalloc(area->addr, real_size, kasan_flags);
> > + area->addr = kasan_unpoison_vmalloc(area->addr, size, kasan_flags);
> >
> > /*
> > * In this function, newly allocated vm_struct has VM_UNINITIALIZED
> > @@ -3878,8 +3875,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align,
> > fail:
> > if (shift > PAGE_SHIFT) {
> > shift = PAGE_SHIFT;
> > - align = real_align;
> > - size = real_size;
> > goto again;
> > }
> >
> > ?
> >
> > --
> > Uladzislau Rezki
>
On Wed, Mar 05, 2025 at 06:02:19PM +0800, Baoquan He wrote: > On 03/05/25 at 09:46am, liuye wrote: > > > > 在 2025/3/4 02:30, Uladzislau Rezki 写道: > > > On Mon, Mar 03, 2025 at 05:44:07PM +0800, Liu Ye wrote: > > >> The same operation already exists in the function __get_vm_area_node, > > >> so delete the duplicate operation to simplify the code. > > >> > > >> Signed-off-by: Liu Ye <liuye@kylinos.cn> > > >> --- > > >> mm/vmalloc.c | 1 - > > >> 1 file changed, 1 deletion(-) > > >> > > >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > >> index dc658d4af181..20d9b9de84b1 100644 > > >> --- a/mm/vmalloc.c > > >> +++ b/mm/vmalloc.c > > >> @@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align, > > >> shift = arch_vmap_pte_supported_shift(size); > > >> > > >> align = max(real_align, 1UL << shift); > > >> - size = ALIGN(real_size, 1UL << shift); > > >> } > > >> > > >> again: > > >> -- > > >> 2.25.1 > > >> > > > There is a mess with: > > > > > > unsigned long real_size = size; > > > unsigned long real_align = align; > > > > > > "real_size" and "real_align". Those are useless. What is about: > > > > Sorry, the order of the patches may be misleading. > > > > The correct order is as follows： > > > > PATCH1. mm/vmalloc: Size should be used instead of real_size " > > PATCH2. mm/vmalloc: Remove unnecessary size ALIGN in __vmalloc_node_range_noprof > > PATCH3. mm/vmalloc: Remove the real_size variable to simplify the code " > > PATCH4. mm/vmalloc: Rename the variable real_align to original_align to prevent misunderstanding > > > > If PATCH1 is the correct fix, then consider PATCH2, PATCH3, and PATCH4. > > Well, seems the patch split is done too subtly. It's only about the > size/align inside one function, maybe one patch is enough in this case. > My personal opinion. > I agree. One patch would be enough. -- Uladzislau Rezki
在 2025/3/5 18:06, Uladzislau Rezki 写道: > On Wed, Mar 05, 2025 at 06:02:19PM +0800, Baoquan He wrote: >> On 03/05/25 at 09:46am, liuye wrote: >>> >>> 在 2025/3/4 02:30, Uladzislau Rezki 写道: >>>> On Mon, Mar 03, 2025 at 05:44:07PM +0800, Liu Ye wrote: >>>>> The same operation already exists in the function __get_vm_area_node, >>>>> so delete the duplicate operation to simplify the code. >>>>> >>>>> Signed-off-by: Liu Ye <liuye@kylinos.cn> >>>>> --- >>>>> mm/vmalloc.c | 1 - >>>>> 1 file changed, 1 deletion(-) >>>>> >>>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >>>>> index dc658d4af181..20d9b9de84b1 100644 >>>>> --- a/mm/vmalloc.c >>>>> +++ b/mm/vmalloc.c >>>>> @@ -3798,7 +3798,6 @@ void *__vmalloc_node_range_noprof(unsigned long size, unsigned long align, >>>>> shift = arch_vmap_pte_supported_shift(size); >>>>> >>>>> align = max(real_align, 1UL << shift); >>>>> - size = ALIGN(real_size, 1UL << shift); >>>>> } >>>>> >>>>> again: >>>>> -- >>>>> 2.25.1 >>>>> >>>> There is a mess with: >>>> >>>> unsigned long real_size = size; >>>> unsigned long real_align = align; >>>> >>>> "real_size" and "real_align". Those are useless. What is about: >>> >>> Sorry, the order of the patches may be misleading. >>> >>> The correct order is as follows： >>> >>> PATCH1. mm/vmalloc: Size should be used instead of real_size " >>> PATCH2. mm/vmalloc: Remove unnecessary size ALIGN in __vmalloc_node_range_noprof >>> PATCH3. mm/vmalloc: Remove the real_size variable to simplify the code " >>> PATCH4. mm/vmalloc: Rename the variable real_align to original_align to prevent misunderstanding >>> >>> If PATCH1 is the correct fix, then consider PATCH2, PATCH3, and PATCH4. >> >> Well, seems the patch split is done too subtly. It's only about the >> size/align inside one function, maybe one patch is enough in this case. >> My personal opinion. >> > I agree. One patch would be enough. > At first, I only wanted to use one patch, but to better understand, I split it up. I can integrate it later and resend a new patch, and add a detailed description in the commit message. Thanks， Liu Ye > -- > Uladzislau Rezki
© 2016 - 2026 Red Hat, Inc.