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

[Stefano Stabellini <sstabellini@kernel.org>]

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