Re: Re: [RFC PATCH 5/5] mm/damon: Add kunit tests

[SeongJae Park <sj38.park@gmail.com>]

On Thu, 23 Jan 2020 13:12:55 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> On Fri, Jan 10, 2020 at 5:18 AM SeongJae Park <sjpark@amazon.com> wrote:
> >
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > This commit adds kunit based unit tests for DAMON.
> >
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> 
> Sorry for the late review on this: I am still getting caught up on my
> vacation backlog.

Thank you so much for this review, Brendan :)

BTW, I posted 'PATCH v1' of this [1] meanwhile and I forgot adding you as
recipients, sorry.  That said, the kunit related part has no change with the
next spin, so your reviews will applied.  Will not miss you from 'v2'.

[1] https://lore.kernel.org/linux-mm/20200120162757.32375-1-sjpark@amazon.com/

> 
> > ---
> >  mm/Kconfig      |  11 +
> >  mm/damon-test.h | 571 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  mm/damon.c      |   2 +
> >  3 files changed, 584 insertions(+)
> >  create mode 100644 mm/damon-test.h
> >
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index b7af8a1b5cb5..7b023799aa38 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -748,4 +748,15 @@ config DAMON
> >           be 1) accurate enough to be useful for performance-centric domains,
> >           and 2) sufficiently light-weight so that it can be applied online.
> >
> > +config DAMON_TEST
> 
> To be consistent with other KUnit tests, this should be "DAMON_KUNIT_TEST".

Good point, will do so.

> 
> > +       bool "Test for damon"
> > +       depends on DAMON && KUNIT
> > +       help
> > +         This builds the DAMON Kunit test suite.
> > +
> > +         For more information on KUnit and unit tests in general, please refer
> > +         to the KUnit documentation.
> > +
> > +         If unsure, say N.
> > +
> >  endmenu
> > diff --git a/mm/damon-test.h b/mm/damon-test.h
> > new file mode 100644
> > index 000000000000..0d94910b8fe5
> > --- /dev/null
> > +++ b/mm/damon-test.h
> > @@ -0,0 +1,571 @@
[...]
> > +
> > +static void damon_test_set_pids(struct kunit *test)
> > +{
> > +       unsigned long pids[] = {1, 2, 3};
> > +       char buf[64];
> > +
> > +       damon_set_pids(pids, 3);
> > +       damon_sprint_pids(buf, 64);
> > +       pr_info("buf: %s (%zu)\n", buf, strlen(buf));
> 
> Might want to use kunit_info here so it matches the TAP test log
> format. Not a requirement, just an FYI.

Oh, this is a debugging code I missed to delete.  Will delete from next spin.
Also, will use 'kunit_info()' like things if I need any log, either.

> 
> > +       KUNIT_EXPECT_EQ(test, 0, strncmp(buf, "1 2 3\n", 64));
> 
> Here and elsewhere: This should probably use KUNIT_EXPECT_STREQ().

Good point, will fix with the next spin.

> 
[...]
> > +
> > +static void damon_test_three_regions_in_vmas(struct kunit *test)
> > +{
> > +       struct region regions[3] = {0,};
> > +
> > +       struct vm_area_struct vmas[] = {
> > +               (struct vm_area_struct) {.vm_start = 10, .vm_end = 20},
> > +               (struct vm_area_struct) {.vm_start = 20, .vm_end = 25},
> > +               (struct vm_area_struct) {.vm_start = 200, .vm_end = 210},
> > +               (struct vm_area_struct) {.vm_start = 210, .vm_end = 220},
> > +               (struct vm_area_struct) {.vm_start = 300, .vm_end = 305},
> > +               (struct vm_area_struct) {.vm_start = 307, .vm_end = 330},
> > +       };
> > +       vmas[0].vm_next = &vmas[1];
> > +       vmas[1].vm_next = &vmas[2];
> > +       vmas[2].vm_next = &vmas[3];
> > +       vmas[3].vm_next = &vmas[4];
> > +       vmas[4].vm_next = &vmas[5];
> > +       vmas[5].vm_next = NULL;
> > +
> > +       damon_three_regions_in_vmas(&vmas[0], regions);
> > +
> > +       KUNIT_EXPECT_EQ(test, 10ul, regions[0].start);
> > +       KUNIT_EXPECT_EQ(test, 25ul, regions[0].end);
> > +       KUNIT_EXPECT_EQ(test, 200ul, regions[1].start);
> > +       KUNIT_EXPECT_EQ(test, 220ul, regions[1].end);
> > +       KUNIT_EXPECT_EQ(test, 300ul, regions[2].start);
> > +       KUNIT_EXPECT_EQ(test, 330ul, regions[2].end);
> 
> It's not obvious to me what property you are proving here. Might want
> to add a comment.

It's closely related with DAMON internal logic.  Will add a reference to a
document for the internal logic and what I'm checking, as you suggested.  Same
for other ambiguous test code fragments you pointed out below.

> 
[...]
> > +
> > +static void damon_test_aggregate(struct kunit *test)
> > +{
> > +       unsigned long pids[] = {1, 2, 3};
> > +       unsigned long saddr[][3] = {{10, 20, 30}, {5, 42, 49}, {13, 33, 55} };
> > +       unsigned long eaddr[][3] = {{15, 27, 40}, {31, 45, 55}, {23, 44, 66} };
> > +       unsigned long accesses[][3] = {{42, 95, 84}, {10, 20, 30}, {0, 1, 2} };
> > +       struct damon_task *t;
> > +       struct damon_region *r;
> > +       int it, ir;
> > +       ssize_t sz, sr, sp;
> > +
> > +       damon_set_pids(pids, 3);
> > +
> > +       it = 0;
> > +       damon_for_each_task(t) {
> > +               for (ir = 0; ir < 3; ir++) {
> > +                       r = damon_new_region(saddr[it][ir], eaddr[it][ir]);
> > +                       r->nr_accesses = accesses[it][ir];
> > +                       damon_add_region_tail(r, t);
> > +               }
> > +               it++;
> > +       }
> > +       kdamond_flush_aggregated();
> 
> I think this test case is also difficult to understand. I think you
> probably need at least a comment on what this test case does.
> 
> > +       it = 0;
> > +       damon_for_each_task(t) {
> > +               ir = 0;
> > +               damon_for_each_region(r, t) {
> > +                       KUNIT_EXPECT_EQ(test, 0u, r->nr_accesses);
> > +                       ir++;
> > +               }
> > +               KUNIT_EXPECT_EQ(test, 3, ir);
> > +               it++;
> > +       }
> > +       KUNIT_EXPECT_EQ(test, 3, it);
> > +
> > +       sr = sizeof(r->vm_start) + sizeof(r->vm_end) + sizeof(r->nr_accesses);
> > +       sp = sizeof(t->pid) + sizeof(unsigned int) + 3 * sr;
> > +       sz = sizeof(struct timespec64) + sizeof(unsigned int) + 3 * sp;
> > +       KUNIT_EXPECT_EQ(test, (unsigned int)sz, damon_rbuf_offset);
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
[...]
> > +
> > +static void damon_test_update_two_gaps(struct kunit *test)
> > +{
> 
> I think this test case is also difficult to understand. I think you
> probably need at least a comment on what this test case does.
> 
> > +       struct damon_task *t;
> > +       struct damon_region *r, *prev = NULL;
> > +       unsigned long regions[] = {10, 20, 20, 30,
> > +               50, 55, 55, 57, 57, 59,
> > +               70, 80, 80, 90, 90, 100};       /* 10-30, 50-59, 70-100 */
> > +       struct region new_regions[3] = {
> > +               (struct region){.start = 5, .end = 27},
> > +               (struct region){.start = 45, .end = 55},
> > +               (struct region){.start = 73, .end = 104} };
> > +       int i;
> > +       bool first_gap = true;
> > +
> > +       t = damon_new_task(42);
> > +       for (i = 0; i < ARRAY_SIZE(regions) / 2; i++) {
> > +               r = damon_new_region(regions[i * 2], regions[i * 2 + 1]);
> > +               damon_add_region_tail(r, t);
> > +       }
> > +       damon_add_task_tail(t);
> > +
> > +       damon_apply_three_regions(t, new_regions);
> > +
> > +       damon_for_each_region(r, t) {
> > +               if (prev == NULL) {
> > +                       KUNIT_EXPECT_EQ(test, r->vm_start, 5ul);
> > +                       goto next;
> > +               }
> > +
> > +               if (prev->vm_end != r->vm_start && first_gap) {
> > +                       KUNIT_EXPECT_EQ(test, prev->vm_end, 27ul);
> > +                       KUNIT_EXPECT_EQ(test, r->vm_start, 45ul);
> > +                       first_gap = false;
> > +                       goto next;
> > +               }
> > +
> > +               if (prev->vm_end != r->vm_start && !first_gap) {
> > +                       KUNIT_EXPECT_EQ(test, prev->vm_end, 55ul);
> > +                       KUNIT_EXPECT_EQ(test, r->vm_start, 73ul);
> > +                       goto next;
> > +               }
> > +
> > +next:
> > +               prev = r;
> > +       }
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
> > +static void damon_test_update_two_gaps2(struct kunit *test)
> > +{
> 
> Same here.
> 
> > +       struct damon_task *t;
> > +       struct damon_region *r;
> > +       /* 10-20-30, 50-55-57-59, 70-80-90-100 */
> > +       unsigned long regions[] = {10, 20, 20, 30,
> > +               50, 55, 55, 57, 57, 59,
> > +               70, 80, 80, 90, 90, 100};
> > +       struct region new_regions[3] = {
> > +               (struct region){.start = 5, .end = 27},
> > +               (struct region){.start = 56, .end = 57},
> > +               (struct region){.start = 65, .end = 104} };
> > +       /* expect 5-27, 56-57, 65-80-90-104 */
> > +       unsigned long answers[] = {5, 20, 20, 27,
> > +               56, 57,
> > +               65, 80, 80, 90, 90, 104};
> > +       int i;
> > +
> > +       t = damon_new_task(42);
> > +       for (i = 0; i < ARRAY_SIZE(regions) / 2; i++) {
> > +               r = damon_new_region(regions[i * 2], regions[i * 2 + 1]);
> > +               damon_add_region_tail(r, t);
> > +       }
> > +       damon_add_task_tail(t);
> > +
> > +       damon_apply_three_regions(t, new_regions);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(answers) / 2; i++) {
> > +               r = damon_nth_region_of(t, i);
> > +               KUNIT_EXPECT_EQ(test, r->vm_start, answers[i * 2]);
> > +               KUNIT_EXPECT_EQ(test, r->vm_end, answers[i++ * 2 + 1]);
> > +       }
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
> > +static void damon_test_update_two_gaps3(struct kunit *test)
> > +{
> 
> Same here.
> 
> > +       struct damon_task *t;
> > +       struct damon_region *r;
> > +       /* 10-20-30, 50-55-57-59, 70-80-90-100 */
> > +       unsigned long regions[] = {10, 20, 20, 30,
> > +               50, 55, 55, 57, 57, 59,
> > +               70, 80, 80, 90, 90, 100};
> > +       struct region new_regions[3] = {
> > +               (struct region){.start = 5, .end = 27},
> > +               (struct region){.start = 61, .end = 63},
> > +               (struct region){.start = 65, .end = 104} };
> > +       /* expect 5-27, 56-57, 65-80-90-104 */
> > +       unsigned long answers[] = {5, 20, 20, 27,
> > +               61, 63,
> > +               65, 80, 80, 90, 90, 104};
> > +       int i;
> > +
> > +       t = damon_new_task(42);
> > +       for (i = 0; i < ARRAY_SIZE(regions) / 2; i++) {
> > +               r = damon_new_region(regions[i * 2], regions[i * 2 + 1]);
> > +               damon_add_region_tail(r, t);
> > +       }
> > +       damon_add_task_tail(t);
> > +
> > +       damon_apply_three_regions(t, new_regions);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(answers) / 2; i++) {
> > +               r = damon_nth_region_of(t, i);
> > +               KUNIT_EXPECT_EQ(test, r->vm_start, answers[i * 2]);
> > +               KUNIT_EXPECT_EQ(test, r->vm_end, answers[i++ * 2 + 1]);
> > +       }
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
> > +static void damon_test_update_two_gaps4(struct kunit *test)
> > +{
> 
> Ditto.
> 
> > +       struct damon_task *t;
> > +       struct damon_region *r;
> > +       /* 10-20-30, 50-55-57-59, 70-80-90-100 */
> > +       unsigned long regions[] = {10, 20, 20, 30,
> > +               50, 55, 55, 57, 57, 59,
> > +               70, 80, 80, 90, 90, 100};
> > +       struct region new_regions[3] = {
> > +               (struct region){.start = 5, .end = 7},
> > +               (struct region){.start = 30, .end = 32},
> > +               (struct region){.start = 65, .end = 68} };
> > +       /* expect 5-27, 56-57, 65-80-90-104 */
> > +       unsigned long answers[] = {5, 7, 30, 32, 65, 68};
> > +       int i;
> > +
> > +       t = damon_new_task(42);
> > +       for (i = 0; i < ARRAY_SIZE(regions) / 2; i++) {
> > +               r = damon_new_region(regions[i * 2], regions[i * 2 + 1]);
> > +               damon_add_region_tail(r, t);
> > +       }
> > +       damon_add_task_tail(t);
> > +
> > +       damon_apply_three_regions(t, new_regions);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(answers) / 2; i++) {
> > +               r = damon_nth_region_of(t, i);
> > +               KUNIT_EXPECT_EQ(test, r->vm_start, answers[i * 2]);
> > +               KUNIT_EXPECT_EQ(test, r->vm_end, answers[i++ * 2 + 1]);
> > +       }
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
[...]


Will apply all of your suggestions, soon.


Thanks,
SeongJae Park