From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C92FCC33CAA for ; Thu, 23 Jan 2020 21:37:17 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6B43920665 for ; Thu, 23 Jan 2020 21:37:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="GpiRDsoW" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6B43920665 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 029966B027D; Thu, 23 Jan 2020 16:37:17 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F1CB06B027E; Thu, 23 Jan 2020 16:37:16 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E0BC26B027F; Thu, 23 Jan 2020 16:37:16 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0095.hostedemail.com [216.40.44.95]) by kanga.kvack.org (Postfix) with ESMTP id C5A106B027D for ; Thu, 23 Jan 2020 16:37:16 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 6B8F98248047 for ; Thu, 23 Jan 2020 21:37:16 +0000 (UTC) X-FDA: 76410210072.20.ship33_3f68834a6430a X-HE-Tag: ship33_3f68834a6430a X-Filterd-Recvd-Size: 15963 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Thu, 23 Jan 2020 21:37:15 +0000 (UTC) Received: by mail-wr1-f68.google.com with SMTP id t2so4931074wrr.1 for ; Thu, 23 Jan 2020 13:37:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to; bh=ZWpBhbXeVZLaPeJZtMiBvJEIVffq3Z2vtGP/vPPTPKA=; b=GpiRDsoWq45KNCle0oqYym+Cf//LmirQry38EQxyuDci+fOCOtrJBGIAk80J6Mz6oI bug/R6fmCPyKMLEo6QS4B7Lm5AD0t0qQnhWQyniozx460nhoPzYJ5oLpp/XtaD01yZew YEtHmfe344UjiWb3+l3p1U/iXjU7zA/FT5g5zRRtzGr7INplASA+QHimqF8dHZRSZTg2 jGbhV+BeIE0hCYvbC0UO1XXL7MB+pw6RO6UxKzeD3QmJGDJSh/1Y6XmVGYifnEe817Qy dwRkHwFPrmdb+9IEyRFgJuQN5GzpCF7Fmj7UpusSxJri0vbV95nHK7XHtdqNoSNgj5+Y y/sA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to; bh=ZWpBhbXeVZLaPeJZtMiBvJEIVffq3Z2vtGP/vPPTPKA=; b=pUPpPvoFyK4LkHZv3XzsKU22HYfjez3E8WNPcq9TlQQMa09DXxyCVsya3dypOBFWrk xvKzHD3tN1ZjAztptG5Q4gviGhPC6x94obMaD485hNaHv8dF+eq9Q1byLnzi0PqDU2uY Y5dHc9JYoIlqxS+7E7cHRdz5seQYt60QV5quG9Bm3ob+sXNZD2HJpgn83mhlGIE1gDu4 u32qSuCZVsLhY4k8urXxVejIRNw/QO9pfRSTcsrUPHZ5AlHD4MbhoLsQHVM8nc/3G+jt Y6OLg34cyQNQEygUjxyRqnKrrsR3IMAIKSwYcSbyR4k5wllU85iyqNECCbDN3va9M+1O 8AiQ== X-Gm-Message-State: APjAAAUybag37Xv4vQflWGey+PyiURiN67sL0Tc9Q3ZVaVqwPE/Juvko 82Yro1h8Gr4tssswk4OLGU8= X-Google-Smtp-Source: APXvYqyoK4ulX4Mr0m2DsYdXGNhPrk70c9qXNO3ZD66nFkirevCR+InAiANF6eKEMb/mUrLyheZhjA== X-Received: by 2002:a5d:448c:: with SMTP id j12mr75021wrq.125.1579815434134; Thu, 23 Jan 2020 13:37:14 -0800 (PST) Received: from x1cbn.MEGAROWIFI.local ([5.148.123.66]) by smtp.gmail.com with ESMTPSA id p18sm4192971wmb.8.2020.01.23.13.37.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jan 2020 13:37:13 -0800 (PST) From: SeongJae Park To: Brendan Higgins Cc: SeongJae Park , Andrew Morton , linux-mm@kvack.org, Linux Kernel Mailing List , Jonathan Corbet , "open list:DOCUMENTATION" , mgorman@suse.de, SeongJae Park , SeongJae Park Subject: Re: Re: [RFC PATCH 5/5] mm/damon: Add kunit tests Date: Thu, 23 Jan 2020 22:37:02 +0100 Message-Id: <20200123213702.14739-1-sj38.park@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: (raw) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, 23 Jan 2020 13:12:55 -0800 Brendan Higgins wrote: > On Fri, Jan 10, 2020 at 5:18 AM SeongJae Park wrote: > > > > From: SeongJae Park > > > > This commit adds kunit based unit tests for DAMON. > > > > Signed-off-by: SeongJae Park > > 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