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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

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.

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

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

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

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

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.

>>>>>> +#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.

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

Jan

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