|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
On Thu, 2020-03-19 at 09:49 +0100, Jan Beulich wrote:
> On 18.03.2020 18:13, David Woodhouse wrote:
> > 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.
>
> I appreciate your open an honest feedback, and having had similar
> comments in the past I can assure you that I've already tried to
> adjust where I find this acceptable. I take it you realize that
> there are two limitations in this - trying doesn't mean succeeding,
> and the boundaries of what I'd consider acceptable to let go with
> no comments.
>
> Of course there are always two sides of the medal.
>
> As a maintainer of some piece of code, I view it as my
> responsibility to look after not only the technical correctness of
> that code, but also after its style (in the broadest sense of the
> word). Looking at some very bad examples in our tree, many of
> which I'm afraid have a Linux origin, I'm in particular of the
> opinion that consistent style is a significant aid in code
> readability and maintainability. And I hope you agree that _style_
> adjustments are pretty easy to make, so I don't view asking for
> such as placing a significant burden on the submitter. The
> alternative of letting it go uncommented and then take the time
> myself to clean up seems quite a bit worse to me, not the least
> because of this scaling even less well than the amount of code
> review that needs doing.
Yes, 100% agreed. And I'll even concede that for the cases of moving
code around that happens to not conform to the current style, and
asking contributors to fix it up as they go.
I was agreeing with you on that point, while simultaneously telling
Julien "nah, I'll fix it while I'm here" when he suggested that I *not*
realign the PGC_state bit definitions.
> The mentioned Linux origin of some of the particularly bad
> examples in our tree is why I view your "they cope with the Linux
> community just fine" as not really applicable. This is despite
> our subsequent changes to those files often having made the
> situation worse rather than better.
Was more about the community effect than technical matters, but let's
not rathole on that.
> To some degree the same goes for bigger than necessary code churn,
> albeit I agree that in a number of cases it is far less objective
> to judge than the aim for consistent style. Extra code churn
> instead is often making review harder, irrespective of the often
> good intentions behind doing so.
Completely agreed.
> > 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.
>
> In which case I ought to consider, of course after first checking
> with my management, to step back as a maintainer. I'd very much
> regret doing so, but if it's in the interest of the community ...
I definitely don't think that would be in the interest of the
community. As I think I may have mentioned once or twice in my previous
message, your detailed reviews are massively appreciated and useful.
> (As an aside, likely being among those doing the largest part of
> code reviews, helping with that part of the overall workload the
> project generates would reduce the number of reviews I'd have to
> do, and hence the chances of me giving comments viewed as
> unhelpful or worse by submitters. Or, to put it in different,
> frank, but hopefully not offending words - I'd like to see you do
> a fair amount of code review, including looking after merely
> cosmetic aspects in the spirit of our written and unwritten rules,
> before you actually comment on me going too far with some of my
> feedback. And without me wanting to put too much emphasis on this:
> It is my opinion that maintainer views generally have somewhat
> higher weight than non-maintainer ones. I'm not going to claim
> though there aren't cases where I might go too far and hence abuse
> rather than use this, but as per above I can only try to avoid
> doing so, I can't promise to succeed. And of course I, like others,
> can be convinced to be wrong.)
Understood.
> > 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.
>
> So are you saying that as a maintainer I should let go uncommented a
> change which I'm unconvinced doesn't have negative side effects,
> besides its positive intended behavioral change? The more that here
> the workaround is rather trivial? As you may imagine, I've run into
> the situation myself a number of times, without considering this a
> reason to make any adjustments to the build machinery.
Jan, I would respectfully request that you take another look at your
initial response, but put yourself in the shoes of a patch submitter:
https://lists.xenproject.org/archives/html/xen-devel/2020-03/msg01171.html
You mention a "simple" workaround... but the workaround I've been using
is to manually remove the offending .o.d files, one at a time (or at
least one directory at a time), until the broken build starts working
again. Is that what you meant? And you really didn't ever consider that
it should be fixed?
And the substance of the response is basically saying "this is voodoo
and we can't touch it or unspecified things might break, but I have no
idea where to tell you to look."
Looking back I realise that the concern about phony rules overriding
pattern rules didn't even come from you; your concern was more nebulous
and unaddressable. It looks like I came up with a straw man and shot
*that* down in my later analysis (although that wasn't my intent; I
think the concern about pattern rules really did come from somewhere).
You asked a question about "why isn't this default behaviour", which is
kind of a silly question when asking about an option (-MP) that was
added to GCC almost a decade after the initial -MD behaviour was
established. Of *course* they didn't retroactively change the default.
Read that message again from the point of view of a contributor.
Pretend it isn't even me; pretend it's someone attempting to make their
first, trivial, improvement to the Xen ecosystem.
I hope you'll understand why my initial reaction was just a
monosyllabic 'no'.
> > > > > > > +#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.
>
> Well, in the specific case here I've meanwhile realized that my
> alternative naming suggested in in no way less ambiguous. So
> stick to what you've chosen, albeit I continue to dislike the
> name ambiguously also suggesting that the offlining operation
> might be broken (e.g. as in "can't be offlined"), rather than the
> page itself. I'm not going to exclude though that this is just
> because of not being a native English speaker.
As a native English speaker, the naming of these bothered me too.
They're too long and redundant. But subsuming the PGC_broken but into a
3-bit PGC_state makes sense, and we can't abandon that idea purely
because we can't come up with a *name* that fills us with joy.
There wasn't a *good* answer. I vacillated for a while, and picked the
one that offended me least.
And then ended up in a debate about it when it really wasn't important.
> > > > > > > +#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.
>
> Your experience may be different, but I hardly ever see any effect from
> reporting bugs (not just against gcc) unless they're really bad or really
> easy to address. That's why I generally prefer to fix such issues myself,
> provided of course that I can find the time.
Perhaps so. But if I *don't* file it, it *certainly* doesn't get fixed.
And I've learned over the years *not* to second-guess the optimisations
that today's compiler might make, with the wind blowing in this
particular direction.
FWIW 'return (x == 6 || x == 7)' ends up being emitted by GCC on x86 as
subl $6, %edi
xorl %eax, %eax
cmpl $1, %edi
setbe %al
ret
And 'return (x == 5 || x == 7)' gives:
andl $-3, %edi
xorl %eax, %eax
cmpl $5, %edi
sete %al
ret
So it does look like GCC is actually doing its job, on this occasion.
But that's entirely beside the point, which is that I'm having some
pointless discussion about compiler optimisation minutiæ when fixing
PGC_broken was *already* deep into yak-shaving for the improvement I
was *actually* trying to make. It's distracting from real work, raising
barriers to getting fixes merged.
> > Don't make the source code gratuitously harder to read.
>
> That's a very subjective aspect again. Personally I find two comparisons
> of the same variable against different constants 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.
>
> Note how I said "I wonder", not "please make".
Perspective again. That distinction really doesn't matter. Perhaps you
underestimate the weight your words carry, as a well-respected
maintainer. You can't negate that effect purely by word tricks like
saying 'I wonder'.
Because understatement is a very common tool in the English language,
especially in British English — and we've all seen people write "I
wonder if you should..." when what was really meant was "I will set
fire to you if you don't...".
Understatement like that doesn't work. It still derails the patch
review. It just didn't need to be said, in that context.
Let me repeat — because I've only said it once today, I think, that
your reviews are incredibly useful. I'm only asking that you recognise
the weight that your 'wondering' can have, and recognise when something
you are asking for is *subjective*.
A review is not about "is this code precisely how it would look if I
had written it myself", but it is about "is this code correct and
maintainable".
Sometimes, as in the example with the PGC_state_ naming above, there
isn't a "nice" answer. We pick the solution that offends us least. And
I completely understand as a maintainer, what it's like to be on the
receiving end of such a choice. You think "that could be nicer"... and
have to work through the alternatives yourself before you realise that
actually, it was the best of the choices available.
Each of the responses I've identified from you as 'excessive' has some
merit, we can focus on each of them and you can justify them, to a
certain extent. But as a whole, the effect is of a barrage of nitpicks
of questionable utility which really does hinder forward progress.
Let's try to focus on comments which will genuinely make the code
better. It's not that we should deliberately stop paying attention to
detail, or deliberately allow buggy and broken code into the tree. It's
that we should be aware that "perfect is the enemy of good".
For my part, I'll stop whining at you now. If I end up giving responses
to parts of your code review which seem to be along the lines of
"that's nice, dear, but I didn't think so and I did the typing", please
hark back to this conversation. I'll try to phrase them more
appropriately than that, but no promises :)
Thanks.
Attachment:
smime.p7s _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |