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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03798C433F5 for ; Thu, 23 Sep 2021 13:54:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8829D61090 for ; Thu, 23 Sep 2021 13:54:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8829D61090 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=csgroup.eu Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id B875A900002; Thu, 23 Sep 2021 09:54:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B36F76B0071; Thu, 23 Sep 2021 09:54:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A278D900002; Thu, 23 Sep 2021 09:54:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0048.hostedemail.com [216.40.44.48]) by kanga.kvack.org (Postfix) with ESMTP id 929F36B006C for ; Thu, 23 Sep 2021 09:54:52 -0400 (EDT) Received: from smtpin40.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4C1578249980 for ; Thu, 23 Sep 2021 13:54:52 +0000 (UTC) X-FDA: 78618984024.40.FFC91D4 Received: from pegase2.c-s.fr (pegase2.c-s.fr [93.17.235.10]) by imf08.hostedemail.com (Postfix) with ESMTP id C71D530000A8 for ; Thu, 23 Sep 2021 13:54:51 +0000 (UTC) Received: from localhost (mailhub3.si.c-s.fr [172.26.127.67]) by localhost (Postfix) with ESMTP id 4HFc9P5sHPz9sV7; Thu, 23 Sep 2021 15:54:49 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from pegase2.c-s.fr ([172.26.127.65]) by localhost (pegase2.c-s.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xdm1Ouzf8XXu; Thu, 23 Sep 2021 15:54:49 +0200 (CEST) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase2.c-s.fr (Postfix) with ESMTP id 4HFc9P4lm1z9sV4; Thu, 23 Sep 2021 15:54:49 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 8BA938B776; Thu, 23 Sep 2021 15:54:49 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id i24VgXHaAF2s; Thu, 23 Sep 2021 15:54:49 +0200 (CEST) Received: from PO20335.IDSI0.si.c-s.fr (unknown [192.168.202.200]) by messagerie.si.c-s.fr (Postfix) with ESMTP id ACF658B763; Thu, 23 Sep 2021 15:54:47 +0200 (CEST) Subject: Re: [PATCH 3/3] memblock: cleanup memblock_free interface To: Mike Rapoport Cc: Mike Rapoport , Linus Torvalds , devicetree@vger.kernel.org, linux-efi@vger.kernel.org, kvm@vger.kernel.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, linux-um@lists.infradead.org, linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, linux-mips@vger.kernel.org, linux-mm@kvack.org, iommu@lists.linux-foundation.org, linux-usb@vger.kernel.org, linux-alpha@vger.kernel.org, sparclinux@vger.kernel.org, xen-devel@lists.xenproject.org, Andrew Morton , linux-snps-arc@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org, linux-arm-kernel@lists.infradead.org References: <20210923074335.12583-1-rppt@kernel.org> <20210923074335.12583-4-rppt@kernel.org> <1101e3c7-fcb7-a632-8e22-47f4a01ea02e@csgroup.eu> From: Christophe Leroy Message-ID: <96e3da9f-70ff-e5c0-ef2e-cf0b636e5695@csgroup.eu> Date: Thu, 23 Sep 2021 15:54:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: fr-FR X-Stat-Signature: ek1qh8xffdpyorb3dyczepkzcxb3mtf4 Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf08.hostedemail.com: domain of christophe.leroy@csgroup.eu designates 93.17.235.10 as permitted sender) smtp.mailfrom=christophe.leroy@csgroup.eu X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: C71D530000A8 X-HE-Tag: 1632405291-67204 Content-Transfer-Encoding: quoted-printable 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: Le 23/09/2021 =C3=A0 14:01, Mike Rapoport a =C3=A9crit=C2=A0: > On Thu, Sep 23, 2021 at 11:47:48AM +0200, Christophe Leroy wrote: >> >> >> Le 23/09/2021 =C3=A0 09:43, Mike Rapoport a =C3=A9crit=C2=A0: >>> From: Mike Rapoport >>> >>> For ages memblock_free() interface dealt with physical addresses even >>> despite the existence of memblock_alloc_xx() functions that return a >>> virtual pointer. >>> >>> Introduce memblock_phys_free() for freeing physical ranges and repurp= ose >>> memblock_free() to free virtual pointers to make the following pairin= g >>> abundantly clear: >>> >>> int memblock_phys_free(phys_addr_t base, phys_addr_t size); >>> phys_addr_t memblock_phys_alloc(phys_addr_t base, phys_addr_t size); >>> >>> void *memblock_alloc(phys_addr_t size, phys_addr_t align); >>> void memblock_free(void *ptr, size_t size); >>> >>> Replace intermediate memblock_free_ptr() with memblock_free() and dro= p >>> unnecessary aliases memblock_free_early() and memblock_free_early_nid= (). >>> >>> Suggested-by: Linus Torvalds >>> Signed-off-by: Mike Rapoport >>> --- >> >>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c >>> index 1a04e5bdf655..37826d8c4f74 100644 >>> --- a/arch/s390/kernel/smp.c >>> +++ b/arch/s390/kernel/smp.c >>> @@ -723,7 +723,7 @@ void __init smp_save_dump_cpus(void) >>> /* Get the CPU registers */ >>> smp_save_cpu_regs(sa, addr, is_boot_cpu, page); >>> } >>> - memblock_free(page, PAGE_SIZE); >>> + memblock_phys_free(page, PAGE_SIZE); >>> diag_amode31_ops.diag308_reset(); >>> pcpu_set_smt(0); >>> } >>> @@ -880,7 +880,7 @@ void __init smp_detect_cpus(void) >>> /* Add CPUs present at boot */ >>> __smp_rescan_cpus(info, true); >>> - memblock_free_early((unsigned long)info, sizeof(*info)); >>> + memblock_free(info, sizeof(*info)); >>> } >>> /* >> >> I'm a bit lost. IIUC memblock_free_early() and memblock_free() where >> identical. >=20 > Yes, they were, but all calls to memblock_free_early() were using > __pa(vaddr) because they had a virtual address at hand. I'm still not following. In the above memblock_free_early() was taking=20 (unsigned long)info . Was it a bug ? It looks odd to hide bug fixes in=20 such a big patch, should that bug fix go in patch 2 ? >=20 >> In the first hunk memblock_free() gets replaced by memblock_phys_free(= ) >> In the second hunk memblock_free_early() gets replaced by memblock_fre= e() >=20 > In the first hunk the memory is allocated with memblock_phys_alloc() an= d we > have a physical range to free. In the second hunk the memory is allocat= ed > with memblock_alloc() and we are freeing a virtual pointer. > =20 >> I think it would be easier to follow if you could split it in several >> patches: >=20 > It was an explicit request from Linus to make it a single commit: >=20 > but the actual commit can and should be just a single commit that ju= st > fixes 'memblock_free()' to have sane interfaces. >=20 > I don't feel strongly about splitting it (except my laziness really > objects), but I don't think doing the conversion in several steps worth= the > churn. The commit is quite big (55 files changed, approx 100 lines modified). If done in the right order the change should be minimal. It is rather not-easy to follow and review when a function that was=20 existing (namely memblock_free() ) disappears and re-appears in the same=20 commit but to do something different. You do: - memblock_free() =3D=3D> memblock_phys_free() - memblock_free_ptr() =3D=3D> memblock_free() At least you could split in two patches, the advantage would be that=20 between first and second patch memblock() doesn't exist anymore so you=20 can check you really don't have anymore user. >=20 >> - First patch: Create memblock_phys_free() and change all relevant >> memblock_free() to memblock_phys_free() - Or change memblock_free() to >> memblock_phys_free() and make memblock_free() an alias of it. >> - Second patch: Make memblock_free_ptr() become memblock_free() and ch= ange >> all remaining callers to the new semantics (IIUC memblock_free(__pa(pt= r)) >> becomes memblock_free(ptr) and make memblock_free_ptr() an alias of >> memblock_free() >> - Fourth patch: Replace and drop memblock_free_ptr() >> - Fifth patch: Drop memblock_free_early() and memblock_free_early_nid(= ) (All >> users should have been upgraded to memblock_free_phys() in patch 1 or >> memblock_free() in patch 2) >> >> Christophe >=20