From: "Liam R. Howlett" <[email protected]>
Prepare for the removal of the vma_mas_store() function by open coding
the maple tree store in this test code. Set the range of the maple
state and call the store function directly.
Cc: SeongJae Park <[email protected]>
Cc: [email protected]
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Liam R. Howlett <[email protected]>
---
mm/damon/vaddr-test.h | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
index bce37c487540..6098933d3272 100644
--- a/mm/damon/vaddr-test.h
+++ b/mm/damon/vaddr-test.h
@@ -14,19 +14,26 @@
#include <kunit/test.h>
-static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
+static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
ssize_t nr_vmas)
{
- int i;
+ int i, ret = -ENOMEM;
MA_STATE(mas, mt, 0, 0);
if (!nr_vmas)
- return;
+ return -ENOENT;
mas_lock(&mas);
- for (i = 0; i < nr_vmas; i++)
- vma_mas_store(&vmas[i], &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;
+ }
+ ret = 0;
+
+failed:
mas_unlock(&mas);
+ return ret;
}
/*
@@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
};
mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
- __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
+ KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
__damon_va_three_regions(&mm, regions);
--
2.35.1
Cc-ing kunit people.
Hi Liam,
Could we put touching file name on the summary?
E.g., mm/damon/vaddr-test: Stop using ...
On Tue, 17 Jan 2023 02:34:19 +0000 Liam Howlett <[email protected]> wrote:
> From: "Liam R. Howlett" <[email protected]>
>
> Prepare for the removal of the vma_mas_store() function by open coding
> the maple tree store in this test code. Set the range of the maple
> state and call the store function directly.
>
> Cc: SeongJae Park <[email protected]>
> Cc: [email protected]
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Liam R. Howlett <[email protected]>
> ---
> mm/damon/vaddr-test.h | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> index bce37c487540..6098933d3272 100644
> --- a/mm/damon/vaddr-test.h
> +++ b/mm/damon/vaddr-test.h
> @@ -14,19 +14,26 @@
>
> #include <kunit/test.h>
>
> -static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> +static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> ssize_t nr_vmas)
> {
> - int i;
> + int i, ret = -ENOMEM;
> MA_STATE(mas, mt, 0, 0);
>
> if (!nr_vmas)
> - return;
> + return -ENOENT;
>
> mas_lock(&mas);
> - for (i = 0; i < nr_vmas; i++)
> - vma_mas_store(&vmas[i], &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;
> + }
> + ret = 0;
> +
> +failed:
> mas_unlock(&mas);
> + return ret;
> }
>
> /*
> @@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
> };
>
> mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
> - __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
> + KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
In case of the __link_vmas() failure, I think we should skip this test using
'kunit_skip()', rather marking this test failed.
Thanks,
SJ
>
> __damon_va_three_regions(&mm, regions);
>
> --
> 2.35.1
On Tue, 17 Jan 2023 19:11:09 +0000 SeongJae Park <[email protected]> wrote:
> Cc-ing kunit people.
>
> Hi Liam,
>
>
> Could we put touching file name on the summary?
> E.g., mm/damon/vaddr-test: Stop using ...
>
> On Tue, 17 Jan 2023 02:34:19 +0000 Liam Howlett <[email protected]> wrote:
>
> > From: "Liam R. Howlett" <[email protected]>
> >
> > Prepare for the removal of the vma_mas_store() function by open coding
> > the maple tree store in this test code. Set the range of the maple
> > state and call the store function directly.
> >
> > Cc: SeongJae Park <[email protected]>
> > Cc: [email protected]
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Liam R. Howlett <[email protected]>
> > ---
> > mm/damon/vaddr-test.h | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> > index bce37c487540..6098933d3272 100644
> > --- a/mm/damon/vaddr-test.h
> > +++ b/mm/damon/vaddr-test.h
> > @@ -14,19 +14,26 @@
> >
> > #include <kunit/test.h>
> >
> > -static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > +static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > ssize_t nr_vmas)
> > {
> > - int i;
> > + int i, ret = -ENOMEM;
> > MA_STATE(mas, mt, 0, 0);
> >
> > if (!nr_vmas)
> > - return;
> > + return -ENOENT;
Also I think it's ok to return zero here, as this function successfully linked
zero vmas as requested.
Thanks,
SJ
> >
> > mas_lock(&mas);
> > - for (i = 0; i < nr_vmas; i++)
> > - vma_mas_store(&vmas[i], &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;
> > + }
> > + ret = 0;
> > +
> > +failed:
> > mas_unlock(&mas);
> > + return ret;
> > }
> >
> > /*
> > @@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
> > };
> >
> > mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
> > - __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
> > + KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
>
> In case of the __link_vmas() failure, I think we should skip this test using
> 'kunit_skip()', rather marking this test failed.
>
>
> Thanks,
> SJ
>
> >
> > __damon_va_three_regions(&mm, regions);
> >
> > --
> > 2.35.1
>
On Tue, Jan 17, 2023 at 11:11 AM SeongJae Park <[email protected]> wrote:
>
> Cc-ing kunit people.
>
> Hi Liam,
>
>
> Could we put touching file name on the summary?
> E.g., mm/damon/vaddr-test: Stop using ...
>
> On Tue, 17 Jan 2023 02:34:19 +0000 Liam Howlett <[email protected]> wrote:
>
> > From: "Liam R. Howlett" <[email protected]>
> >
> > Prepare for the removal of the vma_mas_store() function by open coding
> > the maple tree store in this test code. Set the range of the maple
> > state and call the store function directly.
> >
> > Cc: SeongJae Park <[email protected]>
> > Cc: [email protected]
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Liam R. Howlett <[email protected]>
> > ---
> > mm/damon/vaddr-test.h | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> > index bce37c487540..6098933d3272 100644
> > --- a/mm/damon/vaddr-test.h
> > +++ b/mm/damon/vaddr-test.h
> > @@ -14,19 +14,26 @@
> >
> > #include <kunit/test.h>
> >
> > -static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > +static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > ssize_t nr_vmas)
> > {
> > - int i;
> > + int i, ret = -ENOMEM;
> > MA_STATE(mas, mt, 0, 0);
> >
> > if (!nr_vmas)
> > - return;
> > + return -ENOENT;
We could pass in the `test` object here and give more detailed info, e.g.
(if !nr_vmas)
kunit_skip(test, "...");
And below could be
bool stored_all = false; // instead of ret
...
for (...) {
}
stored_all = true;
failed:
mas_unlock(&mas);
if (!stored_all) kunit_skip(test, "failed to...");
> >
> > mas_lock(&mas);
> > - for (i = 0; i < nr_vmas; i++)
> > - vma_mas_store(&vmas[i], &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;
> > + }
> > + ret = 0;
> > +
> > +failed:
> > mas_unlock(&mas);
> > + return ret;
> > }
> >
> > /*
> > @@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
> > };
> >
> > mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
> > - __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
> > + KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
>
> In case of the __link_vmas() failure, I think we should skip this test using
> 'kunit_skip()', rather marking this test failed.
As noted above, I'd suggest we also pass in the `test` object to
__link_vmas() and call kunit_skip() from there.
>
>
> Thanks,
> SJ
Daniel
* Daniel Latypov <[email protected]> [230117 17:20]:
> On Tue, Jan 17, 2023 at 11:11 AM SeongJae Park <[email protected]> wrote:
> >
> > Cc-ing kunit people.
> >
> > Hi Liam,
> >
> >
> > Could we put touching file name on the summary?
> > E.g., mm/damon/vaddr-test: Stop using ...
> >
> > On Tue, 17 Jan 2023 02:34:19 +0000 Liam Howlett <[email protected]> wrote:
> >
> > > From: "Liam R. Howlett" <[email protected]>
> > >
> > > Prepare for the removal of the vma_mas_store() function by open coding
> > > the maple tree store in this test code. Set the range of the maple
> > > state and call the store function directly.
> > >
> > > Cc: SeongJae Park <[email protected]>
> > > Cc: [email protected]
> > > Reported-by: kernel test robot <[email protected]>
> > > Signed-off-by: Liam R. Howlett <[email protected]>
> > > ---
> > > mm/damon/vaddr-test.h | 19 +++++++++++++------
> > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> > > index bce37c487540..6098933d3272 100644
> > > --- a/mm/damon/vaddr-test.h
> > > +++ b/mm/damon/vaddr-test.h
> > > @@ -14,19 +14,26 @@
> > >
> > > #include <kunit/test.h>
> > >
> > > -static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > > +static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > > ssize_t nr_vmas)
> > > {
> > > - int i;
> > > + int i, ret = -ENOMEM;
> > > MA_STATE(mas, mt, 0, 0);
> > >
> > > if (!nr_vmas)
> > > - return;
> > > + return -ENOENT;
>
> We could pass in the `test` object here and give more detailed info, e.g.
> (if !nr_vmas)
> kunit_skip(test, "...");
>
> And below could be
>
> bool stored_all = false; // instead of ret
> ...
> for (...) {
>
> }
> stored_all = true;
>
> failed:
> mas_unlock(&mas);
> if (!stored_all) kunit_skip(test, "failed to...");
>
> > >
> > > mas_lock(&mas);
> > > - for (i = 0; i < nr_vmas; i++)
> > > - vma_mas_store(&vmas[i], &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;
> > > + }
> > > + ret = 0;
> > > +
> > > +failed:
> > > mas_unlock(&mas);
> > > + return ret;
> > > }
> > >
> > > /*
> > > @@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
> > > };
> > >
> > > mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
> > > - __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
> > > + KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
> >
> > In case of the __link_vmas() failure, I think we should skip this test using
> > 'kunit_skip()', rather marking this test failed.
>
> As noted above, I'd suggest we also pass in the `test` object to
> __link_vmas() and call kunit_skip() from there.
My thoughts were if we are testing adding nothing to the list, then
there is probably a problem with the test and so that should be
highlighted with a failure.
I really don't mind either way.
Hello Daniel and Liam,
Sorry for late reply.
On Tue, 17 Jan 2023 22:47:36 +0000 Liam Howlett <[email protected]> wrote:
> * Daniel Latypov <[email protected]> [230117 17:20]:
> > On Tue, Jan 17, 2023 at 11:11 AM SeongJae Park <[email protected]> wrote:
> > >
> > > Cc-ing kunit people.
> > >
> > > Hi Liam,
> > >
> > >
> > > Could we put touching file name on the summary?
> > > E.g., mm/damon/vaddr-test: Stop using ...
> > >
> > > On Tue, 17 Jan 2023 02:34:19 +0000 Liam Howlett <[email protected]> wrote:
> > >
> > > > From: "Liam R. Howlett" <[email protected]>
> > > >
> > > > Prepare for the removal of the vma_mas_store() function by open coding
> > > > the maple tree store in this test code. Set the range of the maple
> > > > state and call the store function directly.
> > > >
> > > > Cc: SeongJae Park <[email protected]>
> > > > Cc: [email protected]
> > > > Reported-by: kernel test robot <[email protected]>
> > > > Signed-off-by: Liam R. Howlett <[email protected]>
> > > > ---
> > > > mm/damon/vaddr-test.h | 19 +++++++++++++------
> > > > 1 file changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> > > > index bce37c487540..6098933d3272 100644
> > > > --- a/mm/damon/vaddr-test.h
> > > > +++ b/mm/damon/vaddr-test.h
> > > > @@ -14,19 +14,26 @@
> > > >
> > > > #include <kunit/test.h>
> > > >
> > > > -static void __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > > > +static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
> > > > ssize_t nr_vmas)
> > > > {
> > > > - int i;
> > > > + int i, ret = -ENOMEM;
> > > > MA_STATE(mas, mt, 0, 0);
> > > >
> > > > if (!nr_vmas)
> > > > - return;
> > > > + return -ENOENT;
> >
> > We could pass in the `test` object here and give more detailed info, e.g.
> > (if !nr_vmas)
> > kunit_skip(test, "...");
> >
> > And below could be
> >
> > bool stored_all = false; // instead of ret
> > ...
> > for (...) {
> >
> > }
> > stored_all = true;
> >
> > failed:
> > mas_unlock(&mas);
> > if (!stored_all) kunit_skip(test, "failed to...");
> >
> > > >
> > > > mas_lock(&mas);
> > > > - for (i = 0; i < nr_vmas; i++)
> > > > - vma_mas_store(&vmas[i], &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;
> > > > + }
> > > > + ret = 0;
> > > > +
> > > > +failed:
> > > > mas_unlock(&mas);
> > > > + return ret;
> > > > }
> > > >
> > > > /*
> > > > @@ -71,7 +78,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
> > > > };
> > > >
> > > > mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
> > > > - __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas));
> > > > + KUNIT_EXPECT_EQ(test, __link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)), 0);
> > >
> > > In case of the __link_vmas() failure, I think we should skip this test using
> > > 'kunit_skip()', rather marking this test failed.
> >
> > As noted above, I'd suggest we also pass in the `test` object to
> > __link_vmas() and call kunit_skip() from there.
>
> My thoughts were if we are testing adding nothing to the list, then
> there is probably a problem with the test and so that should be
> highlighted with a failure.
>
> I really don't mind either way.
I didn't wrote '__link_vmas()' to test vma manipulation functions it internally
uses, but just to offload test setup for 'damon_test_three_regions_in_vmas()'.
I agree that the detailed failure reason could be helpful for better
understanding as the function can now fail from 'mas_store_gfp()'s memory
allocation failure.
That said, I think we can get the detail from the return value of
'__link_vmas()'. I'm further worrying if passing 'test' object to the function
makes people think the function itself is for testing something inside it.
Also, I don't think the function returning non-error for zero value 'nr_vmas'
as a problem but just expected behavior, as previously commented[1].
So I'd prefer doing kunit_skip() here.
If I'm missing something or wrong, please let me know.
[1] https://lore.kernel.org/damon/[email protected]/
Thanks,
SJ
* SeongJae Park <[email protected]> [230118 21:00]:
> Hello Daniel and Liam,
>
>
> Sorry for late reply.
>
> On Tue, 17 Jan 2023 22:47:36 +0000 Liam Howlett <[email protected]> wrote:
>
> > * Daniel Latypov <[email protected]> [230117 17:20]:
> > > On Tue, Jan 17, 2023 at 11:11 AM SeongJae Park <[email protected]> wrote:
> > > >
> > > > Cc-ing kunit people.
> > > >
> > > > Hi Liam,
> > > >
> > > >
> > > > Could we put touching file name on the summary?
> > > > E.g., mm/damon/vaddr-test: Stop using ...
Sure thing.
...
> > > >
> > > > In case of the __link_vmas() failure, I think we should skip this test using
> > > > 'kunit_skip()', rather marking this test failed.
> > >
> > > As noted above, I'd suggest we also pass in the `test` object to
> > > __link_vmas() and call kunit_skip() from there.
> >
> > My thoughts were if we are testing adding nothing to the list, then
> > there is probably a problem with the test and so that should be
> > highlighted with a failure.
> >
> > I really don't mind either way.
>
> I didn't wrote '__link_vmas()' to test vma manipulation functions it internally
> uses, but just to offload test setup for 'damon_test_three_regions_in_vmas()'.
> I agree that the detailed failure reason could be helpful for better
> understanding as the function can now fail from 'mas_store_gfp()'s memory
> allocation failure.
>
> That said, I think we can get the detail from the return value of
> '__link_vmas()'. I'm further worrying if passing 'test' object to the function
> makes people think the function itself is for testing something inside it.
>
> Also, I don't think the function returning non-error for zero value 'nr_vmas'
> as a problem but just expected behavior, as previously commented[1].
>
> So I'd prefer doing kunit_skip() here.
Sounds good to me. Thanks for clarifying.
I'll make the necessary changes for v4 of the patches.
>
> If I'm missing something or wrong, please let me know.
>
> [1] https://lore.kernel.org/damon/[email protected]/
>
>