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 0E26DC3DA7A for ; Mon, 2 Jan 2023 15:50:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 84B008E0002; Mon, 2 Jan 2023 10:50:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7F9FC8E0001; Mon, 2 Jan 2023 10:50:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6C26F8E0002; Mon, 2 Jan 2023 10:50:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 5F61E8E0001 for ; Mon, 2 Jan 2023 10:50:02 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 2850380514 for ; Mon, 2 Jan 2023 15:50:02 +0000 (UTC) X-FDA: 80310295044.25.59E854C Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by imf13.hostedemail.com (Postfix) with ESMTP id 713C020006 for ; Mon, 2 Jan 2023 15:50:00 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=aKhKI2lS; spf=pass (imf13.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1672674600; 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=nWZfcM+qssrebEF6M4iZr99iAEIr6Weo0zlGmUVXXu8=; b=DnWjcHRvj3dxqiFkO2FCJFH7VDp/BMXXzwt9GMSEol2MFTvhh+ts6VtIn6jjt82bCnNTGp ZhOsFsHwmIcsCHS8Am8Su7G0n+aTxajTSpgyOqfMGtLOPa7V/GyT9mH//uLVoxx/boT0sG M6g/AAv0RcJh61xNCrho7z9WneFsW2g= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=aKhKI2lS; spf=pass (imf13.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1672674600; a=rsa-sha256; cv=none; b=habG4kL8pJ/+h5Lh7OOejyW4FJ51H4wC5abvjX1q32rngJ/30urKrMBmbalxe2IOrrEMXQ I3D1l27b4ETPpUdqO/nOMnVhFVJXgN5P9EpZinD2RYRqFNKyWnco21p6qb1uGHRsJoeI0A 8hM1E39UTYsI5YUJX65hVDCssa9sq70= Received: by mail-wr1-f48.google.com with SMTP id y8so26611963wrl.13 for ; Mon, 02 Jan 2023 07:50:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=nWZfcM+qssrebEF6M4iZr99iAEIr6Weo0zlGmUVXXu8=; b=aKhKI2lSmAEp3QUkT7GORlxGJ1l8a706SORdJiDWUfxvPW/EG03CpzXGzb7q2sG4dM FfyOloQqevyPCVDKJHuXOlriqAE5nHMAvQStH82vvmP8jRjRCe8fmHcZ70QYBoARLUIK tTgvgimuzgPD7c4R0/cNTrrfFl73FOm/ADUhy3KhIddRXnrTz0jojIpSBCGfeLbshEQy ew9ABndIL1Cl88HRvIF7Rqh28v4Hua7CiNbUuahMdaWWL3B2uX4CgT8UAVALYTFYB49I 80QxDa9pk5zDBpusdCuN2Spj7+8ZZrXj9RT7Mce38GXqsU1n0v5b5uWU8DUcAkV62/tc RDfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=nWZfcM+qssrebEF6M4iZr99iAEIr6Weo0zlGmUVXXu8=; b=3W6rsgHSUX5vG4vYTqzGwqnlVmRuAyy7hM8AEvxVb5clh4ON2SA8Xth9+1ydwxSSDk xk7f84ziBDcFScH31J27A8UIvGYKVVCTjSK+c6jen5ljcFL8e4dQYXcn1NS2l/Kusezu 8i9v1zEa/yU2rNIUqn8q+dg14S0gP8gL9YRmCMdOthTeAKAcEhvjgatu2/NISbUEtjMZ pYzmKFpDg4r6ba6TvtHTvCsLoewpAbCcFB0vd/A82uz/bpGTO7UYlFzea0W9uxIO4FHr Rr2/OoYjkEqg1vG6g3DOsog2s0mMqmSUeTAlvt5IaITEvOriffw9CmAb4vIJnp6bXfyZ 4PRw== X-Gm-Message-State: AFqh2koPm7ASB8L3Gnzhb+xKUCwvbZyeGs11H5xzafXyRW+kRy4QmVL+ W8KW3TVgVRTlaKKNWZ/jVnI= X-Google-Smtp-Source: AMrXdXvf+K2pZoe2fyipfAZGQZ2HEV+p006VOc8BDT93uxeYhjvQ16k0Ja1hBZakggIxAVAM+wHrgQ== X-Received: by 2002:adf:f0ca:0:b0:275:e426:4134 with SMTP id x10-20020adff0ca000000b00275e4264134mr21681792wro.51.1672674598885; Mon, 02 Jan 2023 07:49:58 -0800 (PST) Received: from localhost ([2a01:4c8:469:341:d1e1:a149:58ed:f096]) by smtp.gmail.com with ESMTPSA id f2-20020adfdb42000000b00241d544c9b1sm14497378wrj.90.2023.01.02.07.49.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 02 Jan 2023 07:49:58 -0800 (PST) Date: Mon, 2 Jan 2023 15:49:57 +0000 From: Lorenzo Stoakes To: David Hildenbrand Cc: linux-mm@kvack.org, Andrew Morton , Shuah Khan , linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, Vlastimil Babka , Jakub Matena , Matthew Wilcox , Mel Gorman , Michal Hocko Subject: Re: [PATCH v2] selftest/vm: add mremap expand merge offset test Message-ID: References: <3a4fbe90-b46e-aa49-9866-e2b0cf6de38d@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3a4fbe90-b46e-aa49-9866-e2b0cf6de38d@redhat.com> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 713C020006 X-Stat-Signature: yq78p4ymff73key3o1he5o7xexhz6r5e X-HE-Tag: 1672674600-679102 X-HE-Meta: U2FsdGVkX19d19ubttLzEq72OGm8dRIwo/aMjwMKqn3NDKsYG7nge1ihef/EOw1zWyU3bqI+LwiO+KQFOU9uBZkbT7FHwrMhryDdrNn+Nihh6N5qeZYpGuZSeDms3MK8h816YPpMDtvHVqilEG617WZi/qoQUw5FvfBX09p31aVXZj9TySOQRuTBZsyxGdinIRPv084Uv6KcxcsljgUJvmjWmwXVAV2D/w1GZ+nTkyMEGfRkwT658pRXn1HE6E0P1QrpEOrKkJ2qROjnMvwwB7kqKy5XvxKLrD9X5ngV0ECAd3zgIxUHBozkPAJ5LqnW22LIqG6JfHxv6jLM0XWBToVQk7Y4n68wg4q1NXaXd8PN3TXnIhmBrmKT7OOF1HcyMDUVsM3LaxOJ110oeb2Ufqd9HGxm6InUA7lTob7S1XUwpbNbCKo8rTDMfNKLxTHHpSDo++jgVf3Fb8MWo6FUQMFZhPgo4S8n1ppaTz19LPQZ+P3a8do/OUk+ys0PKFJeea8ljXIVNIaEbEJCjPfACxrCK5aK/XG94iiqDsmPc7OPoSt+Mb4c6b4595QRkaEvkR0jyjAkIDXhQ4e9WRKICsNkonnymQQif4Y5wPoELK/yUaprg+AYGnKyOIuBCOVoZDQvPC/Qo82ie+jlSmiIBZK0H9k5fPzQbNtjRXnI/wJ85qlV8wDhijVUpEgZyuTUx8EYM5yDR9AH0ZOR+N9/tdWTJ2RlGVzBbW8Cil5udTFicr0iHzSYPL16qE/6bPiCTXThvMFdMAW/0edIk1XhCjsOHe6TxjHML2XgD/ZvI7C9kjWrxoullnJBSFUkR05ePdRKis9jjE0dqsVYcyHTdjiz0RuW55I+fU+E4mhqh9x1LoW7hi0wns6w3M2thbPhpYUqwASAPe7I4DvA93dPca1LpIpneFfrfkYl2uqO3yAYjk5KqhR7rdYzxGNjHUWVqRcJkylICqJ6GjX4y0A ysOBF8Zk BJQ/oIChg+WQgfhrj4XtGkSjxO7h7CSkjlVO6JAt59I66gfh0KZdqByGP9jI8QLe42DcSoPf6v7UaRG/yNBV+wIHClg== 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 Mon, Jan 02, 2023 at 04:34:14PM +0100, David Hildenbrand wrote: > On 02.01.23 15:44, Lorenzo Stoakes wrote: > > Add a test to assert that we can mremap() and expand a mapping starting > > from an offset within an existing mapping. We unmap the last page in a 3 > > page mapping to ensure that the remap should always succeed, before > > remapping from the 2nd page. > > > > This is additionally a regression test for the issue solved in "mm, mremap: > > fix mremap() expanding vma with addr inside vma" and confirmed to fail > > prior to the change and pass after it. > > > > Finally, this patch updates the existing mremap expand merge test to check > > error conditions and reduce code duplication between the two tests. > > > > Signed-off-by: Lorenzo Stoakes > > --- > > tools/testing/selftests/vm/mremap_test.c | 115 ++++++++++++++++++----- > > 1 file changed, 93 insertions(+), 22 deletions(-) > > > > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c > > > ... > > > + > > + start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE, > > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > > + > > + if (start == MAP_FAILED) { > > + ksft_print_msg("mmap failed: %s\n", strerror(errno)); > > I'd > > ksft_test_result_fail(...) > return; > > > + goto out; > > + } > > + > > + munmap(start + page_size, page_size); > > + remap = mremap(start, page_size, 2 * page_size, 0); > > + if (remap == MAP_FAILED) { > > + ksft_print_msg("mremap failed: %s\n", strerror(errno)); > > + munmap(start, page_size); > > + munmap(start + 2 * page_size, page_size); > > + goto out; > > dito > > ksft_test_result_fail(...) > ... > return; > > > + } > > + > > + success = is_range_mapped(maps_fp, start, start + 3 * page_size); > > + munmap(start, 3 * page_size); > > + > > +out: > > then you can drop the out label. > I have to disagree on this, to be consistent with the other tests the failure messages should include the test name, and putting the ksft_test_result_fail("test name") in each branch as well as the error message would just be wilful duplication. I do think it's a pity C lacks mechanisms such that gotos are sometimes necessary, but I can only right so many wrongs in this patch :) > > + if (success) > > + ksft_test_result_pass("%s\n", test_name); > > + else > > + ksft_test_result_fail("%s\n", test_name); > > +} > > + > > +/* > > + * Similar to mremap_expand_merge() except instead of removing the middle page, > > + * we remove the last then attempt to remap offset from the second page. This > > + * should result in the mapping being restored to its former state. > > + */ > > +static void mremap_expand_merge_offset(FILE *maps_fp, unsigned long page_size) > > +{ > > + > > + char *test_name = "mremap expand merge offset"; > > + bool success = false; > > + char *remap, *start; > > + > > + start = mmap(NULL, 3 * page_size, PROT_READ | PROT_WRITE, > > + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); > > + > > + if (start == MAP_FAILED) { > > + ksft_print_msg("mmap failed: %s\n", strerror(errno)); > > + goto out; > > + } > > + > > + /* Unmap final page to ensure we have space to expand. */ > > + munmap(start + 2 * page_size, page_size); > > + remap = mremap(start + page_size, page_size, 2 * page_size, 0); > > + if (remap == MAP_FAILED) { > > + ksft_print_msg("mremap failed: %s\n", strerror(errno)); > > + munmap(start, 2 * page_size); > > + goto out; > > + } > > + > > + success = is_range_mapped(maps_fp, start, start + 3 * page_size); > > + munmap(start, 3 * page_size); > > + > > +out: > > dito. > > > if (success) > > ksft_test_result_pass("%s\n", test_name); > > else > > ksft_test_result_fail("%s\n", test_name); > > - fclose(fp); > > } > > /* > > @@ -385,6 +447,7 @@ int main(int argc, char **argv) > > struct test perf_test_cases[MAX_PERF_TEST]; > > int page_size; > > time_t t; > > + FILE *maps_fp; > > I'd simply use a global variable, same applies for page_size. But passing it > around is also ok. > I am trying to keep things consistent with what's gone before in this code, and given page_size is being passed around I think the 'when in Rome' principle applies equally to passing the fp around I think. > > pattern_seed = (unsigned int) time(&t); > > @@ -458,7 +521,15 @@ int main(int argc, char **argv) > > run_mremap_test_case(test_cases[i], &failures, threshold_mb, > > pattern_seed); > > - mremap_expand_merge(page_size); > > + maps_fp = fopen("/proc/self/maps", "r"); > > + if (maps_fp == NULL) { > > + ksft_print_msg("Failed to read /proc/self/maps: %s\n", strerror(errno)); > > Maybe simply fail the test completely and return -errno ? > Ack on this one, will spin a v3 with this as otherwise we might miss test failures here. > > + } else { > > + mremap_expand_merge(maps_fp, page_size); > > + mremap_expand_merge_offset(maps_fp, page_size); > > + > > + fclose(maps_fp); > > No need to fclose, just keep it open ... > I'd rather we did fclose() to keep things clean, as who knows what additional tests may be added later and to be pedantic about matching an fopen() with an fclose(). > > + } > > if (run_perf_tests) { > > ksft_print_msg("\n%s\n", > > > Acked-by: David Hildenbrand > Thanks! > -- > Thanks, > > David / dhildenb >