[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

Hi Jan,

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:
On 07.02.2020 16:57, David Woodhouse wrote:
@@ -1145,16 +1145,19 @@ static int reserve_offlined_page(struct
page_info *head)
      for ( cur_head = head; cur_head < head + ( 1UL << head_order);
cur_head++ )
-        if ( !page_state_is(cur_head, offlined) )
+        struct page_list_head *list;
+        if ( page_state_is(cur_head, offlined) )
+            list = &page_offlined_list;
+        else if (page_state_is(cur_head, broken) )
+            list = &page_broken_list;
+        else
          ASSERT(total_avail_pages >= 0);
-        page_list_add_tail(cur_head,
-                           test_bit(_PGC_broken, &cur_head->count_info) ?
-                           &page_broken_list : &page_offlined_list);
+        page_list_add_tail(cur_head, list);

While I realize it's fewer comparisons this way, I still wonder
whether for the reader's sake it wouldn't better be
page_is_offlined() first and then page_is_broken() down here.

Nah, that would be worse. This way there are two cases which are
explicitly handled and the list to use for each of them is explicitly
set. The 'if (a||b) …    some_function(a ? thing_for_a : thing_for_b)'
construct is much less comprehensible.

It's a matter of taste, I agree, and in such a case I generally advise
to see about limiting code churn. For code you then still introduce
anew, yes, taste decisions may typically be to the authors judgement
(there are exceptions, though).

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

--- 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) ==
- /* 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

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?

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


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.