Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Wed, 2020-03-18 at 12:31 +0000, Julien Grall wrote:
> On 18/03/2020 09:56, Jan Beulich wrote:
> > On 17.03.2020 22:52, David Woodhouse wrote:
> > > On Thu, 2020-02-20 at 12:10 +0100, Jan Beulich wrote:
> > > > > @@ -1699,14 +1714,14 @@ unsigned int online_page(mfn_t mfn,
> > > > > uint32_t *status)
> > > > >       do {
> > > > >           ret = *status = 0;
> > > > > -        if ( y & PGC_broken )
> > > > > +        if ( (y & PGC_state) == PGC_state_broken ||
> > > > > +             (y & PGC_state) == PGC_state_broken_offlining )
> > > > >           {
> > > > >               ret = -EINVAL;
> > > > >               *status = PG_ONLINE_FAILED |PG_ONLINE_BROKEN;
> > > > >               break;
> > > > >           }
> > > > > -
> > > > > -        if ( (y & PGC_state) == PGC_state_offlined )
> > > > > +        else if ( (y & PGC_state) == PGC_state_offlined )
> > > > 
> > > > I don't see a need for adding "else" here.
> > > 
> > > They are mutually exclusive cases. It makes things a whole lot clearer
> > > to the reader to put the 'else' there, and sometimes helps a naïve
> > > compiler along the way too.
> > 
> > Well, I'm afraid I'm going to be pretty strict about this: It's again
> > a matter of taste, yes, but we generally try to avoid pointless else.
> > What you consider "a whole lot clearer to the reader" is the opposite
> > from my pov.
> 
> While I agree the 'else' may be pointless, I don't think it is worth an 
> argument. As the author of the patch, it is his choice to write the code 
> like that.

Indeed. While I appreciate your insight, Jan, and your detailed reviews
are undoubtedly helpful — especially to me as I poke around the Xen
code base without knowing where the bodies are buried — I do sometimes
find that it degenerates into what appears to be gratuitous
bikeshedding.

Like *some* others, I'm perfectly capable of responding "I understand
you would have done it differently, but I prefer it this way".

But even for those like me who have the self-confidence (or arrogance?)
to respond in such a way, the end result is often the same — a patch
series which the maintainer doesn't apply because it has "unresolved
issues".

Perfect is the enemy of good. Especially when perfection is so
subjective.

This definitely isn't the kind of welcoming community that I enjoy
trying to get my junior engineers to contribute to. And they aren't
snowflakes; they cope with the Linux community just fine, for the most
part.


Earlier today, I found myself adjusting a patch in order to tweak the
behaviour of a "can never happen" situation, when it was far from clear
that the *existing* behaviour in that situation would have been correct
anyway.

There is a lot of value in your reviews, and they are appreciated. But
the overall effect is seen by some as making the Xen community somewhat
dysfunctional. 

The -MP makefile patch I posted yesterday... I almost didn't bother.
And when I allowed myself to be talked into it, I was entirely
unsurprised when a review came in basically asking me to prove a
negative before the patch could proceed. So as far as I can tell, it'll
fall by the wayside and the build will remain broken any time anyone
removes or renames a header file. Because life's too short to invest
the energy to make improvements like that.

One of these days, I may attempt to revive my series cleaning up the
16-bit and 32-bit boot code. Which was a clear improvement and
simplification, and again you gave extremely valid feedback which
helped to improve it — but again it was interspersed with more
subjective and less helpful comments which basically derailed it. 

Having carefully threaded my way through the existing byzantine code
and made incremental bisectable changes, I ended up with feedback that
basically would have required me to start again from scratch, in order
to satisfy what appeared to be fairly arbitrary and subjective demands.

As is often the case in creating a bisectable series out of complex
changes, I had sometimes moved/refactored code, only to move/refactor
it again in a subsequent patch. Sometimes the ordering of such inter-
related changes can be fairly arbitrary, and I had made my choices as
I'd progressed; the real focus being the end result. At one point you
were picking on intermediate details of how I'd made my overall series
bisectable, and seemed to be demanding that I go back and refactor (the
intermediate stages for no better reason than because you would have
done it differently. 

Again, your attention to detail and your expertise are massively
appreciated. But please let's remember that "perfect is the enemy of
good", and strike a balance which allows forward progress without
blocking improvements.

Sometimes I wonder if you truly realise how much you derail the
progress of a patch series just by raising well-intentioned "queries"
around it.


> > > > > --- a/xen/include/asm-x86/mm.h
> > > > > +++ b/xen/include/asm-x86/mm.h
> > > > > @@ -67,18 +67,27 @@
> > > > >    /* 3-bit PAT/PCD/PWT cache-attribute hint. */
> > > > >   #define PGC_cacheattr_base PG_shift(6)
> > > > >   #define PGC_cacheattr_mask PG_mask(7, 6)
> > > > > - /* Page is broken? */
> > > > > -#define _PGC_broken       PG_shift(7)
> > > > > -#define PGC_broken        PG_mask(1, 7)
> > > > > - /* Mutually-exclusive page states: { inuse, offlining, offlined,
> > > > > free }. */
> > > > > -#define PGC_state         PG_mask(3, 9)
> > > > > -#define PGC_state_inuse   PG_mask(0, 9)
> > > > > -#define PGC_state_offlining PG_mask(1, 9)
> > > > > -#define PGC_state_offlined PG_mask(2, 9)
> > > > > -#define PGC_state_free    PG_mask(3, 9)
> > > > > -#define page_state_is(pg, st) (((pg)->count_info&PGC_state) ==
> > > > > PGC_state_##st)
> > > > > -
> > > > > - /* Count of references to this frame. */
> > > > > + /*
> > > > > +  * Mutually-exclusive page states:
> > > > > +  * { inuse, offlining, offlined, free, broken_offlining, broken }
> > > > > +  */
> > > > > +#define PGC_state                  PG_mask(7, 9)
> > > > > +#define PGC_state_inuse            PG_mask(0, 9)
> > > > > +#define PGC_state_offlining        PG_mask(1, 9)
> > > > > +#define PGC_state_offlined         PG_mask(2, 9)
> > > > > +#define PGC_state_free             PG_mask(3, 9)
> > > > > +#define PGC_state_broken_offlining PG_mask(4, 9)
> > > > 
> > > > TBH I'd prefer PGC_state_offlining_broken, as it's not the
> > > > offlining which is broken, but a broken page is being
> > > > offlined.
> > > 
> > > It is the page which is both broken and offlining.
> > > Or indeed it is the page which is both offlining and broken.
> > 
> > I.e. you agree with flipping the two parts around?

I hope I have respectfully made it clear that no, I'm really not happy
with the very concept of such a request.

Perhaps it would be easier for me to acquiesce, in the short term.

But on the whole I think it's better to put my foot down and say 'no',
and focus on real work and things that matter.

> > > > > +#define page_is_offlining(pg)      (page_state_is((pg), 
> > > > > broken_offlining) || \
> > > > > +                                    page_state_is((pg), offlining))
> > > > 
> > > > Overall I wonder whether the PGC_state_* ordering couldn't be
> > > > adjusted such that at least some of these three won't need
> > > > two comparisons (by masking off a bit before comparing).
> > > 
> > > The whole point in this exercise is that there isn't a whole bit for
> > > these; they are each *two* states out of the possible 8.
> > 
> > Sure. But just consider the more general case: Instead of writing
> > 
> >      if ( i == 6 || i == 7 )
> > 
> > you can as well write
> > 
> >      if ( (i | 1) == 7 )
> 
> I stumbled accross a few of those recently and this is not the obvious 
> things to read. Yes, your code may be faster. But is it really worth it 
> compare to the cost of readability and futureproofness?

No. Just no.

If that kind of change is really a worthwhile win, it'll depend on the
CPU. File a GCC PR with a test case as a missed optimisation. Don't
make the source code gratuitously harder to read.

Honestly, this, right here, is the *epitome* of why I, and others,
sometimes feel that submitting a patch to Xen can be more effort than
it's worth.

This email is not intended as a personal attack; I hope you don't feel
that it is. For about the fifth time: your careful reviews and your
attention to detail are *massively* appreciated. Just a little over the
time sometimes, and I'd like to ask you to take care to be aware of the
overall effect, and that you are not blocking progress.

Thanks.

Attachment: smime.p7s
Description: S/MIME cryptographic signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel