On Tue, 3 Sep 2024 17:58:15 -0700 SeongJae Park <s...@kernel.org> wrote:
> On Tue, 3 Sep 2024 20:48:53 -0400 "Liam R. Howlett" <liam.howl...@oracle.com> > wrote: > > > * SeongJae Park <s...@kernel.org> [240903 20:45]: > > > damon_test_three_regions_in_vmas() initializes a maple tree with > > > MM_MT_FLAGS. The flags contains MT_FLAGS_LOCK_EXTERN, which means > > > mt_lock of the maple tree will not be used. And therefore the maple > > > tree initialization code skips initialization of the mt_lock. However, > > > __link_vmas(), which adds vmas for test to the maple tree, uses the > > > mt_lock. In other words, the uninitialized spinlock is used. The > > > problem becomes celar when spinlock debugging is turned on, since it > > > reports spinlock bad magic bug. Fix the issue by not using the mt_lock > > > as promised. > > > > You can't do this, lockdep will tell you this is wrong. > > Hmm, but lockdep was silence on my setup? > > > We need a lock and to use the lock for writes. > > This code is executed by a single-thread test code. Do we still need the > lock? > > > > > I'd suggest using different flags so the spinlock is used. > > The reporter mentioned simply dropping MT_FLAGS_LOCK_EXTERN from the flags > causes suspicious RCU usage message. May I ask if you have a suggestion of > better flags? I was actually thinking replacing the mt_init_flags() with mt_init(), which same to mt_init_flags() with zero flag, like below. ``` --- a/mm/damon/tests/vaddr-kunit.h +++ b/mm/damon/tests/vaddr-kunit.h @@ -77,7 +77,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test) (struct vm_area_struct) {.vm_start = 307, .vm_end = 330}, }; - mt_init_flags(&mm.mm_mt, MM_MT_FLAGS); + mt_init(&mm.mm_mt); if (__link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas))) kunit_skip(test, "Failed to create VMA tree"); ``` And just confirmed it also convinces the reproducer. But because I'm obviously not familiar with maple tree, would like to hear some comments from Liam or others first. FYI, I ended up writing v1 to simply remove lock usage based on my humble understanding of the documetnation. The maple tree uses a spinlock by default, but external locks can be used for tree updates as well. To use an external lock, the tree must be initialized with the ``MT_FLAGS_LOCK_EXTERN flag``, this is usually done with the MTREE_INIT_EXT() #define, which takes an external lock as an argument. (from Documentation/core-api/maple_tree.rst) I was thinking the fact that the test code is executed in single thread is same to use of external lock. I will be happy to learn if I missed something. Thanks, SJ > > > Thanks, > SJ > > > > > > > > > Reported-by: Guenter Roeck <li...@roeck-us.net> > > > Closes: > > > https://lore.kernel.org/1453b2b2-6119-4082-ad9e-f3c5239bf...@roeck-us.net > > > Fixes: d0cf3dd47f0d ("damon: convert __damon_va_three_regions to use the > > > VMA iterator") > > > Signed-off-by: SeongJae Park <s...@kernel.org> > > > --- > > > mm/damon/tests/vaddr-kunit.h | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h > > > index 83626483f82b..c6c7e0e0ab07 100644 > > > --- a/mm/damon/tests/vaddr-kunit.h > > > +++ b/mm/damon/tests/vaddr-kunit.h > > > @@ -17,23 +17,19 @@ > > > static int __link_vmas(struct maple_tree *mt, struct vm_area_struct > > > *vmas, > > > ssize_t nr_vmas) > > > { > > > - int i, ret = -ENOMEM; > > > + int i; > > > MA_STATE(mas, mt, 0, 0); > > > > > > if (!nr_vmas) > > > return 0; > > > > > > - mas_lock(&mas); > > > for (i = 0; i < nr_vmas; i++) { > > > mas_set_range(&mas, vmas[i].vm_start, vmas[i].vm_end - 1); > > > if (mas_store_gfp(&mas, &vmas[i], GFP_KERNEL)) > > > - goto failed; > > > + return -ENOMEM; > > > } > > > > > > - ret = 0; > > > -failed: > > > - mas_unlock(&mas); > > > - return ret; > > > + return 0; > > > } > > > > > > /* > > > -- > > > 2.39.2 > > >