Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210831202141.13846-1-david@redhat.com> <20210831202141.13846-2-david@redhat.com>
 <16141669368a89f8304b1777094e982942d51bba.camel@intel.com> <22be7bd0-3f67-0e9c-87d4-1ec2c184cc6a@redhat.com>
In-Reply-To: <22be7bd0-3f67-0e9c-87d4-1ec2c184cc6a@redhat.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Thu, 2 Sep 2021 08:54:47 -0700
Message-ID: <CAPcyv4gXgK+1hbzdCVJmkw7N=SE5tBrxB2UZ5Uh-DZCOjCEN1A@mail.gmail.com>
Subject: Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()
To:     David Hildenbrand <david@redhat.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Sep 2, 2021 at 12:52 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 01.09.21 21:43, Williams, Dan J wrote:
> > On Tue, 2021-08-31 at 22:21 +0200, David Hildenbrand wrote:
> >> We end up traversing subtrees of ranges we are not interested in; let's
> >> optimize this case, skipping such subtrees, cleaning up the function a bit.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>   kernel/resource.c | 25 ++++++++++++++++++++-----
> >>   1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > That diffstat does not come across as "cleanup", and the skip_children
> > flag changing values mid-iteration feels tricky. Is there a win here,
> > the same number of entries still need to be accessed, right?
>
> Right, most of the patch changes falls under "optimize". The cleanup is
> using for_each_resource() and not using r_next(NULL, p, &l). Sure, I
> could have split this up but then I'd just introduce for_each_resource()
> to modify it immediately again.
>
>
> Let's take a look at /proc/iomem on my notebook:
>
> 00000000-00000fff : Reserved
> 00001000-00057fff : System RAM
> 00058000-00058fff : Reserved
> 00059000-0009cfff : System RAM
> 0009d000-000fffff : Reserved
>    000a0000-000bffff : PCI Bus 0000:00
>    000c0000-000c3fff : PCI Bus 0000:00
>    000c4000-000c7fff : PCI Bus 0000:00
>    000c8000-000cbfff : PCI Bus 0000:00
>    000cc000-000cffff : PCI Bus 0000:00
>    000d0000-000d3fff : PCI Bus 0000:00
>    000d4000-000d7fff : PCI Bus 0000:00
>    000d8000-000dbfff : PCI Bus 0000:00
>    000dc000-000dffff : PCI Bus 0000:00
>    000e0000-000e3fff : PCI Bus 0000:00
>    000e4000-000e7fff : PCI Bus 0000:00
>    000e8000-000ebfff : PCI Bus 0000:00
>    000ec000-000effff : PCI Bus 0000:00
>    000f0000-000fffff : PCI Bus 0000:00
>      000f0000-000fffff : System ROM
> 00100000-3fffffff : System RAM
> 40000000-403fffff : Reserved
>    40000000-403fffff : pnp 00:00
> 40400000-80a79fff : System RAM
> ...
>
> Why should we take a look at any children of "0009d000-000fffff :
> Reserved" if we can just skip these 15 items directly because the parent
> range is not of interest?

Oh I misread, it never loads the child entries into cache, so it's a
true skip and not a continue.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...I was going to say it should be named for_each_top_resource(), but
we can cross that bridge when / if something needs an iterator that
includes children.