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.