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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan

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