On Fri, 2020-02-07 at 20:27 +0000, Julien Grall wrote:

> > +        switch ( x & PGC_state )
> >           {
> > -            nx &= ~PGC_state;
> > -            nx |= (((x & PGC_state) == PGC_state_free)
> > -                   ? PGC_state_offlined : PGC_state_offlining);
> > -        }
> > +        case PGC_state_inuse:
> > +        case PGC_state_offlining:
> > +            nx |= broken ? PGC_state_offlining : PGC_state_broken_offlining;
> > +            break;
> > +
> > +        case PGC_state_free:
> > +            nx |= broken ? PGC_state_broken : PGC_state_offlined;
> >   
> > -        if ( broken )
> > -            nx |= PGC_broken;
> > +        case PGC_state_broken_offlining:
> > +            nx |= PGC_state_broken_offlining;
> > +
> > +        case PGC_state_offlined:
> > +        case PGC_state_broken:
> > +            nx |= PGC_state_broken;
> 
> Shouldn't this be:
> 
> case PGC_state_offlined:
>      nx |= broken ? PGC_state_offlined : PGC_state_broken;
> 
> case PGC_state_broken:
>      nx |= PGC_state_broken;
> 
> There are also quite a difference with the default case now. Without 
> this patch, if you were to add a new state but not handling it here, you 
> would transition to PGC_state_offlining. With this patch, we will 
> transtion to 0 (i.e PGC_state_inuse for now).
> 
> PGC_state_* are not an enum, the compiler can't help to catch new state 
> that doesn't have a corresponding case. So I would suggest to add a 
> default matching the current behavior and adding an 
> ASSERT_UNREACHABLE(). Note that I am open to a different kind of 
> handling here.

I revamped this, taking into account your later suggestion of a helper
that works on the count_info. Looks more like this:


        /* If it was already broken, it stays broken */
        if ( pgc_is_broken(x) )
            broken = 1;

        if ( pgc_is_offlined(x) || pgc_is(x, free) )
            nx |= broken ? PGC_state_broken : PGC_state_offlined;
        else
            nx |= broken ? PGC_state_broken_offlining : PGC_state_offlining;


> > - /* 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)
> > + /*
> > +  * 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)
> > +#define PGC_state_broken           PG_mask(5, 9)
> 
> I agree that making all the value aligned make it nicer to read, but 
> this is increasing number of "unrelated" changes and makes the review 
> more difficult.
> 
> I would prefer if we leave the indentation alone for the current 
> #define. But I am not going to push for it :).

I'm generally sympathetic to that point of view but at this point, all
those page states are kind of being redefined and it makes sense to
think about them all; having them all change doesn't hurt.