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

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

[Andrew Cooper]

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.

This compiles to marginally better code in all cases.  No functional change.

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)
-- 
2.30.2

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

[Julien Grall]

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

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

[Andrew Cooper]

On 13/04/2023 9:13 pm, Julien Grall wrote:
>
> 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?

I happened to be working in the area while fixing a bug.  But am not
justifying "because I judged it to be worth doing" further; it is
entirely self evident from the fact I sent the patch.

Whether you meant it to be or not, the request comes across as
insulting, and is not something which should be made of any submitter.

But as this kind of thing has come up before, any further debate on the
matter can be made to the code of conduct board.

A better phrasing might have been "I'm sorry, I don't understand.  Why
is this an improvement?"  But I'm only guessing as to what this issue is.


But moving to the technicals aspects, in an attempt to help this along.

Do you understand what folding the exit paths means?  It's a term which
is used frequently enough on list that it ought to be taken for granted,
and is what in my opinion makes the patch entirely self-evident.

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

Yes.

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

Perhaps, but I don't have the time, energy, or willing to dive into the
viper pit which is trying to make any changes to that document at all. 
Especially when there's a laundry list of more important topics that
ought to take priority...

~Andrew

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

[Stefano Stabellini]

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

On Thu, 13 Apr 2023, Andrew Cooper wrote:
> On 13/04/2023 9:13 pm, Julien Grall wrote:
> >
> > 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?
> 
> I happened to be working in the area while fixing a bug.  But am not
> justifying "because I judged it to be worth doing" further; it is
> entirely self evident from the fact I sent the patch.
> 
> Whether you meant it to be or not, the request comes across as
> insulting, and is not something which should be made of any submitter.
> 
> But as this kind of thing has come up before, any further debate on the
> matter can be made to the code of conduct board.
> 
> A better phrasing might have been "I'm sorry, I don't understand.  Why
> is this an improvement?"  But I'm only guessing as to what this issue is.

Hi Andrew,

I don't think Julien's comment was insulting. You probably thought this
was a two steps process:
step1: Why?
step2: Gotcha! NACK!

But Julien's question should be taken at face value. Julien even wrote
that he thinks the code is OK. "Why is this an improvement?" is a nicer
way to phrase it but both are OK in my view.


When we make code improvements (not bug fixes or new features) and the
improvement is marginal, I think it is an OK question to ask why you
thought it was worth doing.

For instance, could it be that there are other additional benefits that
you didn't write down in the commit message? Such as, is the code more
readable, easier to maintain, more resilient to attacks?

It is also OK if it is only a marginal improvement but in any case "why
are you doing it" should be or be seen as a challenge.


> But moving to the technicals aspects, in an attempt to help this along.
> 
> Do you understand what folding the exit paths means?  It's a term which
> is used frequently enough on list that it ought to be taken for granted,
> and is what in my opinion makes the patch entirely self-evident.
> 
> > Lastly what do you mean by "all cases"? Is it all arch? All compilers?
> 
> Yes.
> 
> >
> > 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).
> 
> Perhaps, but I don't have the time, energy, or willing to dive into the
> viper pit which is trying to make any changes to that document at all. 
> Especially when there's a laundry list of more important topics that
> ought to take priority...

Also here, I don't think Julien meant to put one more
potentially-blocking obstacle in your path to upstreaming the
improvement.

If folding the exit paths is an important pattern it is a good idea to
make it a recommended guideline project-wide under CODING_STYLE. It
makes your idea more generally applicable.

Otherwise we end up with, let's say, xen/arch/x86 with good exit paths
and xen/arch/arm with bad exit paths.
 
It should not be taboo to update CODING_STYLE. In the past it was
difficult, but now we have a process in place. Specifically, I am happy
to add it to the agenda for the next MISRA C meeting, where we can make
a snap decision on this guideline in few minutes. 

Once it is in CODING_STYLE, you won't have to discuss on the mailing
list why you are doing this sort of things anymore, and you can follow
up with dozen of patches improving exit paths everywhere.

I don't think you need to wait for the next MISRA C meeting (or other
calls) before following up on this one patch, but suggesting an update
of CODING_STYLE I think it is positive.

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

[Jan Beulich]

On 13.04.2023 21: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.

Mind me asking about "cannot"? Iirc the compiler is permitted to fold
even volatile asm()s, as long as certain guarantees aren't violated.
With "... often won't ..." or alike I'd be happy to give my ack, albeit
doing so with another maintainer's questions pending (and imo wrongly
taken by you as possibly insulting) it's never really clear to me
whether doing so is appropriate. I would specifically argue that ...

> This compiles to marginally better code in all cases.  No functional change.

... even minor improvements are worth it.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Preferably (unless proven wrong) with said adjustment and on the basis
that Julien signals his question as addressed (once it really is, of
course):
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan