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 D624AC4345F for ; Sun, 28 Apr 2024 12:36:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 323886B007B; Sun, 28 Apr 2024 08:36:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2D42E6B0083; Sun, 28 Apr 2024 08:36:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1747F6B0085; Sun, 28 Apr 2024 08:36:38 -0400 (EDT) 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 ECD576B007B for ; Sun, 28 Apr 2024 08:36:37 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 69138160F09 for ; Sun, 28 Apr 2024 12:36:37 +0000 (UTC) X-FDA: 82058889234.21.3678806 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by imf15.hostedemail.com (Postfix) with ESMTP id 98BA1A0016 for ; Sun, 28 Apr 2024 12:36:35 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="Ni2/HtSJ"; spf=pass (imf15.hostedemail.com: domain of richard.weiyang@gmail.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=richard.weiyang@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714307795; a=rsa-sha256; cv=none; b=Jmp5yYN9QHCKijEh+rftefOe3fl9sZxmG36B15sPf0+tt0RfbzcsXG+eG94UclPMJWdTNx hQsa5QZ8V+yM3VIoS5q+142Gc4ss2qu5s0MX3GD+5CspSd1WkJ2R94BjE5TFUi0ROj0/LC IDfVqqbQMpVshh9SJS/s/ROuX1snhWQ= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="Ni2/HtSJ"; spf=pass (imf15.hostedemail.com: domain of richard.weiyang@gmail.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=richard.weiyang@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=1714307795; h=from:from:sender:reply-to: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=JBkmSalA3YkFFsDksGLJHV8MYuAh/RfQtgJWHxm4KjI=; b=jcpAk1QKPQ2E5O0gVZKnAKKy54evVLZ2hy94nUA0/kLDT0Eo/iQW6yH6Wb/ioJDpNcafKz 4HfwI9Hbc+8YXUnQKqMsiaOPzpHXRv75vOMKlqemK2QHjTAkDWYKwqx0ST5koI21V3M5C4 PyrYm+qfHFeVjFFMRKiS0tdvCMuewHM= Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-56e37503115so3716048a12.1 for ; Sun, 28 Apr 2024 05:36:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714307794; x=1714912594; darn=kvack.org; h=user-agent:in-reply-to:content-disposition:mime-version:references :reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=JBkmSalA3YkFFsDksGLJHV8MYuAh/RfQtgJWHxm4KjI=; b=Ni2/HtSJ+MlnoOzH5b0SLLYbfQsN3UZwBh04C5MTXIvRKJoO7gGRy77Y+mrtIGwrL4 j3vQP55fHnk5/4UeCLMr/1T4bI8Slaf0bfFqBLq5H/8wbSWL3CHayyayqBqapbfhozaS +Tx6iKAMtwmljXkflU0OBahLLzc9ZBzMxE5ku++/Bz4Mpa7hIDcOJytbfqV+reSJcQHL 64XyrfojUBubDY+olHUcmMk6d/ESRjjapHfPb5DXddk8VoUeKOHUXFZ2jGkZLQakSqow oG1UMLUJ7O+X1JQ7Ne+mjwbMwphYpuhS5I6+O81K5DmAS8st/ZK7P6RHbZekYt7TvD4s 5wCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714307794; x=1714912594; h=user-agent:in-reply-to:content-disposition:mime-version:references :reply-to:message-id:subject:cc:to:from:date:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=JBkmSalA3YkFFsDksGLJHV8MYuAh/RfQtgJWHxm4KjI=; b=roLsk4rZzucMpSIQ9wfmLxCuiqWkQ55mPdNRrVG1UIcqmSrP7+tStyGhtr+/ggFnEv K0I6pqT7Fp4DPA9+mNcdQ9JGiJC2WOor9eMU8XKsyVFzXB4JdUUwp1TmFckqAeBBWiez iW0R+EuWeRc5HHtZ12XqsY7sLZxbO1Nci5IdLup8pc9oAYcxwHlmhnbtIMOt1Fq5ARma Gd0InUIimW9rebmwg1O4QkAYpqAQXya4e6R9ifFYjqUT7uU9ZwW/8qTv7SLtmHQDhA6v tKa7mUkgWpYnRRY9FkPJIuKoshd9JAlFuu91ln5mcOHm7dKmiHYOrQYvLpy0uNmAhNwv u2WA== X-Forwarded-Encrypted: i=1; AJvYcCWdyUfksPyKntN7BQNbBT19tl2jSFYP7i1+++8bMbeXFXWgZ74gS31HsKRQD6bLFrrahSPxjfcu52OtLrIjKKBSfyQ= X-Gm-Message-State: AOJu0Yx3TJa9voFAvRNxdpY0+jhmDdIvm+9BA319TEoc9+2ZEgv8tPf4 sUW6iNYIet2V0HzwgoXLmlYn9QkPhLv6WEtniP30wFNJmjKbl2L7JiLdGKoX X-Google-Smtp-Source: AGHT+IHxJG23qA/xqe3b5jANnPyb+8/KmT74Wqpo+wZrstU3McQq5hwZO1mG+YyuY0FhyggZ2gU+tw== X-Received: by 2002:a17:906:258a:b0:a58:e86d:40e6 with SMTP id m10-20020a170906258a00b00a58e86d40e6mr2473499ejb.26.1714307793760; Sun, 28 Apr 2024 05:36:33 -0700 (PDT) Received: from localhost ([185.92.221.13]) by smtp.gmail.com with ESMTPSA id qs29-20020a170906459d00b00a5197fa2970sm12741513ejc.25.2024.04.28.05.36.32 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Sun, 28 Apr 2024 05:36:32 -0700 (PDT) Date: Sun, 28 Apr 2024 12:36:31 +0000 From: Wei Yang To: Mike Rapoport Cc: Wei Yang , akpm@linux-foundation.org, linux-mm@kvack.org Subject: Re: [Patch v2 2/8] memblock tests: add memblock_reserve_many_may_conflict_check() Message-ID: <20240428123631.fbzq2akxw6p722ig@master> Reply-To: Wei Yang References: <20240425071929.18004-1-richard.weiyang@gmail.com> <20240425071929.18004-3-richard.weiyang@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) X-Stat-Signature: 3wssy6s5okk8kfrh1c7g9qtxqhqc7qb4 X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 98BA1A0016 X-HE-Tag: 1714307795-746219 X-HE-Meta: U2FsdGVkX18SxtYCyjFGFpoSQzVMG3UlBdNhjdWR71VMEJLdkLYllkIxR32Z9FV3kiSxErnF6fhGoDrlCrmoPNXZAGnCxnwe0QqWQKwnPlA9Vlkrg/dfTRwF5pCTXwKWww/mtA6TRviikaK/VDiQh+p/RNR4KmfqihXcw2nYbq8RQk0kdIOCcLLv3hmkWFaPMPQCxu9LNR2k/5wCXMZA5iO4HZL18pKm/eIvHhNiSMQV1C80bUTvzVXpclTBdDtD0js0HXqWVdWFVo/drpw4n9Kq5uwlFOgEQix7J3d6lYyCY+P+mJZNFxyLazox0k8/gPR+MPpVkz8pymZ5pGBNIHPoLXXByiwSWTmhpK3eliD04lrHm3Pa5anPutX4k+p+jdMZATEdfWGi0kCwTj/ohfGMR8n2Vk1OEg9+aYQeBPKpuXyEuJKKtJhuvl3x6GglA+H/d4OYulEqSrrBeT5hoVV3eSxxrttHaxnxvcwbVBGspGVqdz6L7K3Bl2uAoa9RHPqJzoE8sHb+yudWTyCUQOqWSFwGVeRDXe+SJu60YyWsi67rAm+bma9Ej18dhcufX1wnBfGUvdzTpZtQ0uZoQiBDSjyeTGL/Pl4zKT7q2wjgvLNGc3g6NXmWEtpIYRBMnPwCPuINVZWpr6RCeRBSox5Fep2RBoeZgDcepNkgFak85YlThRmBhyHHbimbkkzqmWTMC+/XXxdQmX9GiCG8CL2SNqHFxHuDawjgL5PBg6HsdkAAHEGeu4edI8czKPaDL51LA3KMUk1cwceKe8zBRp9I+9tGLTrFcd3ylwvr80L4w35csIIXQdpt5GNJP4SX5QJqQCfdrf+JGrJ3ebcgSwbCa9e0WQ2Jqf54bnG0ZxZl7hnJv0atsP4udu0kNNgm//T2TQkIxAd8oNZSplai4kV6vqFPWd9+iUyZE/0CYmg03AxWjeqHFi+Js2MkqNhRT9uTQnxH20CrufMfH2t xS+x41v5 ExeQ2qnXJEVgPkzSFqpZUsqYBWzR7ceed/oBb0myodiIzqzNQ4xFVIJhOxqIAwFulEpZ04BjLswZSQ81aLRVYJZBPSQpG5VwoDKIlinOMTK6CniaVbE9XhdV6U+hfisjkg7Fi43tjTGOgt+tz6jQYzODbjYxcH/mpo5tbaMK765hW1eD1owRbIiRmHOUFGCinliUwnMZhhSZ4VxbzEoKJ5SFa1THGUIehYfG3X+YZSmZh5dgkFEvgQgZxdyKaLuJKUgQHprMTAmPCA5fC30O/UPV3ieFEMqEWOcuTH269KsSx1KNUXBp9+Eem7UXgTLhWOe94 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: List-Subscribe: List-Unsubscribe: On Sun, Apr 28, 2024 at 09:40:33AM +0300, Mike Rapoport wrote: >On Thu, Apr 25, 2024 at 07:19:23AM +0000, Wei Yang wrote: >> This may trigger the case fixed by commit 48c3b583bbdd ("mm/memblock: >> fix overlapping allocation when doubling reserved array"). >> >> This is done by adding the 129th reserve region into memblock.memory. If >> memblock_double_array() use this reserve region as new array, it fails. >> >> Signed-off-by: Wei Yang >> --- >> tools/testing/memblock/tests/basic_api.c | 123 +++++++++++++++++++++++ >> tools/testing/memblock/tests/common.c | 4 +- >> tools/testing/memblock/tests/common.h | 1 + >> 3 files changed, 126 insertions(+), 2 deletions(-) >> >> diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c >> index 1ae62272867a..748950e02589 100644 >> --- a/tools/testing/memblock/tests/basic_api.c >> +++ b/tools/testing/memblock/tests/basic_api.c >> @@ -991,6 +991,128 @@ static int memblock_reserve_many_check(void) >> return 0; >> } >> >> +/* Keep the gap so these memory region will not be merged. */ > >The gap where? What regions should not be merged? We would reserve range [1-129] like below. The gap is between each range, which is 32K, to prevent merge during memblock_reserve(). 0 1 129 +---+ +---+ +---+ |32K| |32K| .. |32K| +---+ +---+ +---+ |gap | |<-->| >Also please add a comment with the test description Sure. > >> +#define MEMORY_BASE_OFFSET(idx, offset) ((offset) + (MEM_SIZE * 2) * (idx)) >> +static int memblock_reserve_many_may_conflict_check(void) >> +{ >> + int i, skip; >> + void *orig_region; >> + struct region r = { >> + .base = SZ_16K, >> + .size = SZ_16K, >> + }; >> + phys_addr_t new_reserved_regions_size; >> + >> + /* >> + * 0 1 129 >> + * +---+ +---+ +---+ >> + * |32K| |32K| .. |32K| >> + * +---+ +---+ +---+ >> + * >> + * Pre-allocate the range for 129 memory block + one range for double >> + * memblock.reserved.regions at idx 0. >> + * See commit 48c3b583bbdd ("mm/memblock: fix overlapping allocation >> + * when doubling reserved array") > >Sorry, but I'm failing to parse it > >> + */ >> + dummy_physical_memory_init(); >> + phys_addr_t memory_base = dummy_physical_memory_base(); >> + phys_addr_t offset = PAGE_ALIGN(memory_base); >> + >> + PREFIX_PUSH(); >> + >> + /* Reserve the 129th memory block for all possible positions*/ >> + for (skip = 1; skip <= INIT_MEMBLOCK_REGIONS + 1; skip++) { >> + reset_memblock_regions(); >> + memblock_allow_resize(); >> + >> + reset_memblock_attributes(); >> + /* Add a valid memory region used by double_array(). */ >> + memblock_add(MEMORY_BASE_OFFSET(0, offset), MEM_SIZE); >> + /* >> + * Add a memory region which will be reserved as 129th memory >> + * region. This is not expected to be used by double_array(). >> + */ >> + memblock_add(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); >> + >> + for (i = 1; i <= INIT_MEMBLOCK_REGIONS + 1; i++) { >> + if (i == skip) >> + continue; >> + >> + /* Reserve some fakes memory region to fulfill the memblock. */ >> + memblock_reserve(MEMORY_BASE_OFFSET(i, offset), MEM_SIZE); >> + >> + if (i < skip) { >> + ASSERT_EQ(memblock.reserved.cnt, i); >> + ASSERT_EQ(memblock.reserved.total_size, i * MEM_SIZE); >> + } else { >> + ASSERT_EQ(memblock.reserved.cnt, i - 1); >> + ASSERT_EQ(memblock.reserved.total_size, (i - 1) * MEM_SIZE); >> + } >> + } >> + >> + orig_region = memblock.reserved.regions; >> + >> + /* This reserve the 129 memory_region, and makes it double array. */ >> + memblock_reserve(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); >> + >> + /* >> + * This is the memory region size used by the doubled reserved.regions, >> + * and it has been reserved due to it has been used. The size is used to >> + * calculate the total_size that the memblock.reserved have now. >> + */ >> + new_reserved_regions_size = PAGE_ALIGN((INIT_MEMBLOCK_REGIONS * 2) * >> + sizeof(struct memblock_region)); >> + /* >> + * The double_array() will find a free memory region as the new >> + * reserved.regions, and the used memory region will be reserved, so >> + * there will be one more region exist in the reserved memblock. And the >> + * one more reserved region's size is new_reserved_regions_size. >> + */ >> + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 2); >> + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + >> + new_reserved_regions_size); >> + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); >> + >> + /* >> + * The first reserved region is allocated for double array >> + * with the size of new_reserved_regions_size and the base to be >> + * MEMORY_BASE_OFFSET(0, offset) + SZ_32K - new_reserved_regions_size >> + */ >> + ASSERT_EQ(memblock.reserved.regions[0].base + memblock.reserved.regions[0].size, >> + MEMORY_BASE_OFFSET(0, offset) + SZ_32K); >> + ASSERT_EQ(memblock.reserved.regions[0].size, new_reserved_regions_size); >> + >> + /* >> + * Now memblock_double_array() works fine. Let's check after the >> + * double_array(), the memblock_reserve() still works as normal. >> + */ >> + memblock_reserve(r.base, r.size); >> + ASSERT_EQ(memblock.reserved.regions[0].base, r.base); >> + ASSERT_EQ(memblock.reserved.regions[0].size, r.size); >> + >> + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 3); >> + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + >> + new_reserved_regions_size + >> + r.size); >> + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); >> + >> + /* >> + * The current reserved.regions is occupying a range of memory that >> + * allocated from dummy_physical_memory_init(). After free the memory, >> + * we must not use it. So restore the origin memory region to make sure >> + * the tests can run as normal and not affected by the double array. >> + */ >> + memblock.reserved.regions = orig_region; >> + memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS; >> + } >> + >> + dummy_physical_memory_cleanup(); >> + >> + test_pass_pop(); >> + >> + return 0; >> +} >> + >> static int memblock_reserve_checks(void) >> { >> prefix_reset(); >> @@ -1006,6 +1128,7 @@ static int memblock_reserve_checks(void) >> memblock_reserve_between_check(); >> memblock_reserve_near_max_check(); >> memblock_reserve_many_check(); >> + memblock_reserve_many_may_conflict_check(); >> >> prefix_pop(); >> >> diff --git a/tools/testing/memblock/tests/common.c b/tools/testing/memblock/tests/common.c >> index c2c569f12178..5633ffc5aaa7 100644 >> --- a/tools/testing/memblock/tests/common.c >> +++ b/tools/testing/memblock/tests/common.c >> @@ -61,7 +61,7 @@ void reset_memblock_attributes(void) >> >> static inline void fill_memblock(void) >> { >> - memset(memory_block.base, 1, MEM_SIZE); >> + memset(memory_block.base, 1, MEM_ALLOC_SIZE); > >I believe PHYS_MEM_SIZE is a better name. > Sounds better. >> } >> >> void setup_memblock(void) >> @@ -103,7 +103,7 @@ void setup_numa_memblock(const unsigned int node_fracs[]) >> >> void dummy_physical_memory_init(void) >> { >> - memory_block.base = malloc(MEM_SIZE); >> + memory_block.base = malloc(MEM_ALLOC_SIZE); >> assert(memory_block.base); >> fill_memblock(); >> } >> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h >> index b5ec59aa62d7..741d57315ba6 100644 >> --- a/tools/testing/memblock/tests/common.h >> +++ b/tools/testing/memblock/tests/common.h >> @@ -12,6 +12,7 @@ >> #include <../selftests/kselftest.h> >> >> #define MEM_SIZE SZ_32K >> +#define MEM_ALLOC_SIZE SZ_16M > >Do we really need 16M? Wouldn't one or two suffice? > 0 1 129 +---+ +---+ +---+ |32K| |32K| .. |32K| +---+ +---+ +---+ This is the range we are manipulating, which is roughly 130 * 64K = 128 * 64K + 128K = 8M + 128K So I choose the next power of 2, 16M. Since we insert the 129th range at all possible position, we may allocate the double array at any place when the bug is triggered. So we need to allocate the whole range instead of just allocate range 0 as we expect. >> #define NUMA_NODES 8 >> >> #define INIT_MEMBLOCK_REGIONS 128 >> -- >> 2.34.1 >> > >-- >Sincerely yours, >Mike. -- Wei Yang Help you, Help me