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 0B15FE9270B for ; Thu, 5 Oct 2023 15:20:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 54EA26B0206; Thu, 5 Oct 2023 11:20:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4FEA46B020A; Thu, 5 Oct 2023 11:20:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3C5D66B0246; Thu, 5 Oct 2023 11:20:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 276686B0206 for ; Thu, 5 Oct 2023 11:20:33 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 0403A40299 for ; Thu, 5 Oct 2023 15:20:32 +0000 (UTC) X-FDA: 81311769546.27.BC70F33 Received: from out-193.mta0.migadu.com (out-193.mta0.migadu.com [91.218.175.193]) by imf28.hostedemail.com (Postfix) with ESMTP id 8DD8FC001F for ; Thu, 5 Oct 2023 15:20:29 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="lI/Q7NgE"; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf28.hostedemail.com: domain of yajun.deng@linux.dev designates 91.218.175.193 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1696519229; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9SEe3HVAQ2UvwZ+ljN1AmavJckeuvuc0VD534RjDCa8=; b=cG34LdFKNX8/ZatFVEgHCzDY+Dh334cAFNcUzkvqtiQRSZcTHZoDS07yDQCA55U+idOO/5 ynzkRX2bGeiQNIegvSBHEV9H8XRKV62NKwln+RMIZMOymOF2lMgunij1tD4VYgC+LR4x4m ernOJdcn8tNSqG3OOsMwIspuTE2H4D4= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="lI/Q7NgE"; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf28.hostedemail.com: domain of yajun.deng@linux.dev designates 91.218.175.193 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1696519229; a=rsa-sha256; cv=none; b=wB1vXmOzSExiHhY9S5kHKfDMyLlp5e2H3G92mqf+Bbtn7aLg8hvJKkAyhxJVC2vk4QA5Z8 iUHPKChkeS/GN9xZ/KOzfknWaddSiiJd+fWV3O35QSBWxv4CmdoyX3TbCpGEbMRGYp6vu9 u6QgOMMdg7+YMAhh8eWYNn4hy+V7FcE= Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1696519227; h=from:from: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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9SEe3HVAQ2UvwZ+ljN1AmavJckeuvuc0VD534RjDCa8=; b=lI/Q7NgEe2Y2v1aZFKAhWvOA8+ffjVJKk1bTipynNIDabpi67ioqySJBKcgQnM8NV3hxyU GWIAVllejDcvND0Ol/cYDTZ2ZZh0RkLn4rUUOnjJ71QrwMpXtcdNjCmHXN6hxyoXtSq6/1 civDHVPzTAjqSWZHK9i4b53yYBNKOGQ= Date: Thu, 5 Oct 2023 23:20:19 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v3] memblock: don't run loop in memblock_add_range() twice Content-Language: en-US To: Mike Rapoport Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20231003163045.191184-1-yajun.deng@linux.dev> <20231005051959.GC3303@kernel.org> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yajun Deng In-Reply-To: <20231005051959.GC3303@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 8DD8FC001F X-Stat-Signature: auas77bhfp1ukwnmd4dnwegy8tti7bpe X-Rspam-User: X-HE-Tag: 1696519229-899927 X-HE-Meta: U2FsdGVkX1/NKzc7Z1VrRhUJ9PgzStHDrhp4Z0D/gim67AIEeo9M7RtpfWxjRMekg9clfaIDg/WLTpFRQYrHiSSRNHTfvquroSkChIjFcWq+oC12d1Lcjy+bYtErS6DNsUhksiuZVlkJG4BgWfzHtfInkzKRKT5F6HO47NNpXVTluBNBBB+RB7mU1XhDcssJ3BWUtyOqyCha7TVJJPm6RFhy+63nH2sJyqEPe+Oeww4RkAxLOA4CCdgt0WWQ+lR5SADz/3FIGmkAn/13P8t/yDbt6E5QfTd5ibQuLGrMiy3wv6Xxy8Y0OAe/0TFf8DaY71VQwGXNZbQnGBMPS1tQpFbchZ5y9iEW4OPumII/pJsQ3PVxF1PZFUvFAbpLSc6W3q7imLFRXBWKrnVuBMCkXCJ+9gdlTOsxig+94rLmxQvvYTVrXPX2rqT6BDAMZ1uYg7y8ndx0tgDcT2sikLAO69hQLsOnmpdgxFXlq/LxnOVZC3fbtaUW7SwYN50z+GVGsDz3SNr9WWSNarO5fmuMYt3RrwbkvQWJnonyTevtiPq8LiESmwZ64VL4jGfeq8bX1M0AE54bSQkOses632U1I9Fk7IqX+sPvvDtfuK0JhwbQerH2aK+ctfX70W1J1LpFfjopHfFze/y4s4mMo5DyDz85Yr6yNlm4CaEqI6pK3dvqp96m7BxDROu6tw/CYmghHsgEhMA82vpl2gJV6m2QNc9J2NrK4YL2RlwFGxInYkOrNFfHq2YxbYWpgnjdsPDEA+d8E+LadRuJdgQoYvoZXsVADM+gIA8rdZ8ksmbR0zfybga0flROay8yP8NvUf6XYRkPFEaqyCU5fxe9Fv1AdjaNKINHAB2W0PD4q6EYrTWDtysAsSmRSvM7fOGfeRm6NO4UNOsSt2RG5R7u61DW/bP8qdTnhrwQBljkbsqi9gAhmLOZg+o4KaipZCksifsGPia0mFqwhr68yCNcC0f Zmjx83NP z4HEinVJQ9bdZd/fbWUQTyYPfRXoncn+PR8HPIrtP1BkLG8D/rRhwhxA3oBgWFd0HmS6X2gdBU6oXosPEZSCHFN7TMoyFX49JPqnkzPXuBk5z+nroNzUL5ssRIFC4KiCFUMLC87RqAG/zELT4fRmIjkwittGUIlu9K3T4ua4qcVXSDO+KbGNkzM7W+Q== 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 2023/10/5 13:19, Mike Rapoport wrote: > On Wed, Oct 04, 2023 at 12:30:45AM +0800, Yajun Deng wrote: >> There is round twice in memblock_add_range(). The first counts the number >> of regions needed to accommodate the new area. The second actually inserts >> them. But the first round isn't really needed, we just need to check the >> counts before inserting them. >> >> Check the count before memblock_insert_region. If the count is equal to >> the maximum, it needs to resize the array. Otherwise, insert it directly. >> >> Also, there is a nested call here, we need to reserve the current array >> immediately if slab is unavailable. > I presume this fixes a bug you found in v2, but are you sure it'll _never_ > explode on a machine with different memory layout and different sequence of > memblock_reservee() calls? Not really. It has become complex. Because it has to deal with the nested call. I think v1 is the best solution if you accept it. It doesn't need to deal with the nested call. It would be safer. I don't think we must do memblock_reserve() in memblock_double_array(). These parameters 'addr' and 'new_alloc_size' in memblock_reserve come from memblock_find_in_range(). Since we can handle these parameters out of memblock_find_in_range(), we can also handle 'addr' and 'new_alloc_size' out of memblock_double_array(). > I don't see this micro-optimization is worth the churn and potential bugs. > NAK. There are many handouts that tell people it needs to run twice in memblock_add_range(). I think it's time to change this. I'm trying to tell people that running twice is unnecessary. Like v1, it just needs to check the count and handle memblock_reserve out of memblock_double_array. > >> Signed-off-by: Yajun Deng >> --- >> v3: reserve the current array immediately if slab is unavailable. >> v2: remove the changes of memblock_double_array. >> v1: https://lore.kernel.org/all/20230927013752.2515238-1-yajun.deng@linux.dev/ >> --- >> mm/memblock.c | 93 +++++++++++++++++++++++---------------------------- >> 1 file changed, 41 insertions(+), 52 deletions(-) >> >> diff --git a/mm/memblock.c b/mm/memblock.c >> index 5a88d6d24d79..71449c0b8bc8 100644 >> --- a/mm/memblock.c >> +++ b/mm/memblock.c >> @@ -588,11 +588,12 @@ static int __init_memblock memblock_add_range(struct memblock_type *type, >> phys_addr_t base, phys_addr_t size, >> int nid, enum memblock_flags flags) >> { >> - bool insert = false; >> phys_addr_t obase = base; >> phys_addr_t end = base + memblock_cap_size(base, &size); >> - int idx, nr_new, start_rgn = -1, end_rgn; >> + int idx, start_rgn = -1, end_rgn; >> struct memblock_region *rgn; >> + int use_slab = slab_is_available(); >> + unsigned long ocnt = type->cnt; >> >> if (!size) >> return 0; >> @@ -608,25 +609,6 @@ static int __init_memblock memblock_add_range(struct memblock_type *type, >> return 0; >> } >> >> - /* >> - * The worst case is when new range overlaps all existing regions, >> - * then we'll need type->cnt + 1 empty regions in @type. So if >> - * type->cnt * 2 + 1 is less than or equal to type->max, we know >> - * that there is enough empty regions in @type, and we can insert >> - * regions directly. >> - */ >> - if (type->cnt * 2 + 1 <= type->max) >> - insert = true; >> - >> -repeat: >> - /* >> - * The following is executed twice. Once with %false @insert and >> - * then with %true. The first counts the number of regions needed >> - * to accommodate the new area. The second actually inserts them. >> - */ >> - base = obase; >> - nr_new = 0; >> - >> for_each_memblock_type(idx, type, rgn) { >> phys_addr_t rbase = rgn->base; >> phys_addr_t rend = rbase + rgn->size; >> @@ -644,15 +626,30 @@ static int __init_memblock memblock_add_range(struct memblock_type *type, >> WARN_ON(nid != memblock_get_region_node(rgn)); >> #endif >> WARN_ON(flags != rgn->flags); >> - nr_new++; >> - if (insert) { >> - if (start_rgn == -1) >> - start_rgn = idx; >> - end_rgn = idx + 1; >> - memblock_insert_region(type, idx++, base, >> - rbase - base, nid, >> - flags); >> + >> + /* >> + * If type->cnt is equal to type->max, it means there's >> + * not enough empty region and the array needs to be >> + * resized. Otherwise, insert it directly. >> + * >> + * If slab is unavailable, it means a new array was reserved >> + * in memblock_double_array. There is a nested call here, We >> + * need to reserve the current array now if its type is >> + * reserved. >> + */ >> + if (type->cnt == type->max) { >> + if (memblock_double_array(type, obase, size)) >> + return -ENOMEM; >> + else if (!use_slab && type == &memblock.reserved) >> + return memblock_reserve(obase, size); >> } >> + >> + if (start_rgn == -1) >> + start_rgn = idx; >> + end_rgn = idx + 1; >> + memblock_insert_region(type, idx++, base, >> + rbase - base, nid, >> + flags); >> } >> /* area below @rend is dealt with, forget about it */ >> base = min(rend, end); >> @@ -660,33 +657,25 @@ static int __init_memblock memblock_add_range(struct memblock_type *type, >> >> /* insert the remaining portion */ >> if (base < end) { >> - nr_new++; >> - if (insert) { >> - if (start_rgn == -1) >> - start_rgn = idx; >> - end_rgn = idx + 1; >> - memblock_insert_region(type, idx, base, end - base, >> - nid, flags); >> + >> + if (type->cnt == type->max) { >> + if (memblock_double_array(type, obase, size)) >> + return -ENOMEM; >> + else if (!use_slab && type == &memblock.reserved) >> + return memblock_reserve(obase, size); >> } >> - } >> >> - if (!nr_new) >> - return 0; >> + if (start_rgn == -1) >> + start_rgn = idx; >> + end_rgn = idx + 1; >> + memblock_insert_region(type, idx, base, end - base, >> + nid, flags); >> + } >> >> - /* >> - * If this was the first round, resize array and repeat for actual >> - * insertions; otherwise, merge and return. >> - */ >> - if (!insert) { >> - while (type->cnt + nr_new > type->max) >> - if (memblock_double_array(type, obase, size) < 0) >> - return -ENOMEM; >> - insert = true; >> - goto repeat; >> - } else { >> + if (ocnt != type->cnt) >> memblock_merge_regions(type, start_rgn, end_rgn); >> - return 0; >> - } >> + >> + return 0; >> } >> >> /** >> -- >> 2.25.1 >>