Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

[David Hildenbrand]

On 11.08.21 22:47, Andy Shevchenko wrote:
> On Wednesday, August 11, 2021, David Hildenbrand <david@xxxxxxxxxx 
> <mailto:david@xxxxxxxxxx>> 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@xxxxxxxxxx
>     <mailto:david@xxxxxxxxxx>>
>     ---
>       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