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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56F6DC83F12 for ; Mon, 28 Aug 2023 20:17:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9C157280022; Mon, 28 Aug 2023 16:17:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 971088E001E; Mon, 28 Aug 2023 16:17:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 811B9280022; Mon, 28 Aug 2023 16:17:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 6F8008E001E for ; Mon, 28 Aug 2023 16:17:20 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 47E7BC0579 for ; Mon, 28 Aug 2023 20:17:20 +0000 (UTC) X-FDA: 81174623040.16.DB5D5BC Received: from mail-il1-f178.google.com (mail-il1-f178.google.com [209.85.166.178]) by imf23.hostedemail.com (Postfix) with ESMTP id 7401814001E for ; Mon, 28 Aug 2023 20:17:18 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=c60Co+ce; dmarc=none; spf=pass (imf23.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.166.178 as permitted sender) smtp.mailfrom=joel@joelfernandes.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1693253838; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=KlISlEu0cpqO/RX5l1LnfQgs++xGajM2SUCs3sXlUpk=; b=lmsRY0F6fmqpAw+OBxWD+mHn9CllgkCGxxN1crZZ5qOeLzJNPINlvR+te+z0rNndotlIB9 3UJPlyXL9v/28RpnyhsGO7R8YYqupO5/5qr5oc8l1rteVnz8k7BmnlEE9bzxpG/qpiCg/p eWP9+rX+Ua4Hb2ed5/bNnb4+cO32FjI= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=joelfernandes.org header.s=google header.b=c60Co+ce; dmarc=none; spf=pass (imf23.hostedemail.com: domain of joel@joelfernandes.org designates 209.85.166.178 as permitted sender) smtp.mailfrom=joel@joelfernandes.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693253838; a=rsa-sha256; cv=none; b=4xJCfolls1zboEX/PHt4YFl3TchB5GTXP3p5FV+ygZj1of0vQn0afCArrr2ZdcJC8fVjmI Q57GQ1cBvThyUUdOCf/KH/SRYD0nsL56X33OFoWfc7IXRCFHJvBiy+Qamn8dX5AEqKqRHH ZBl4rhEaJlPG5Z61uLtqnVN1Tas156s= Received: by mail-il1-f178.google.com with SMTP id e9e14a558f8ab-34cc0ad6f61so12958815ab.0 for ; Mon, 28 Aug 2023 13:17:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; t=1693253837; x=1693858637; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=KlISlEu0cpqO/RX5l1LnfQgs++xGajM2SUCs3sXlUpk=; b=c60Co+cegI9Y5Q06F0dfQqmtsy8XO3MinrtOd5T4Wr8llpp3kf8lsIiTUQ2ny+Tsff vb2fqKfRRLLySM6YB4OJkwvrJfZ2OFxBxI8+gFZicRH+x7Jrt7s2nUzpZC/c+FZMiD1M UoZjfR8H7TFg6onETibCksfLS7nkY/LRgfxUI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693253837; x=1693858637; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=KlISlEu0cpqO/RX5l1LnfQgs++xGajM2SUCs3sXlUpk=; b=cMhfBAGlbHrG7pQ8AkAJoLVpbKo1HsI7dAMU30LZM4wWkJskChxWp4y7O5fShEZAnO spjFnXTgH++McHQvR5+Lm6MfMr/cqgPGSSJVqIVxUU4VGlSznIeswfnuJG962qiNOen+ jo1/T8lcWF6ojhZy+8IYYNRSzkHULT/eveRFmP8K0pV5TJaToofWFHwngMjgo7SV1HAn AHD605XMKcY0B8pB5/ysKW4BAKh80D4CXQC9GN19wI9T+40NhkMA2gQ+Frw1BDAwYcT/ +/LWDMNpOxeJg9nC8mqtDJvo0q1+e2cEXeKDclwWP/vvyNk/FrPOt27JLwy2hFWCJvjK 3alw== X-Gm-Message-State: AOJu0YzKH16LYFi758AmrtZ3nylD704LdBppzB9qbUEf6Fp4GqLrUfgP WADQCfVycVjFctgnfd+xUR2mCg== X-Google-Smtp-Source: AGHT+IEUkxyBcdQwHwcAHtd1mz/VK3GZ/7Kg1/s7WFmrBaImGhpJQ7c/cZC6UaAAd1SZ+hyLWMl9hA== X-Received: by 2002:a05:6e02:152f:b0:34c:b981:52d4 with SMTP id i15-20020a056e02152f00b0034cb98152d4mr20217007ilu.31.1693253837442; Mon, 28 Aug 2023 13:17:17 -0700 (PDT) Received: from localhost (156.190.123.34.bc.googleusercontent.com. [34.123.190.156]) by smtp.gmail.com with ESMTPSA id i6-20020a92c946000000b0034cac5ced38sm2663157ilq.13.2023.08.28.13.17.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Aug 2023 13:17:16 -0700 (PDT) Date: Mon, 28 Aug 2023 20:17:16 +0000 From: Joel Fernandes To: Lorenzo Stoakes Cc: linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, Shuah Khan , Vlastimil Babka , Michal Hocko , Linus Torvalds , Kirill A Shutemov , "Liam R. Howlett" , "Paul E. McKenney" , Suren Baghdasaryan , Kalesh Singh , Lokesh Gidra Subject: Re: [PATCH v5 7/7] selftests: mm: Add a test for moving from an offset from start of mapping Message-ID: <20230828201716.GF1621761@google.com> References: <20230822015501.791637-1-joel@joelfernandes.org> <20230822015501.791637-8-joel@joelfernandes.org> <18a2c79d-3feb-41aa-93a9-bafbfb188cbc@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <18a2c79d-3feb-41aa-93a9-bafbfb188cbc@lucifer.local> X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 7401814001E X-Stat-Signature: afd5wooxqss99armwgz45i8f4aiu1nqt X-Rspam-User: X-HE-Tag: 1693253838-803607 X-HE-Meta: U2FsdGVkX1/NY1CthTXFkvvQmVHsI9mU8jOZ/HbQ44mI4uVCHjSP6pmiu+N64KnBjoLIu+TsLNy3g634xm6TCVSazANJQ/QvdemltrnxHVM7TlIwAuu2c7Ad6TjvNWwc36RCgmhwMNJuxmwOs7/MrTZRifEJr5H7IEVnPfILFru0tL0AGIBStyZ608spjvAuQXPfV4IA6/Y0R9kBUJA04A+VwbpXWUVlpZg/fFZ8zU+jpdUPQOBVOLJ9cZ8rHmjWk/RJislLlWzcjsq2idLzSj8tRtKKroGzpXQp4hOF6iROzXgGd4I5ie8PlU7AYuAau2MJThVeUj0zbQ31MQSmLSp1pl+cbrtsdpfeIAM2GNy4sZo21C5/VmbmyP01wJfBfYa3FHhXZygHe91vda3VM9+1tP94xL5y8Cjqzy0pVtDd2qOd4kQmbqkF8x9dMH2OEWaK64EGR+N+jb1LiOPb/H18HhjahwAqeKOmA76wLy9TwclgFuRVJ8PiHD6te3I4cexxWbjb3sm+mk1FPgFKeExwHwAqTrltWzxXmFIXILGRvjkiOvmh7TCZqOBVr6TAQOCMj6xjHpjKPUmbBW8VelA19qE0imL9sb+oXGluQw4iJu4HyzaKkj2s8Gz8zjto4ztLzNdVoq/R9W6oyiwIIduiOulBjvWRpkNLRJbF4FtSfT5TkDhbLFf8yJOW3qV/VUMj/pd0JntMSnCkCM7apKZxWO34O2hy5BlfDiA1+9TgiVEWIftIgDDB0XlKQ+AeOrWHXOsPImxOMc8M1C06NknoPAuf1iu9sQP7eERyuLwZv6P8Sg7nX6ILN1X7oSB6s3Xk72UJDPwY+nU1BNHWyRMBoguqnZMfouFldmQi0df6KDRnbOIHhy7/d0mhdcA811ZqENoVnmiefus2n7DWcoaca55Em5HXoTSAt4aNc6Y5OtNWFekntmJSEl07gY1sp8aUqSytLvELzgZo2v6 fRTOpd1L 4EFlgDagcrdWdU2BSP8y1tlcRc4/eHyx1DC48DgqkeIfMBK5Y1CPSz6gkf8MbSvbJcjSV/RyVEKLbMB0EzUomzMkuH3lXoLu4k/FP2InxyyKZf2Af+MB8OXq4figjSP4s6YjHMVNRlABrxwT3f2ahgjFroqe89MkuvCrsCVXSb6WkyI63iqxhLGpFLlS4P7qdkJoQT1K9boqj2tWj8fBnpEcTxzauhW7CGnp+ddgK3tFISkJQhl8LzmJYkve4BlS9k1Ie9Id+i4fn3y6/YqMkMCGt08GLD4OfYg+pnOWTcx8j22hphn1CewnMvgugvVvkcasoPjVbDqNpIrRGIsyl2HhIC6UnxvE5qJzh5syo4hQo2iOvqa2Hf90n7ZAtbXC+4vGvcJcl+HSagoCY+3+uZjoJh+Lq7D6lTYKEKlmYkDYMDULja20/I2pzZAhnspo1dAPhxGSP5yE7GQawX1gLnetagA== 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 Sun, Aug 27, 2023 at 11:12:14AM +0100, Lorenzo Stoakes wrote: > On Tue, Aug 22, 2023 at 01:55:00AM +0000, Joel Fernandes (Google) wrote: > > From: Joel Fernandes > > > > It is possible that the aligned address falls on no existing mapping, > > however that does not mean that we can just align it down to that. > > This test verifies that the "vma->vm_start != addr_to_align" check in > > can_align_down() prevents disastrous results if aligning down when > > source and dest are mutually aligned within a PMD but the source/dest > > addresses requested are not at the beginning of the respective mapping > > containing these addresses. > > > > Signed-off-by: Joel Fernandes (Google) > > --- > > tools/testing/selftests/mm/mremap_test.c | 189 ++++++++++++++++------- > > 1 file changed, 134 insertions(+), 55 deletions(-) > > > > diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c > > index f45d1abedc9c..c71ac8306c89 100644 > > --- a/tools/testing/selftests/mm/mremap_test.c > > +++ b/tools/testing/selftests/mm/mremap_test.c > > @@ -24,6 +24,7 @@ > > > > #define MIN(X, Y) ((X) < (Y) ? (X) : (Y)) > > #define SIZE_MB(m) ((size_t)m * (1024 * 1024)) > > +#define SIZE_KB(k) ((size_t)k * 1024) > > Same comment as previous re: wrapping k in parens, it doesn't really matter > here that much but for good practice (and consistency with MIN()) would be > nice to do. Ack. > > > > struct config { > > unsigned long long src_alignment; > > @@ -148,6 +149,60 @@ static bool is_range_mapped(FILE *maps_fp, void *start, void *end) > > return success; > > } > > > > +/* > > + * Returns the start address of the mapping on success, else returns > > + * NULL on failure. > > + */ > > +static void *get_source_mapping(struct config c) > > +{ > > + unsigned long long addr = 0ULL; > > + void *src_addr = NULL; > > + unsigned long long mmap_min_addr; > > + > > + mmap_min_addr = get_mmap_min_addr(); > > + /* > > + * For some tests, we need to not have any mappings below the > > + * source mapping. Add some headroom to mmap_min_addr for this. > > + */ > > + mmap_min_addr += 10 * _4MB; > > + > > +retry: > > + addr += c.src_alignment; > > + if (addr < mmap_min_addr) > > + goto retry; > > + > > + src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE, > > + MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED, > > + -1, 0); > > + if (src_addr == MAP_FAILED) { > > + if (errno == EPERM || errno == EEXIST) > > + goto retry; > > + goto error; > > + } > > + /* > > + * Check that the address is aligned to the specified alignment. > > + * Addresses which have alignments that are multiples of that > > + * specified are not considered valid. For instance, 1GB address is > > + * 2MB-aligned, however it will not be considered valid for a > > + * requested alignment of 2MB. This is done to reduce coincidental > > + * alignment in the tests. > > + */ > > + if (((unsigned long long) src_addr & (c.src_alignment - 1)) || > > + !((unsigned long long) src_addr & c.src_alignment)) { > > + munmap(src_addr, c.region_size); > > + goto retry; > > + } > > + > > + if (!src_addr) > > + goto error; > > + > > + return src_addr; > > +error: > > + ksft_print_msg("Failed to map source region: %s\n", > > + strerror(errno)); > > + return NULL; > > +} > > + > > A meta thing, but it'd be nice to separate out _moving_ functions into > their own patch to make reviewing easier as here it's not obvious whether Sure, good point and I'll give that a shot. [..] > > +/* > > + * Verify that an mremap aligning down does not destroy > > + * the beginning of the mapping just because the aligned > > + * down address landed on a mapping that maybe does not exist. > > + */ > > +static void mremap_move_1mb_from_start(char pattern_seed) > > +{ > > + char *test_name = "mremap move 1mb from start at 2MB+256KB aligned src"; > > + void *src = NULL, *dest = NULL; > > + int i, success = 1; > > + > > + /* Config to reuse get_source_mapping() to do an aligned mmap. */ > > + struct config c = { > > + .src_alignment = SIZE_MB(1) + SIZE_KB(256), > > + .region_size = SIZE_MB(6) > > + }; > > + > > + src = get_source_mapping(c); > > + if (!src) { > > + success = 0; > > + goto out; > > + } > > + > > + c.src_alignment = SIZE_MB(1) + SIZE_KB(256); > > Why are you setting this again? Yes, I may not need to but since 'c' is passed to a function, I just do it for robustness. If you'd like, I can drop it. > > + dest = get_source_mapping(c); > > + if (!dest) { > > + success = 0; > > + goto out; > > + } > > + > > + /* Set byte pattern for source block. */ > > + srand(pattern_seed); > > + for (i = 0; i < SIZE_MB(2); i++) { > > + ((char *)src)[i] = (char) rand(); > > + } > > + > > + /* > > + * Unmap the beginning of dest so that the aligned address > > + * falls on no mapping. > > + */ > > + munmap(dest, SIZE_MB(1)); > > This actually aligns (no pun intended) with my comments on the min mmap > address + 4 MiB comments previously. But I guess for the generalised mremap > test you will still need to have that headroom... Yes, I guess the situation you are validly concerned about is if someone mapped something just when I unmapped. However, as this is test code it probably should be OK? > > + > > + void *new_ptr = mremap(src + SIZE_MB(1), SIZE_MB(1), SIZE_MB(1), > > + MREMAP_MAYMOVE | MREMAP_FIXED, dest + SIZE_MB(1)); > > + if (new_ptr == MAP_FAILED) { > > + perror("mremap"); > > + success = 0; > > + goto out; > > + } > > + > > + /* Verify byte pattern after remapping */ > > + srand(pattern_seed); > > + for (i = 0; i < SIZE_MB(1); i++) { > > + char c = (char) rand(); > > + > > + if (((char *)src)[i] != c) { > > + ksft_print_msg("Data at src at %d got corrupted due to unrelated mremap\n", > > + i); > > + ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff, > > + ((char *) src)[i] & 0xff); > > + success = 0; > > + } > > + } > > + > > +out: > > + if (src && munmap(src, c.region_size) == -1) > > + perror("munmap src"); > > + > > + if (dest && munmap(dest, c.region_size) == -1) > > + perror("munmap dest"); > > + > > + if (success) > > + ksft_test_result_pass("%s\n", test_name); > > + else > > + ksft_test_result_fail("%s\n", test_name); > > +} > > + > > static void run_mremap_test_case(struct test test_case, int *failures, > > unsigned int threshold_mb, > > unsigned int pattern_seed) > > @@ -565,7 +643,7 @@ int main(int argc, char **argv) > > unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD; > > unsigned int pattern_seed; > > int num_expand_tests = 2; > > - int num_misc_tests = 1; > > + int num_misc_tests = 2; > > struct test test_cases[MAX_TEST] = {}; > > struct test perf_test_cases[MAX_PERF_TEST]; > > int page_size; > > @@ -666,6 +744,7 @@ int main(int argc, char **argv) > > fclose(maps_fp); > > > > mremap_move_within_range(pattern_seed); > > + mremap_move_1mb_from_start(pattern_seed); > > > > if (run_perf_tests) { > > ksft_print_msg("\n%s\n", > > -- > > 2.42.0.rc1.204.g551eb34607-goog > > > > Have you verified this test fails if you eliminate the vma->vm_start != > addr_to_align check? Yes. When Linus found the issue, I wrote this very test to ensure that we caught it and that the fix made the test pass. > Unrelated to this patch, but I guess it might be tricky to come up with a > test for the shift_arg_pages() stack case? Yes I think so, at the moment I am resorting to manually hacking the kernel to test that which is not ideal but.. > Irrespective of the above, this looks correct to me so, > > Reviewed-by: Lorenzo Stoakes > Thanks for putting in so much effort on the testing side too! :) My pleasure, and thanks for the awesome review! I'll get to work on it and post a new version soon. thanks, - Joel