linux-kernel - Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPcyv4gXgK+1hbzdCVJmkw7N=SE5tBrxB2UZ5Uh-DZCOjCEN1A@mail.gmail.com>
Date:   Thu, 2 Sep 2021 08:54:47 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] kernel/resource: clean up and optimize iomem_is_exclusive()

On Thu, Sep 2, 2021 at 12:52 AM David Hildenbrand <david@...hat.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@...hat.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@...el.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.