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
> >              continue;
> >  
> >          avail[node][zone]--;
> >          total_avail_pages--;
> >          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.



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


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


> > +#define PGC_state_broken           PG_mask(5, 9)
> > +
> > +#define page_state_is(pg, st)      (((pg)->count_info&PGC_state)
> > == PGC_state_##st)
> 
> Blanks around & please.

That part I hadn't touched but sure, I'll add those while I'm touching
it. I'd already ignored Julien's request *not* to make whitespace
cleanups while I'm here, after all :)

> > +#define page_is_broken(pg)         (page_state_is((pg),
> > broken_offlining) ||  \
> > +                                    page_state_is((pg), broken))
> > +#define page_is_offlined(pg)       (page_state_is((pg), broken)
> > ||    \
> > +                                    page_state_is((pg), offlined))
> 
> The inclusion of "broken" here would seem to deserve a (brief)
> comment, either here or next to PGC_state_broken's #define.

Done, in the version which will be sent shortly.

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

> Also for all three - no need for extra parentheses around pg
> (or in general macro arguments which get handed on without
> being part of an expression).

Yeah, I'll remove some of those.