Re: [PATCH] xen: Fold exit paths in find_text_region()

[Julien Grall <julien@xen.org>]

Hi Andrew,

You may want to update your runes to find the maintainers as you are 
CCing the x86 folks but not the REST (the patch modifies common/ after all).

On 13/04/2023 20:22, Andrew Cooper wrote:
> Despite rcu_read_unlock() being fully inlineable, the optimiser cannot fold
> these exit paths, because of the various compiler barriers providing RCU
> safety.  Help the compiler out.

Please mention which compiler(s) (including version) you used.

> 
> This compiles to marginally better code in all cases.
So the code itself is fine with me. But this raises a few questions. If 
this is marginal, then why are you doing it? What's your end goal?

Lastly what do you mean by "all cases"? Is it all arch? All compilers?

Anyway, if this pattern is important (TBD why), then I think we should 
update the CODING_STYLE with some guidance. Otherwise, we may introduce 
similar patterns (we already have some).

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>   xen/common/virtual_region.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index 30b0b4ab9c85..5ecdba9c08ed 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -40,20 +40,20 @@ static DEFINE_RCU_READ_LOCK(rcu_virtual_region_lock);
>   
>   const struct virtual_region *find_text_region(unsigned long addr)
>   {
> -    const struct virtual_region *region;
> +    const struct virtual_region *iter, *region = NULL;
>   
>       rcu_read_lock(&rcu_virtual_region_lock);
> -    list_for_each_entry_rcu( region, &virtual_region_list, list )
> +    list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>       {
> -        if ( (void *)addr >= region->start && (void *)addr < region->end )
> +        if ( (void *)addr >= iter->start && (void *)addr < iter->end )
>           {
> -            rcu_read_unlock(&rcu_virtual_region_lock);
> -            return region;
> +            region = iter;
> +            break;
>           }
>       }
>       rcu_read_unlock(&rcu_virtual_region_lock);
>   
> -    return NULL;
> +    return region;
>   }
>   
>   void register_virtual_region(struct virtual_region *r)

Cheers,

-- 
Julien Grall