[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/2] xen/mm: Introduce PG_state_uninitialised

On 17.03.2020 23:15, David Woodhouse wrote:
On Thu, 2020-02-20 at 12:59 +0100, Jan Beulich wrote:
On 07.02.2020 19:04, David Woodhouse wrote:
         ASSERT((pg[i].count_info & ~PGC_extra) == PGC_state_inuse ||
                (pg[i].count_info & ~PGC_extra) == PGC_state_uninitialised);
          page_set_owner(&pg[i], d);
          smp_wmb(); /* Domain pointer must be visible before updating refcnt. 
-        pg[i].count_info = PGC_allocated | 1;
+        pg[i].count_info |= PGC_allocated | 1;

This is too relaxed for my taste: I understand you want to
retain page state, but I suppose other bits would want clearing

You seem to have dropped the ASSERT immediately before the code snippet
you cited, in which arbitrary other contents of count_info are not
permitted. I put it back, in its current form after I rebase on top of
Paul's commit c793d13944b45d assing PGC_extra.

But that' only an ASSERT(), i.e. no protection at all in release builds.

--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -72,12 +72,13 @@
    * { 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_uninitialised    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)
  #define PGC_state_broken           PG_mask(5, 9)
+#define PGC_state_inuse            PG_mask(6, 9)

Would imo be nice if this most common state was actually
either 1 or 7, for easy recognition. But the most suitable
value to pick may also depend on the outcome of one of the
comments on patch 1.

Not quite sure why 1 and 7 are easier to recognise than other values.
The important one is that uninitialised has to be zero, since that's
the default (because that's what the frame table is memset to. Which is
changeable, but non-trivially so).

In particular 7 may imo be easier to recognize, as having all
of the three bits set. It sometimes helps seeing such at (almost)
the first glance in e.g. logged data.


Xen-devel mailing list



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