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=-9.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 C2B42C4338F for ; Thu, 12 Aug 2021 07:14:19 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3D3686103E for ; Thu, 12 Aug 2021 07:14:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 3D3686103E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 77A9E6B006C; Thu, 12 Aug 2021 03:14:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 72B1D6B0071; Thu, 12 Aug 2021 03:14:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 61A396B0072; Thu, 12 Aug 2021 03:14:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0171.hostedemail.com [216.40.44.171]) by kanga.kvack.org (Postfix) with ESMTP id 496186B006C for ; Thu, 12 Aug 2021 03:14:18 -0400 (EDT) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id DCEDA1F04C for ; Thu, 12 Aug 2021 07:14:17 +0000 (UTC) X-FDA: 78465564954.25.B43179B Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by imf07.hostedemail.com (Postfix) with ESMTP id 91DDB10081D7 for ; Thu, 12 Aug 2021 07:14:17 +0000 (UTC) Received: by mail-pj1-f51.google.com with SMTP id u13-20020a17090abb0db0290177e1d9b3f7so13813949pjr.1 for ; Thu, 12 Aug 2021 00:14:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=uTpuPc6jybGb8/DvYBQSSW/IZuaDxvXztvE6S4Hicb4=; b=sOyqW7iTclNbSetuTzyFasL3dzFtTN+bLyAwmBY6LlQXReYYAe8slkDoAvt+rGT8fS MJniPIh9uxD9XEQ5uNEw6uzZ9HCYsUIQ01gPNQri12s684OGlzYB5pL9GFsPgXup3Xog QfH7WN8lcLN+lUxkPJT5J5ED32MUxQwhad7OVEuUTlWtRJbhmHEbd3NAEO0Eg6s83jM4 SBwQ1A3GL3Oj5LffEE1uGIDyYGBr7zvyvKT7LoivSdv9EziZGyoSel0sYIRj6MWY1Dj2 TUM5my2966TT4Hnq6RDPzbSO11RoI/xij5s+amJgawu2zGshW8jFIZ31GRDkTuRGlI9/ DxBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uTpuPc6jybGb8/DvYBQSSW/IZuaDxvXztvE6S4Hicb4=; b=tpWd8xykNaO1u2r8bLjHdJrDQA27gIH091dJkoo685L1Kk35l/4xum+p7L7t7mefzm iq8Smne97E4Zpw+A7YmP+aKs2GJMNPT4PrfgewU0iHqwPI+Lp9rW8nwtYllHxnEvH+HT PXpeLVItEfPeD7TPInScxdkMtu7fh8qUXhdDDaYszDe668aW7qIlfJkf7c9jmJ3D3TWE AM5ighCarDB9x9XVshNYXtMbp1gzA0kdY6QCrpfbn7uT9+ZaTkN6Dk6D3RVppYpYSEiJ zo/C/Kn8KGZrKkpHxTo4FlkkcswI0wkp1j/GgHyaKz4Fdo6PcTiBHRpq7qPgI4LybGbD ehSg== X-Gm-Message-State: AOAM5311KOyvbmmmW4Hu4B2bJo9EpbLYu1aP/PQX2+cS4Z0WnrFC6jUF Sg5zAzvoar6Zj0/+PYs8u/esXIBlRmEqnWCpwa0= X-Google-Smtp-Source: ABdhPJycl3YPcVTG/5S7l85T8vjJVmFk80phN0hpMs/FjQrcfKM+Rgn2LAnOaxX+/woLbx7ZOAVmPYWhxgRWhuRBT6I= X-Received: by 2002:a63:cf0a:: with SMTP id j10mr2668987pgg.4.1628752456522; Thu, 12 Aug 2021 00:14:16 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a17:90b:4c4e:0:0:0:0 with HTTP; Thu, 12 Aug 2021 00:14:16 -0700 (PDT) In-Reply-To: <37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com> References: <20210811203612.138506-1-david@redhat.com> <20210811203612.138506-4-david@redhat.com> <37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com> From: Andy Shevchenko Date: Thu, 12 Aug 2021 10:14:16 +0300 Message-ID: Subject: Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive() To: David Hildenbrand Cc: "linux-kernel@vger.kernel.org" , Arnd Bergmann , Greg Kroah-Hartman , "Michael S. Tsirkin" , Jason Wang , "Rafael J. Wysocki" , Andrew Morton , Dan Williams , Hanjun Guo , Andy Shevchenko , "virtualization@lists.linux-foundation.org" , "linux-mm@kvack.org" Content-Type: multipart/alternative; boundary="000000000000991e6a05c9577b03" Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=sOyqW7iT; spf=pass (imf07.hostedemail.com: domain of andyshevchenko@gmail.com designates 209.85.216.51 as permitted sender) smtp.mailfrom=andyshevchenko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 91DDB10081D7 X-Stat-Signature: hqgpmkt1grzsfndsjg3i8k7hb5ozwqcb X-HE-Tag: 1628752457-908344 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: --000000000000991e6a05c9577b03 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thursday, August 12, 2021, David Hildenbrand wrote: > On 11.08.21 22:47, Andy Shevchenko wrote: > >> >> >> On Wednesday, August 11, 2021, David Hildenbrand > > wrote: >> >> Let's clean it up a bit, removing the unnecessary usage of r_next() = by >> next_resource(), and use next_range_resource() in case we are not >> interested in a certain subtree. >> >> Signed-off-by: David Hildenbrand > > >> --- >> kernel/resource.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/kernel/resource.c b/kernel/resource.c >> index 2938cf520ca3..ea853a075a83 100644 >> --- a/kernel/resource.c >> +++ b/kernel/resource.c >> @@ -1754,9 +1754,8 @@ static int strict_iomem_checks; >> */ >> bool iomem_is_exclusive(u64 addr) >> { >> - struct resource *p =3D &iomem_resource; >> + struct resource *p; >> bool err =3D false; >> - loff_t l; >> int size =3D PAGE_SIZE; >> >> if (!strict_iomem_checks) >> @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr) >> addr =3D addr & PAGE_MASK; >> >> read_lock(&resource_lock); >> - for (p =3D p->child; p ; p =3D r_next(NULL, p, &l)) { >> + for (p =3D iomem_resource.child; p ;) { >> >> > Hi Andy, > > >> I consider the ordinal part of p initialization is slightly better and >> done outside of read lock. >> >> Something like >> p=3D &iomem_res...; >> read lock >> for (p =3D p->child; ...) { >> > > Why should we care about doing that outside of the lock? That smells like > a micro-optimization the compiler will most probably overwrite either way > as the address of iomem_resource is just constant? > > Also, for me it's much more readable and compact if we perform a single > initialization instead of two separate ones in this case. > > We're using the pattern I use in, find_next_iomem_res() and > __region_intersects(), while we use the old pattern in > iomem_map_sanity_check(), where we also use the same unnecessary r_next() > call. > > I might just cleanup iomem_map_sanity_check() in a similar way. > > Yes, it=E2=80=99s like micro optimization. If you want your way I suggest t= hen to add a macro #define for_each_iomem_resource_child() \ for (iomem_resource...) > > Thanks, > > David / dhildenb > > --=20 With Best Regards, Andy Shevchenko --000000000000991e6a05c9577b03 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com> wrote:
On 11.08.21 22:47, Andy Shevchenko wrote:


On Wednesday, August 11, 2021, David Hildenbrand <david@redhat.com <mailto:david@redhat.com>> wro= te:

=C2=A0 =C2=A0 Let's clean it up a bit, removing the unnecessary usage o= f r_next() by
=C2=A0 =C2=A0 next_resource(), and use next_range_resource() in case we are= not
=C2=A0 =C2=A0 interested in a certain subtree.

=C2=A0 =C2=A0 Signed-off-by: David Hildenbrand <david@redhat.com
=C2=A0 =C2=A0 <mailto:david@redhat.com>>
=C2=A0 =C2=A0 ---
=C2=A0 =C2=A0 =C2=A0=C2=A0kernel/resource.c | 19 +++++++++++--------
=C2=A0 =C2=A0 =C2=A0=C2=A01 file changed, 11 insertions(+), 8 deletions(-)<= br>
=C2=A0 =C2=A0 diff --git a/kernel/resource.c b/kernel/resource.c
=C2=A0 =C2=A0 index 2938cf520ca3..ea853a075a83 100644
=C2=A0 =C2=A0 --- a/kernel/resource.c
=C2=A0 =C2=A0 +++ b/kernel/resource.c
=C2=A0 =C2=A0 @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
=C2=A0 =C2=A0 =C2=A0=C2=A0 */
=C2=A0 =C2=A0 =C2=A0=C2=A0bool iomem_is_exclusive(u64 addr)
=C2=A0 =C2=A0 =C2=A0=C2=A0{
=C2=A0 =C2=A0 -=C2=A0 =C2=A0 =C2=A0 =C2=A0struct resource *p =3D &iomem= _resource;
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct resource *p;
=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool err =3D false;
=C2=A0 =C2=A0 -=C2=A0 =C2=A0 =C2=A0 =C2=A0loff_t l;
=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 int size =3D PAGE_SIZE;

=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!strict_iomem_checks) =C2=A0 =C2=A0 @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D addr & PAGE_MA= SK;

=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 read_lock(&resource_loc= k);
=C2=A0 =C2=A0 -=C2=A0 =C2=A0 =C2=A0 =C2=A0for (p =3D p->child; p ; p =3D= r_next(NULL, p, &l)) {
=C2=A0 =C2=A0 +=C2=A0 =C2=A0 =C2=A0 =C2=A0for (p =3D iomem_resource.child; = p ;) {


Hi Andy,


I consider the ordinal part of p initialization is slightly better and done= outside of read lock.

Something like
p=3D &iomem_res...;
read lock
for (p =3D p->child; ...) {

Why should we care about doing that outside of the lock? That smells like a= micro-optimization the compiler will most probably overwrite either way as= the address of iomem_resource is just constant?

Also, for me it's much more readable and compact if we perform a single= initialization instead of two separate ones in this case.

We're using the pattern I use in, find_next_iomem_res() and __region_in= tersects(), while we use the old pattern in iomem_map_sanity_check(), where= we also use the same unnecessary r_next() call.

I might just cleanup iomem_map_sanity_check() in a similar way.



Yes, it=E2=80=99s like = micro optimization. If you want your way I suggest then to add a macro

#define for_each_iomem_resource_child() \
= =C2=A0for (iomem_resource...)

=C2=A0

Thanks,

David / dhildenb



--
With Best Regards,
Andy Shevchenko

--000000000000991e6a05c9577b03--