On Thu, 23 Jan 2020 13:12:55 -0800 Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote: > On Fri, Jan 10, 2020 at 5:18 AM SeongJae Park <sjpark@xxxxxxxxxx> wrote: > > > > From: SeongJae Park <sjpark@xxxxxxxxx> > > > > This commit adds kunit based unit tests for DAMON. > > > > Signed-off-by: SeongJae Park <sjpark@xxxxxxxxx> > > 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@xxxxxxxxxx/ > > > --- > > 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
