linux-kernel - Re: [PATCH v1 3/3] kernel/resource: cleanup 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>] [day] [month] [year] [list]

Message-ID: <37179df3-13d7-9b98-4cd8-13bb7f735129@redhat.com>
Date:   Thu, 12 Aug 2021 09:07:41 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Hanjun Guo <guohanjun@...wei.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize
 iomem_is_exclusive()

On 11.08.21 22:47, Andy Shevchenko wrote:
> 
> 
> On Wednesday, August 11, 2021, David Hildenbrand <david@...hat.com 
> <mailto:david@...hat.com>> 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 <david@...hat.com
>     <mailto:david@...hat.com>>
>     ---
>       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 = &iomem_resource;
>     +       struct resource *p;
>              bool err = false;
>     -       loff_t l;
>              int size = PAGE_SIZE;
> 
>              if (!strict_iomem_checks)
>     @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
>              addr = addr & PAGE_MASK;
> 
>              read_lock(&resource_lock);
>     -       for (p = p->child; p ; p = r_next(NULL, p, &l)) {
>     +       for (p = 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= &iomem_res...;
> read lock
> for (p = 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.

-- 
Thanks,

David / dhildenb