Re: [PATCH 2/2] Optimize compound_head() by avoiding a shared pageflag

[Andrew Morton]

On Mon, 9 Apr 2007 11:09:40 -0700 (PDT)
Christoph Lameter <clameter@xxxxxxx> wrote:

> Add PageTail / PageHead in order to avoid multiple branches when compound 
> pages are checked.
> 
> The patch adds PageTail(page) and PageHead(page) to check if a page is the
> head or the tail of a compound page. This is done by masking the two
> bits describing the state of a compound page and then comparing them. So 
> one comparision and a branch instead of two bit checks and two branches.
> 

OK.  I'm still a bit concerned about bypassing the bitops synchronisation:
barriers, volatile, etc.  We had lengthy ruminations on that a few years
ago, I think when working on free_pages_check().


> @@ -221,12 +215,24 @@ static inline void SetPageUptodate(struc
>  #define __ClearPageCompound(page) __clear_bit(PG_compound, &(page)->flags)
>  
>  /*
> - * Note: PG_tail is an alias of another page flag. The result of PageTail()
> - * is only valid if PageCompound(page) is true.
> + * PG_reclaim is used in combination with PG_compound to mark the
> + * head and tail of a compound page
> + *
> + * PG_compound & PG_reclaim	=> Tail page
> + * PG_compound & ~PG_reclaim	=> Head page
>   */
> -#define PageTail(page)	test_bit(PG_tail, &(page)->flags)
> -#define __SetPageTail(page)	__set_bit(PG_tail, &(page)->flags)
> -#define __ClearPageTail(page)	__clear_bit(PG_tail, &(page)->flags)
> +
> +#define PG_head_tail_mask ((1L << PG_compound) | (1L << PG_reclaim))
> +
> +#define PageTail(page)	((page->flags & PG_head_tail_mask) \
> +				== PG_head_tail_mask)
> +#define __SetPageTail(page)	page->flags |= PG_head_tail_mask
> +#define __ClearPageTail(page)	page->flags ~= PG_head_tail_mask

hm.  The lack of parenthesisation here _might_ be OK, but I haven't
thought it through.

And I'd prefer not to have to, because I know that the do { } while (0)
thing works.  As do static inline functions.

> +#define PageHead(page)	((page->flags & PG_head_tail_mask) \
> +				== (1L << PG_compound))
> +#define __SetPageHead(page)	__SetCompoundPage(page)
> +#define __ClearPageHead(page)	__ClearCompoundPage(page)

You meant __SetPageCompound and __ClearPageCompound.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/