Re: Re: [PATCH v2 8/9] mm/damon: Add kunit tests

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

On Thu, 30 Jan 2020 16:14:45 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> On Tue, Jan 28, 2020 at 1:01 AM <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>
> > ---
> >  MAINTAINERS     |   1 +
> >  mm/Kconfig      |  11 +
> >  mm/damon-test.h | 571 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  mm/damon.c      |   2 +
> >  4 files changed, 585 insertions(+)
> >  create mode 100644 mm/damon-test.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5c8a0c4e69b8..cb091bee16c7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4616,6 +4616,7 @@ M:        SeongJae Park <sjpark@amazon.de>
> >  L:     linux-mm@kvack.org
> >  S:     Maintained
> >  F:     mm/damon.c
> > +F:     mm/damon-test.h
> >  F:     tools/damon/*
> >  F:     Documentation/admin-guide/mm/data_access_monitor.rst
> >
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 144fb916aa8b..8b34711c6ee1 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -751,4 +751,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_KUNIT_TEST
> > +       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..3d92548058a7
> > --- /dev/null
> > +++ b/mm/damon-test.h
> [...]
> > +/*
> > + * Test damon_three_regions_in_vmas() function
> > + *
> > + * DAMON converts the complex and dynamic memory mappings of each target task
> > + * to three discontiguous regions which cover every mapped areas.  The two gaps
> > + * between the areas is two biggest unmapped areas in the original mapping.
> > + * 'damon_three_regions_in_vmas() receives an address space of a process.  It
> > + * first identifies the start of mappings, end of mappings, and the two biggest
> > + * unmapped areas.  After that, based on the information, it constructs the
> > + * three regions and returns.  For more detail, refer to the comment of
> > + * 'damon_init_regions_of()' function definition in 'mm/damon.c' file.
> > + *
> > + * For example, suppose virtual address ranges of 10-20, 20-25, 200-210,
> > + * 210-220, 300-305, and 307-330 (Other comments represent this mappings in
> > + * more short form: 10-20-25, 200-210-220, 300-305, 307-330) of a process are
> > + * mapped.  To cover every mappings, the three regions should start with 10,
> > + * and end with 305.
> 
> Why? What's special about those numbers? Don't you need 10 to 330 to
> cover all the mappings?

No big special, I just wanted to show whether the function really identify the
two biggest gaps in given regions and convert to the three regions.

I'm not using onlt 10 to 330 because the two biggest unmapped area are usually
the gap between 1) heap and the mmap()-ed area, and 2) the mmap()-ed area and
stack, which is so huge.  Therefore I wanted to test the gap existing case.  If
you have any different opinion, please let me know.

Will document this point more in next spin.

[...]
> > +
> > +static void damon_test_write_rbuf(struct kunit *test)
> > +{
> > +       char *data;
> > +
> > +       data = "hello";
> > +       damon_write_rbuf(data, strnlen(data, 256));
> > +       KUNIT_EXPECT_EQ(test, damon_rbuf_offset, 5u);
> > +
> > +       damon_write_rbuf(data, 0);
> > +       KUNIT_EXPECT_EQ(test, damon_rbuf_offset, 5u);
> > +
> > +       KUNIT_EXPECT_STREQ(test, (char *)damon_rbuf, data);
> > +}
> > +
> > +/*
> > + * Test 'damon_apply_three_regions()'
> > + *
> > + * test                        kunit object
> > + * regions             an array containing start/end addresses of current
> > + *                     monitoring target regions
> > + * nr_regions          the number of the addresses in 'regions'
> > + * three_regions       The three regions that need to be applied now
> > + * expected            start/end addresses of monitoring target regions that
> > + *                     'three_regions' are applied
> > + * nr_expected         the number of addresses in 'expected'
> > + *
> > + * The memory mapping of the target processes changes dynamically.  To follow
> > + * the change, DAMON periodically reads the mappings, simplifies it to the
> > + * three regions, and updates the monitoring target regions to fit in the three
> > + * regions.  The update of current target regions is the role of
> > + * 'damon_apply_three_regions()'.
> > + *
> > + * This test passes the given target regions and the new three regions that
> > + * need to be applied to the function and check whether it updates the regions
> > + * as expected.
> > + */
> > +static void damon_do_test_apply_three_regions(struct kunit *test,
> > +                               unsigned long *regions, int nr_regions,
> > +                               struct region *three_regions,
> > +                               unsigned long *expected, int nr_expected)
> > +{
> > +       struct damon_task *t;
> > +       struct damon_region *r;
> > +       int i;
> > +
> > +       t = damon_new_task(42);
> > +       for (i = 0; i < nr_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, three_regions);
> > +
> > +       for (i = 0; i < nr_expected / 2; i++) {
> > +               r = damon_nth_region_of(t, i);
> > +               KUNIT_EXPECT_EQ(test, r->vm_start, expected[i * 2]);
> > +               KUNIT_EXPECT_EQ(test, r->vm_end, expected[i * 2 + 1]);
> > +       }
> > +
> > +       damon_cleanup_global_state();
> > +}
> > +
> > +/* below 4 functions test differnt inputs for 'damon_apply_three_regions()' */
> 
> Spelling. Also, I think this comment doesn't really say anything that
> you cannot get from reading the code. Maybe provide some more details?
> Maybe add why you picked the inputs that you did?

Good point.  Will add more comments your points in next spin.

> 
> > +static void damon_test_apply_three_regions1(struct kunit *test)
> > +{
> > +       /* 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};
> > +       /* 5-27, 45-55, 73-104 */
> > +       struct region new_three_regions[3] = {
> > +               (struct region){.start = 5, .end = 27},
> > +               (struct region){.start = 45, .end = 55},
> > +               (struct region){.start = 73, .end = 104} };
> > +       /* 5-20-27, 45-55, 73-80-90-104 */
> > +       unsigned long expected[] = {5, 20, 20, 27, 45, 55,
> > +                               73, 80, 80, 90, 90, 104};
> > +
> > +       damon_do_test_apply_three_regions(test, regions, ARRAY_SIZE(regions),
> > +                       new_three_regions, expected, ARRAY_SIZE(expected));
> > +}
[...]
> > +
> > +static void damon_test_kdamond_need_stop(struct kunit *test)
> > +{
> > +       KUNIT_EXPECT_TRUE(test, kdamond_need_stop());
> 
> This doesn't look like a useful test, or a useful function. Why even
> check if the function always returns true? And if it doesn't always
> return true, then this test is not hermetic which is bad unit testing
> practice.

You're right, will remove the test code in next spin.

Thank you so much for your reviews!


Thanks,
SeongJae Park

> 
[...]