Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

Similar for multiple == vs a single <= or >=.

Jan

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