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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

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

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

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

Jan

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