| Message ID | 20220831052734.3423079-1-song@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vmalloc: Extend find_vmap_lowest_match_check with extra arguments | expand |
On 08/30/22 at 10:27pm, Song Liu wrote: > find_vmap_lowest_match() is now able to handle different roots. Make > similar changes to find_vmap_lowest_match_check() and > find_vmap_lowest_linear_match() to handle different trees. Good catch. Guess usually nobody eables DEBUG_AUGMENT_LOWEST_MATCH_CHECK to trigger compilation of the code. Reviewed-by: Baoquan He <bhe@redhat.com> > > Fixes: f9863be49312 ("mm/vmalloc: extend __alloc_vmap_area() with extra arguments") > Cc: Uladzislau Rezki (Sony) <urezki@gmail.com> > Cc: Baoquan He <bhe@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Song Liu <song@kernel.org> > --- > mm/vmalloc.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index dd6cdb201195..088b421601c4 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -1300,12 +1300,12 @@ find_vmap_lowest_match(struct rb_root *root, unsigned long size, > #include <linux/random.h> > > static struct vmap_area * > -find_vmap_lowest_linear_match(unsigned long size, > +find_vmap_lowest_linear_match(struct list_head *head, unsigned long size, > unsigned long align, unsigned long vstart) > { > struct vmap_area *va; > > - list_for_each_entry(va, &free_vmap_area_list, list) { > + list_for_each_entry(va, head, list) { > if (!is_within_this_va(va, size, align, vstart)) > continue; > > @@ -1316,7 +1316,8 @@ find_vmap_lowest_linear_match(unsigned long size, > } > > static void > -find_vmap_lowest_match_check(unsigned long size, unsigned long align) > +find_vmap_lowest_match_check(struct rb_root *root, struct list_head *head, > + unsigned long size, unsigned long align) > { > struct vmap_area *va_1, *va_2; > unsigned long vstart; > @@ -1325,8 +1326,8 @@ find_vmap_lowest_match_check(unsigned long size, unsigned long align) > get_random_bytes(&rnd, sizeof(rnd)); > vstart = VMALLOC_START + rnd; > > - va_1 = find_vmap_lowest_match(size, align, vstart, false); > - va_2 = find_vmap_lowest_linear_match(size, align, vstart); > + va_1 = find_vmap_lowest_match(root, size, align, vstart, false); > + va_2 = find_vmap_lowest_linear_match(head, size, align, vstart); > > if (va_1 != va_2) > pr_emerg("not lowest: t: 0x%p, l: 0x%p, v: 0x%lx\n", > @@ -1513,7 +1514,7 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, > return vend; > > #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK > - find_vmap_lowest_match_check(size, align); > + find_vmap_lowest_match_check(root, head, size, align); > #endif > > return nva_start_addr; > -- > 2.30.2 >
> On 08/30/22 at 10:27pm, Song Liu wrote: > > find_vmap_lowest_match() is now able to handle different roots. Make > > similar changes to find_vmap_lowest_match_check() and > > find_vmap_lowest_linear_match() to handle different trees. > > Good catch. Guess usually nobody eables DEBUG_AUGMENT_LOWEST_MATCH_CHECK > to trigger compilation of the code. > > Reviewed-by: Baoquan He <bhe@redhat.com> > I do it. Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com> -- Uladzislau Rezki
On Tue, 30 Aug 2022 22:27:34 -0700 Song Liu <song@kernel.org> wrote: > find_vmap_lowest_match() is now able to handle different roots. Make > similar changes to find_vmap_lowest_match_check() and > find_vmap_lowest_linear_match() to handle different trees. What are the runtime effects of this change?
On Wed, Aug 31, 2022 at 4:01 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 30 Aug 2022 22:27:34 -0700 Song Liu <song@kernel.org> wrote: > > > find_vmap_lowest_match() is now able to handle different roots. Make > > similar changes to find_vmap_lowest_match_check() and > > find_vmap_lowest_linear_match() to handle different trees. > > What are the runtime effects of this change? The code is gated by DEBUG_AUGMENT_LOWEST_MATCH_CHECK. It is only compiled when the developer enables it explicitly. Therefore, there isn't any runtime effect. Thanks, Song
This is only for debug purpose. Even without this patch, the debug path would work correctly. The difference is just only in whether roots are hardcoded or passed over function paramter. -- Uladzislau Rezki On Thu, Sep 1, 2022 at 2:47 AM Song Liu <song@kernel.org> wrote: > > On Wed, Aug 31, 2022 at 4:01 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 30 Aug 2022 22:27:34 -0700 Song Liu <song@kernel.org> wrote: > > > > > find_vmap_lowest_match() is now able to handle different roots. Make > > > similar changes to find_vmap_lowest_match_check() and > > > find_vmap_lowest_linear_match() to handle different trees. > > > > What are the runtime effects of this change? > > The code is gated by DEBUG_AUGMENT_LOWEST_MATCH_CHECK. It > is only compiled when the developer enables it explicitly. Therefore, > there isn't > any runtime effect. > > Thanks, > Song
On 09/01/22 at 02:16pm, Uladzislau Rezki wrote: > This is only for debug purpose. > > Even without this patch, the debug path would work correctly. The > difference is just only in > whether roots are hardcoded or passed over function paramter. Calling find_vmap_lowest_match() inside find_vmap_lowest_match_check() will fail compilation because the function interface has been changed. > > On Thu, Sep 1, 2022 at 2:47 AM Song Liu <song@kernel.org> wrote: > > > > On Wed, Aug 31, 2022 at 4:01 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > On Tue, 30 Aug 2022 22:27:34 -0700 Song Liu <song@kernel.org> wrote: > > > > > > > find_vmap_lowest_match() is now able to handle different roots. Make > > > > similar changes to find_vmap_lowest_match_check() and > > > > find_vmap_lowest_linear_match() to handle different trees. > > > > > > What are the runtime effects of this change? > > > > The code is gated by DEBUG_AUGMENT_LOWEST_MATCH_CHECK. It > > is only compiled when the developer enables it explicitly. Therefore, > > there isn't > > any runtime effect. > > > > Thanks, > > Song > > > > -- > Uladzislau Rezki >
> > Even without this patch, the debug path would work correctly. The > > difference is just only in > > whether roots are hardcoded or passed over function paramter. > > Calling find_vmap_lowest_match() inside find_vmap_lowest_match_check() > will fail compilation because the function interface has been changed. > Ah. That makes sense, though the commit message has to reflect it. So it is only about compilation error if debug is ON.
On 09/02/22 at 06:45pm, Uladzislau Rezki wrote: > > > Even without this patch, the debug path would work correctly. The > > > difference is just only in > > > whether roots are hardcoded or passed over function paramter. > > > > Calling find_vmap_lowest_match() inside find_vmap_lowest_match_check() > > will fail compilation because the function interface has been changed. > > > Ah. That makes sense, though the commit message has to reflect it. > So it is only about compilation error if debug is ON. Indeed, the current patch log sounds like an improvement or normal change. In fact it's a code fix.
> On 09/02/22 at 06:45pm, Uladzislau Rezki wrote: > > > > Even without this patch, the debug path would work correctly. The > > > > difference is just only in > > > > whether roots are hardcoded or passed over function paramter. > > > > > > Calling find_vmap_lowest_match() inside find_vmap_lowest_match_check() > > > will fail compilation because the function interface has been changed. > > > > > Ah. That makes sense, though the commit message has to reflect it. > > So it is only about compilation error if debug is ON. > > Indeed, the current patch log sounds like an improvement or normal change. > In fact it's a code fix. > Then i think it is worth to mention about this in the commit message. At least i have missed the main point of this change looking at the commit message. Song Liu, Could you please upload a v2 of it stating exactly what it fixes? <snip> urezki@pc638:~/data/raid0/coding/linux-next.git$ git diff diff --git a/mm/vmalloc.c b/mm/vmalloc.c index e68c0081e861..7552f1f8350e 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -713,7 +713,7 @@ EXPORT_SYMBOL(vmalloc_to_pfn); /*** Global kva allocator ***/ #define DEBUG_AUGMENT_PROPAGATE_CHECK 0 -#define DEBUG_AUGMENT_LOWEST_MATCH_CHECK 0 +#define DEBUG_AUGMENT_LOWEST_MATCH_CHECK 1 static DEFINE_SPINLOCK(vmap_area_lock); urezki@pc638:~/data/raid0/coding/linux-next.git$ make -j64 bzImage DESCEND objtool CALL scripts/checksyscalls.sh CHK include/generated/compile.h CC mm/vmalloc.o mm/vmalloc.c: In function ‘find_vmap_lowest_match_check’: mm/vmalloc.c:1328:32: warning: passing argument 1 of ‘find_vmap_lowest_match’ makes pointer from integer without a cast [-Wint-conversion] 1328 | va_1 = find_vmap_lowest_match(size, align, vstart, false); | ^~~~ | | | long unsigned int mm/vmalloc.c:1236:40: note: expected ‘struct rb_root *’ but argument is of type ‘long unsigned int’ 1236 | find_vmap_lowest_match(struct rb_root *root, unsigned long size, | ~~~~~~~~~~~~~~~~^~~~ mm/vmalloc.c:1328:9: error: too few arguments to function ‘find_vmap_lowest_match’ 1328 | va_1 = find_vmap_lowest_match(size, align, vstart, false); | ^~~~~~~~~~~~~~~~~~~~~~ mm/vmalloc.c:1236:1: note: declared here 1236 | find_vmap_lowest_match(struct rb_root *root, unsigned long size, | ^~~~~~~~~~~~~~~~~~~~~~ make[1]: *** [scripts/Makefile.build:250: mm/vmalloc.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:2003: mm] Error 2 make: *** Waiting for unfinished jobs.... urezki@pc638:~/data/raid0/coding/linux-next.git$ <snip> Thank you in advance! -- Uladzislau Rezki
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index dd6cdb201195..088b421601c4 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1300,12 +1300,12 @@ find_vmap_lowest_match(struct rb_root *root, unsigned long size, #include <linux/random.h> static struct vmap_area * -find_vmap_lowest_linear_match(unsigned long size, +find_vmap_lowest_linear_match(struct list_head *head, unsigned long size, unsigned long align, unsigned long vstart) { struct vmap_area *va; - list_for_each_entry(va, &free_vmap_area_list, list) { + list_for_each_entry(va, head, list) { if (!is_within_this_va(va, size, align, vstart)) continue; @@ -1316,7 +1316,8 @@ find_vmap_lowest_linear_match(unsigned long size, } static void -find_vmap_lowest_match_check(unsigned long size, unsigned long align) +find_vmap_lowest_match_check(struct rb_root *root, struct list_head *head, + unsigned long size, unsigned long align) { struct vmap_area *va_1, *va_2; unsigned long vstart; @@ -1325,8 +1326,8 @@ find_vmap_lowest_match_check(unsigned long size, unsigned long align) get_random_bytes(&rnd, sizeof(rnd)); vstart = VMALLOC_START + rnd; - va_1 = find_vmap_lowest_match(size, align, vstart, false); - va_2 = find_vmap_lowest_linear_match(size, align, vstart); + va_1 = find_vmap_lowest_match(root, size, align, vstart, false); + va_2 = find_vmap_lowest_linear_match(head, size, align, vstart); if (va_1 != va_2) pr_emerg("not lowest: t: 0x%p, l: 0x%p, v: 0x%lx\n", @@ -1513,7 +1514,7 @@ __alloc_vmap_area(struct rb_root *root, struct list_head *head, return vend; #if DEBUG_AUGMENT_LOWEST_MATCH_CHECK - find_vmap_lowest_match_check(size, align); + find_vmap_lowest_match_check(root, head, size, align); #endif return nva_start_addr;
find_vmap_lowest_match() is now able to handle different roots. Make similar changes to find_vmap_lowest_match_check() and find_vmap_lowest_linear_match() to handle different trees. Fixes: f9863be49312 ("mm/vmalloc: extend __alloc_vmap_area() with extra arguments") Cc: Uladzislau Rezki (Sony) <urezki@gmail.com> Cc: Baoquan He <bhe@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Song Liu <song@kernel.org> --- mm/vmalloc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)