On Mon 15-10-18 13:26:56, Alexander Duyck wrote:
> This change makes it so that we use the same approach that was already in
> use on Sparc on all the archtectures that support a 64b long.
>
> This is mostly motivated by the fact that 8 to 10 store/move instructions
> are likely always going to be faster than having to call into a function
> that is not specialized for handling page init.
>
> An added advantage to doing it this way is that the compiler can get away
> with combining writes in the __init_single_page call. As a result the
> memset call will be reduced to only about 4 write operations, or at least
> that is what I am seeing with GCC 6.2 as the flags, LRU poitners, and
> count/mapcount seem to be cancelling out at least 4 of the 8 assignments on
> my system.
>
> One change I had to make to the function was to reduce the minimum page
> size to 56 to support some powerpc64 configurations.
This really begs for numbers. I do not mind the change itself with some
minor comments below.
[...]
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index bb0de406f8e7..ec6e57a0c14e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -102,8 +102,42 @@ static inline void set_max_mapnr(unsigned long limit) { }
> * zeroing by defining this macro in <asm/pgtable.h>.
> */
> #ifndef mm_zero_struct_page
Do we still need this ifdef? I guess we can wait for an arch which
doesn't like this change and then add the override. I would rather go
simple if possible.
> +#if BITS_PER_LONG == 64
> +/* This function must be updated when the size of struct page grows above 80
> + * or reduces below 64. The idea that compiler optimizes out switch()
> + * statement, and only leaves move/store instructions
> + */
> +#define mm_zero_struct_page(pp) __mm_zero_struct_page(pp)
> +static inline void __mm_zero_struct_page(struct page *page)
> +{
> + unsigned long *_pp = (void *)page;
> +
> + /* Check that struct page is either 56, 64, 72, or 80 bytes */
> + BUILD_BUG_ON(sizeof(struct page) & 7);
> + BUILD_BUG_ON(sizeof(struct page) < 56);
> + BUILD_BUG_ON(sizeof(struct page) > 80);
> +
> + switch (sizeof(struct page)) {
> + case 80:
> + _pp[9] = 0; /* fallthrough */
> + case 72:
> + _pp[8] = 0; /* fallthrough */
> + default:
> + _pp[7] = 0; /* fallthrough */
> + case 56:
> + _pp[6] = 0;
> + _pp[5] = 0;
> + _pp[4] = 0;
> + _pp[3] = 0;
> + _pp[2] = 0;
> + _pp[1] = 0;
> + _pp[0] = 0;
> + }
This just hit my eyes. I have to confess I have never seen default: to
be not the last one in the switch. Can we have case 64 instead or does gcc
complain? I would be surprised with the set of BUILD_BUG_ONs.
> +}
> +#else
> #define mm_zero_struct_page(pp) ((void)memset((pp), 0, sizeof(struct page)))
> #endif
> +#endif
>
> /*
> * Default maximum number of active map areas, this limits the number of vmas
>
--
Michal Hocko
SUSE Labs
