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 38F92C83F15 for ; Sun, 27 Aug 2023 10:12:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9C993900007; Sun, 27 Aug 2023 06:12:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 97ABC8E0001; Sun, 27 Aug 2023 06:12:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8413B900007; Sun, 27 Aug 2023 06:12:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 745568E0001 for ; Sun, 27 Aug 2023 06:12:20 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 446DDC018F for ; Sun, 27 Aug 2023 10:12:20 +0000 (UTC) X-FDA: 81169469640.30.B620EA8 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by imf23.hostedemail.com (Postfix) with ESMTP id 707ED140017 for ; Sun, 27 Aug 2023 10:12:18 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=pPur96SO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1693131138; 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=eGly6+ZahROEoc3sjOB5m3qZerStQgRkheVYTMYF2fg=; b=HPGsOkqnaDq1dQF1nxYVeXaqa/HemRFgJCTk2YFl7N8sBQpe2EQikRBGnXqLBeY9iY4Zyn kt2FyU2XD4XridLo8bLLeHFaz9FwE9T1+XYOV6eYzBIiVr8/ZBx1DvDzRPPRI4Ppn8xiSu xZ3v9SjZMWugamyuF0sQB8J7Uo7TTxQ= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=pPur96SO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.44 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1693131138; a=rsa-sha256; cv=none; b=tdPr8KWR3Xr8VDFZjWE9MWndZLusWPkwP6k87nUt1pM1Y0Cdd6eZlfeo+0v7B8WSH+fhiq yZZeSzMtP/htgW3ZzMZ3XF+CU+6H2VF+9KZKUrT1ewvGjheN+8CJEkE1txl7UIisrb2IJS nJ836lKY/yZvh+OImIYFDPqUS5UYEEo= Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-3197b461bb5so1913835f8f.3 for ; Sun, 27 Aug 2023 03:12:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1693131137; x=1693735937; 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=eGly6+ZahROEoc3sjOB5m3qZerStQgRkheVYTMYF2fg=; b=pPur96SOWmQ1D8mFh1zIRe6H6sgtHlFCwq2oR6H8RCpxbY+k6I3EMixq/iTTq9OGqR ndvGDSYGdm1jIWD0e3G5Y6Oxf1Dh390NJE35sVp/67PI5TOc7Gwq2+NmLaOH7at4rVyF 8MGlRWOwGvTPMdNirIf6m1+T7Gd+HpNW6Xv/810GT4+CkdgicBW00IEmQneaIaHtINXz FJZYjBFy8RVDvom82WGtP0z0TSIvh1sebPb1rS0Tvk8HfBPU+F/aDuRe5IlE6J6LbIJ7 O5J9yCvEFqY1ul5TOk2IbhryA2fJ6TJQWUj6nzgguEGIdTYsPLDDu89+goNjPvEL33K2 0MQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693131137; x=1693735937; 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=eGly6+ZahROEoc3sjOB5m3qZerStQgRkheVYTMYF2fg=; b=FCmo0Prf8UJoU69/NnrWO19EFkHJNJ8biYm4a3x1ncmdTmtCi58tZOPKpEKHjoZoM5 T7Tffn0KA402DF+tmIXauOVwxfMbFg3YVJZgYtdpZ2pPKq0vBWt09R3AVZdR8YZGQypb Qd9AMY+3kRLivCvGUeJREBDOHcoj1bJoq+tUG/YD6CJKEX2UwGQCWSN8IJsePn8aS5kM BwEIv8AUqDgXOfCvKr/SCrVppCtGNkwtg7ToaqUeOdcdIxNjy7tCxldOunF2hA0khrRh c7vKdbFP1c0kd6Rf++AeG3Zgeh+IXouXlp2cpeoK2oaVOhsCyhzs2ec1gREnkPlBhk9u t1iQ== X-Gm-Message-State: AOJu0Yzthq3UV4fCIKcKByv83gQSjmo4A6v2aVKlK5V6gArxK/DA3BYW DJLr/0xUvtglV3eM/IhuBCzM7E+cyBM= X-Google-Smtp-Source: AGHT+IHimX1d0lfBUEDephlJBGab5Vw/5YLovsV3UoHJbwnvw1cSND9Pt9rRGAsMioD8VmEqkxf/1g== X-Received: by 2002:adf:ded1:0:b0:313:f395:f5a3 with SMTP id i17-20020adfded1000000b00313f395f5a3mr17723051wrn.38.1693131136754; Sun, 27 Aug 2023 03:12:16 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id r9-20020adff709000000b0031ad5a54bedsm7266756wrp.9.2023.08.27.03.12.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 27 Aug 2023 03:12:15 -0700 (PDT) Date: Sun, 27 Aug 2023 11:12:14 +0100 From: Lorenzo Stoakes To: "Joel Fernandes (Google)" 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: <18a2c79d-3feb-41aa-93a9-bafbfb188cbc@lucifer.local> References: <20230822015501.791637-1-joel@joelfernandes.org> <20230822015501.791637-8-joel@joelfernandes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230822015501.791637-8-joel@joelfernandes.org> X-Rspamd-Queue-Id: 707ED140017 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: kj1owcpeb9jj6prbo8w6cpsredwakrdy X-HE-Tag: 1693131138-601333 X-HE-Meta: U2FsdGVkX18ZJq61KCrsGQgAJR462ui/mCxqJcMoiURW/xP8+jkmHB51e9mwQbUFMSCjup2Q7JA461107RA4zyMBywu7k4W3YFv4cRcDh8WLGsk2TABUk8v9/KHZe2Mtb8cx9mZU7t+qmqdux3PMc4TEkXLeIyeGA+zz1CtbDOFEVybBoaiUJPTnCuKVpFU1GFAK4HgX+YdFUqZdRpFRhcAKYdTAKr43WHxTxckhkWinHNT4+/yoeHg57jWQEl9rNONgbRwtFE4DibNfV9XXjV6aQAi85fPLvOA+MoZ+cv25Ye2ibO92hJC74FI5BttHVq1ysbkozcy7MSZ04oUNUMaZNJp/RpbPg3KCch+Sv/EUhQjD0rv8OTHO6b2CyJ6gEAMvdU7khBKgtxmk3kwevy719P7kNl6GSGLZbmrgMw6bMkEeusO2t/vZwEzOMBFyTunM+dyBmzQX8c3LPaSNFzxY9HdZjfsyOyNY5o2Ws/o7exiesJht89Wlgea8bZiVOmuoGNvudcsxx9rXOzUXrABjSn7peG8sVWxzpr8mTzogdiI0fVA9mofdp7A2/iZ/5iprg+diY11VtAwVieaSJYVH/M/o85tYq8b4eEiNbbwlMjvfDNz2Dc+MIoMwn1jqunzEIgNJw1gXnQXGx0cWavAyUiERkD4tJcu+JoXR6p+nubFqemYCPCwbmsjknlteNbgsRmmBQVp9m8oFbZVYhwp0GLXP32DA3duXkAw2y/qjvUoK5wfhLCLFEvzVsLnR686sVOJwH5XklLOQYQ38ikkrX4LPsUm2vS/5zzyYmKyG5LOsjwV/3UTjQgt8W94UdAOZaMlAJN/OrjN5kiiQm584923pJ5nKUC+8brOgmlcUvc2emzeIYqOoiEepB5x6BJ4RKH/PctwhzlodgEYqYdOa5gxux1lZjMbR/fovVglJyTSdrzDAr3KTWUSFVKBjzK6Ws4DJcBlKZXVx7lt MspW0VmS Oa4JMbg6i4uPyjMpLK2Ay0/3Jz1crfnTMJYxFJGiAXdvqW+ZMx8J+j+e6jm8RV8f5IMlWHZyFO8uG1W8JPWUbU7v37VpeNMSOSP8wxCS87049NxEzQiWKxEIliDwRD2Ku9lS0CLjtqepzwtV7z45pQNrSrYDjsVRuq4iQREoyAw/dRiS/S/QBCqH5B2Qg9tHXKVkY985wEUP/poDokPuwhQHJX5X6VcBdqV62SJuhfkOhKtTtsZusaT2015rlnaKBRKK6mxRAxVw9lfj2E4OovLemDI5MOUGxZ8POPS0pgnD4Qy7/LNZdRKbIGYG2ifJd9mk5Bhxs//7zVtJi960SWxMqzJPXgicD1Nlh/Hvh2l9lxQnkDq2Ob1psiof1qPjiCGzyHULrMiI9FiXhvs1uhpwCv/JzgXAcw79AJKGIp80Hrw+u9cPWF6HOTLGuVZ0V9iaG+zsmt1tkfmSQuzbJinPbFVKsbwnC94raZOcLxKodxZH0rO4P8VqyevhEE3p7KIfg0Ehh381xO88O5do4x0pT5QJwEg9AbCcWnWQoaAOOk0w= 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 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. > > 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 you changed anything or not (I eyeballed and seems like you didn't though!) > /* > * This test validates that merge is called when expanding a mapping. > * Mapping containing three pages is created, middle page is unmapped > @@ -300,60 +355,6 @@ static void mremap_move_within_range(char pattern_seed) > ksft_test_result_fail("%s\n", test_name); > } > > -/* > - * 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; > -} > - > /* Returns the time taken for the remap on success else returns -1. */ > static long long remap_region(struct config c, unsigned int threshold_mb, > char pattern_seed) > @@ -487,6 +488,83 @@ static long long remap_region(struct config c, unsigned int threshold_mb, > return ret; > } > > +/* > + * 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? > + 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... > + > + 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? Unrelated to this patch, but I guess it might be tricky to come up with a test for the shift_arg_pages() stack case? 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! :)