Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
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>,
	SeongJae Park <sjpark@amazon.de>,
	SeongJae Park <sj38.park@gmail.com>,
	acme@kernel.org, amit@kernel.org, brendan.d.gregg@gmail.com,
	Jonathan Corbet <corbet@lwn.net>,
	dwmw@amazon.com, mgorman@suse.de,
	Steven Rostedt <rostedt@goodmis.org>,
	kirill@shutemov.name, colin.king@canonical.com,
	minchan@kernel.org, vdavydov.dev@gmail.com,
	vdavydov@parallels.com, linux-mm@kvack.org,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH v2 8/9] mm/damon: Add kunit tests
Date: Fri, 31 Jan 2020 05:55:27 +0100
Message-ID: <20200131045527.30320-1-sj38.park@gmail.com> (raw)
In-Reply-To: <CAFd5g47R_Cma_fi_rA1_9+Ns0dQPFRJ=zy9kq5vuMOGdzPHNKw@mail.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

> 
[...] 


  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-28  8:57 [PATCH v2 0/9] Introduce Data Access MONitor (DAMON) sjpark
2020-01-28  8:57 ` [PATCH v2 1/9] mm: " sjpark
2020-01-28 16:06   ` Randy Dunlap
2020-01-28 16:09     ` sjpark
2020-01-30 23:58   ` Brendan Higgins
2020-01-31  4:38     ` SeongJae Park
2020-01-28  8:57 ` [PATCH v2 2/9] mm/damon: Implement region based sampling sjpark
2020-01-28  8:57 ` [PATCH v2 3/9] mm/damon: Adaptively adjust regions sjpark
2020-01-28  8:57 ` [PATCH v2 4/9] mm/damon: Apply dynamic memory mapping changes sjpark
2020-01-28  8:59 ` [PATCH v2 5/9] mm/damon: Add debugfs interface sjpark
2020-01-28  9:00 ` [PATCH v2 6/9] mm/damon: Add minimal user-space tools sjpark
2020-01-31  0:02   ` Brendan Higgins
2020-01-31  4:44     ` SeongJae Park
2020-02-01  8:52       ` SeongJae Park
2020-01-28  9:00 ` [PATCH v2 7/9] Documentation/admin-guide/mm: Add a document for DAMON sjpark
2020-01-28  9:01 ` [PATCH v2 8/9] mm/damon: Add kunit tests sjpark
2020-01-31  0:14   ` Brendan Higgins
2020-01-31  4:55     ` SeongJae Park [this message]
2020-01-28  9:01 ` [PATCH v2 9/9] mm/damon: Add a tracepoint for result buffer writing sjpark
2020-01-28 10:20 ` [PATCH v2 0/9] Introduce Data Access MONitor (DAMON) Qian Cai
2020-01-28 10:49   ` sjpark
2020-01-28 11:20     ` Qian Cai
2020-01-28 12:00       ` sjpark
2020-01-29 12:56         ` Peter Zijlstra
2020-01-29 14:37           ` sjpark
2020-01-29 18:07             ` Peter Zijlstra
2020-01-29 19:06               ` SeongJae Park
2020-01-29 19:36                 ` Peter Zijlstra
2020-01-29 19:59                   ` 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=20200131045527.30320-1-sj38.park@gmail.com \
    --to=sj38.park@gmail.com \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=amit@kernel.org \
    --cc=brendan.d.gregg@gmail.com \
    --cc=brendanhiggins@google.com \
    --cc=colin.king@canonical.com \
    --cc=corbet@lwn.net \
    --cc=dwmw@amazon.com \
    --cc=kirill@shutemov.name \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=minchan@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sjpark@amazon.com \
    --cc=sjpark@amazon.de \
    --cc=vdavydov.dev@gmail.com \
    --cc=vdavydov@parallels.com \
    /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

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git