Virtualization - Aug 2021 - [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()

On Wednesday, August 11, 2021, David Hildenbrand <david at 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 at 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 ;) {

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; ...) {


>                 /*
>                  * We can probably skip the resources without
>                  * IORESOURCE_IO attribute?
>                  */
>                 if (p->start >= addr + size)
>                         break;
> -               if (p->end < addr)
> +               if (p->end < addr) {
> +                       /* No need to consider children */
> +                       p = next_resource_skip_children(p);
>                         continue;
> +               }
> +
>                 /*
>                  * A resource is exclusive if IORESOURCE_EXCLUSIVE is set
>                  * or CONFIG_IO_STRICT_DEVMEM is enabled and the
>                  * resource is busy.
>                  */
> -               if ((p->flags & IORESOURCE_BUSY) == 0)
> -                       continue;
> -               if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM)
> -                               || p->flags & IORESOURCE_EXCLUSIVE)
{
> +               if (p->flags & IORESOURCE_BUSY &&
> +                   (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) ||
> +                    p->flags & IORESOURCE_EXCLUSIVE)) {
>                         err = true;
>                         break;
>                 }
> +               p = next_resource(p);
>         }
>         read_unlock(&resource_lock);
>
> --
> 2.31.1
>
>
-- 
With Best Regards,
Andy Shevchenko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210811/0adb078d/attachment.html>

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

-- 
Thanks,

David / dhildenb