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 810F5C433F5 for ; Tue, 15 Feb 2022 14:14:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C1F496B0078; Tue, 15 Feb 2022 09:14:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BCEF66B007B; Tue, 15 Feb 2022 09:14:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A96236B007D; Tue, 15 Feb 2022 09:14:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.26]) by kanga.kvack.org (Postfix) with ESMTP id 9A7756B0078 for ; Tue, 15 Feb 2022 09:14:41 -0500 (EST) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 58B482289A for ; Tue, 15 Feb 2022 14:14:41 +0000 (UTC) X-FDA: 79145209962.02.5CFC7E2 Received: from mail-yb1-f174.google.com (mail-yb1-f174.google.com [209.85.219.174]) by imf18.hostedemail.com (Postfix) with ESMTP id DA6411C0004 for ; Tue, 15 Feb 2022 14:14:40 +0000 (UTC) Received: by mail-yb1-f174.google.com with SMTP id bt13so56529375ybb.2 for ; Tue, 15 Feb 2022 06:14:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eclypsium.com; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5TB6dGxazC1uji3PM0bPijKJuSzmBt7/lbdUM4chpqU=; b=YDCb3hCHhZLLEPG5t2STM5k0BBUZ1OEQR1TmPowgoYCROFj1eheYFEv8Zi7sstHXNL VCBTAIw2bDn79YlEFIL/v80w7H7F5VUUrP4QmZylVQptNG183UARrdVBR1riwFhuPWLG RnrdND1gvstyq87OFph0FtblJkH4LvT8UxjIaie/w22X2pX3Ju6XRdRu17qLHgEij1kk xKLKK1zYpenx+E76kTXm1UPrgNnOneMahJtbCupPCBENe/oLh3Wlvj/ngbz/FBn/zGiv b0mllHKRloGS5T5sYehEFiPGnwRjKHq7NGaMGcodGBBncoOYkFY+BppdgSRblGKXKrAB +j6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5TB6dGxazC1uji3PM0bPijKJuSzmBt7/lbdUM4chpqU=; b=CLmhL/Pg7utT+2KdsKyf/BZaQuNjgh6MPBLcEEKLTzyLKExHQNn0IJrIgufxN1uJI2 pyVoaFn13AiYmIRSKvqk5ugN37WKPKueZNhwk5ThQAd0RfN02KzbI6sso75FwAOIyp7a pH1vB5d8bM8gX1kilBd5I1TmFkJdMsJuUvgQPtR7OScz6dfbXHOcYRdoGdSNLHYRCgTt eC97dyT/I/JckK2zMp4xnHkOBlER5vShQq4SJlOqayh8hpLUE2BmeZ03Rx6L2Pchf1k0 qDjNJvfswlb3/GDAK5xjhqKqy//B0olcRRSUCXt7pIJyK69rjfq1NVc4alHw8ZRr2PRq 5vOg== X-Gm-Message-State: AOAM5321dyRZybbMam/q73CntPPQ9QLT0UdGcE4fGB68uDlsNfTeUbha NLy/LkETzLyajeH/coU9XaAYxLv2/+tiNESbXvrMuw== X-Google-Smtp-Source: ABdhPJxiNz0d2nqELO9RSvILyKX1eArLqLnZoM38DGg0w/FBN2U8DfR5PNrq+qbmCdAr+SwUpTSLA3xgu53apagAxwM= X-Received: by 2002:a25:ad51:: with SMTP id l17mr3946323ybe.20.1644934480016; Tue, 15 Feb 2022 06:14:40 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a81:174d:0:0:0:0:0 with HTTP; Tue, 15 Feb 2022 06:14:39 -0800 (PST) In-Reply-To: References: <20220203164328.203629-1-martin.fernandez@eclypsium.com> <20220203164328.203629-4-martin.fernandez@eclypsium.com> <202202071325.F8450B3B2D@keescook> From: Martin Fernandez Date: Tue, 15 Feb 2022 11:14:39 -0300 Message-ID: Subject: Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove To: Mike Rapoport Cc: Kees Cook , linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org, linux-mm@kvack.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, ardb@kernel.org, dvhart@infradead.org, andy@infradead.org, gregkh@linuxfoundation.org, rafael@kernel.org, akpm@linux-foundation.org, daniel.gutson@eclypsium.com, hughsient@gmail.com, alex.bazhaniuk@eclypsium.com, alison.schofield@intel.com Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: DA6411C0004 X-Stat-Signature: kcyqntyc7ftbo9trdxzxq5gcfzzqg8xh Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=eclypsium.com header.s=google header.b=YDCb3hCH; dmarc=pass (policy=quarantine) header.from=eclypsium.com; spf=pass (imf18.hostedemail.com: domain of martin.fernandez@eclypsium.com designates 209.85.219.174 as permitted sender) smtp.mailfrom=martin.fernandez@eclypsium.com X-HE-Tag: 1644934480-734669 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 2/15/22, Mike Rapoport wrote: > Hi Martin, > > On Tue, Feb 08, 2022 at 06:01:21PM -0300, Martin Fernandez wrote: >> On 2/8/22, Mike Rapoport wrote: >> > On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote: >> >> On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote: >> >> > __e820__range_update and e820__range_remove had a very similar >> >> > implementation with a few lines different from each other, the lines >> >> > that actually perform the modification over the e820_table. The >> >> > similiraties were found in the checks for the different cases on how >> >> > each entry intersects with the given range (if it does at all). >> >> > These >> >> > checks were very presice and error prone so it was not a good idea >> >> > to >> >> > have them in both places. >> >> >> >> Yay removing copy/paste code! :) >> > >> > Removing copy/paste is nice but diffstat of >> > >> > arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++----------- >> > 1 file changed, 283 insertions(+), 100 deletions(-) >> > >> > does not look nice even accounting for lots of comments :( >> > >> > I didn't look closely, but diffstat clues that the refactoring making >> > things much more complex. >> > >> >> Yes, that diffstat surprised me as well. >> >> I have to mention that 110 of those lines are kerneldocs and blank >> lines, which is quite a lot. Also you have to take into account that I >> expanded most of the function definitions for better formatting, which >> also took some space. > > At last I had time to look more closely and I think that using a set of > callbacks is over-complicated. > > I think this can be done way simpler, e.g like this (untested) draft: > > https://git.kernel.org/rppt/h/x86/e820-update-range > Thanks for taking the time to reviewing it. Yeah, I did something like that in a previous version. Altough I wasn't really happy with that. https://lore.kernel.org/linux-efi/20220113213027.457282-4-martin.fernandez@eclypsium.com/ I think that with the struct with the function arguments looks more clear than what I did, but you have to take into account that I need to create yet another function similar to those and another parameter to the struct, and with that I think that __e820__range_update will look scary. I'll give it a try anyway!