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

[Julien Grall <julien@xxxxxxx>]

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
> > > >               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.
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) ==
> > > > 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 )
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?
Cheers,

--
Julien Grall

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