On Thursday, August 12, 2021, David Hildenbrand [off-list ref] wrote:

On 11.08.21 22:47, Andy Shevchenko wrote:

quoted

On Wednesday, August 11, 2021, David Hildenbrand <david@redhat.com
<mailto:david@redhat.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@redhat.com
    <mailto:david@redhat.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,

quoted

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.

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

#define for_each_iomem_resource_child() \
 for (iomem_resource...)

Thanks,

David / dhildenb

-- 
With Best Regards,
Andy Shevchenko