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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

On Thursday, August 12, 2021, David Hildenbrand <david@redhat.com> wrote:

> On 11.08.21 22:47, Andy Shevchenko wrote:
>
>>
>>
>> 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,
>
>
>> 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

[-- Attachment #2: Type: text/html, Size: 3676 bytes --]