From: SeongJae Park <sj38.park@gmail.com>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: SeongJae Park <sjpark@amazon.com>,
Andrew Morton <akpm@linux-foundation.org>,
linux-mm@kvack.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Jonathan Corbet <corbet@lwn.net>,
"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
mgorman@suse.de, SeongJae Park <sj38.park@gmail.com>,
SeongJae Park <sjpark@amazon.de>
Subject: Re: Re: [RFC PATCH 5/5] mm/damon: Add kunit tests
Date: Thu, 23 Jan 2020 22:37:02 +0100 [thread overview]
Message-ID: <20200123213702.14739-1-sj38.park@gmail.com> (raw)
In-Reply-To: <CAFd5g47eB42E3X7m_rfmG=vEcMK9dtdAFZT5WjwV3sx3MO0-MQ@mail.gmail.com> (raw)
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
next prev parent reply other threads:[~2020-01-23 21:37 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-10 13:15 [RFC PATCH 0/5] Introduce Data Access MONitor (DAMON) SeongJae Park
2020-01-10 13:15 ` [RFC PATCH 1/5] mm: " SeongJae Park
2020-01-10 13:15 ` [RFC PATCH 2/5] mm/damon: Add debugfs interface SeongJae Park
2020-01-10 13:15 ` [RFC PATCH 3/5] mm/damon: Add minimal user-space tools SeongJae Park
2020-01-10 13:15 ` [RFC PATCH 4/5] Documentation/admin-guide/mm: Add a document for DAMON SeongJae Park
2020-01-23 21:17 ` Brendan Higgins
2020-01-23 21:17 ` Brendan Higgins
2020-01-23 21:41 ` SeongJae Park
2020-01-10 13:17 ` [RFC PATCH 5/5] mm/damon: Add kunit tests SeongJae Park
2020-01-23 21:12 ` Brendan Higgins
2020-01-23 21:12 ` Brendan Higgins
2020-01-23 21:37 ` SeongJae Park [this message]
2020-01-13 8:56 ` [RFC PATCH 0/5] Introduce Data Access MONitor (DAMON) SeongJae Park
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200123213702.14739-1-sj38.park@gmail.com \
--to=sj38.park@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=brendanhiggins@google.com \
--cc=corbet@lwn.net \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=sjpark@amazon.com \
--cc=sjpark@amazon.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.