Re: [RFC PATCH v1 00/57] Boot-time page size selection for arm64

Ryan Roberts
2024-10-21T11:51:26Z

On 18/10/2024 15:41, Petr Tesarik wrote:
> On Fri, 18 Oct 2024 14:56:00 +0200
> Petr Tesarik <ptesarik@suse.com> wrote:
>
>> On Thu, 17 Oct 2024 13:32:43 +0100
>> Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>>> On 17/10/2024 13:27, Petr Tesarik wrote:
>>>> On Mon, 14 Oct 2024 11:55:11 +0100
>>>> Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>>> [...]
>>>>> The series is arranged as follows:
>>>>>
>>>>> - patch 1: Add macros required for converting non-arch code to support
>>>>> boot-time page size selection
>>>>> - patches 2-36: Remove PAGE_SIZE compile-time constant assumption from all
>>>>> non-arch code
>>>>
>>>> I have just tried to recompile the openSUSE kernel with these patches
>>>> applied, and I'm running into this:
>>>>
>>>> CC arch/arm64/hyperv/hv_core.o
>>>> In file included from ../arch/arm64/hyperv/hv_core.c:14:0:
>>>> ../include/linux/hyperv.h:158:5: error: variably modified ‘reserved2’ at file scope
>>>> u8 reserved2[PAGE_SIZE - 68];
>>>> ^~~~~~~~~
>>>>
>>>> It looks like one more place which needs a patch, right?
>>>
>>> As mentioned in the cover letter, so far I've only converted enough to get the
>>> defconfig *image* building (i.e. no modules). If you are compiling a different
>>> config or compiling the modules for defconfig, you will likely run into these
>>> types of issues.
>>>
>>> That said, I do have some patches to fix Hyper-V, which Michael Kelley was kind
>>> enough to send me.
>>>
>>> I understand that Suse might be able to help with wider performance testing - if
>>> that's the reason you are trying to compile, you could send me your config and
>>> I'll start working on fixing up other drivers?
>>
>> You're right, performance testing is my goal.
>>
>> Heh, the openSUSE master config is cranked up to max. ;-) That would be
>> a lot of work, and we don't need all those options for running our test
>> suite. Let me disable the conflicting options instead.
>> [...]
>> I'll see if I can do something about btrfs. Then I can try to boot the
>> kernel...
>
> FWIW the kernel builds and _boots_ after applying this patch:

Amazing - thanks for doing this!

>
> fs/btrfs/compression.h | 2 +-
> fs/btrfs/defrag.c | 2 +-
> fs/btrfs/extent_io.h | 2 +-
> fs/btrfs/scrub.c | 2 +-
> include/linux/raid/pq.h | 4 ++--
> lib/raid6/algos.c | 2 +-
> 6 files changed, 7 insertions(+), 7 deletions(-)
>
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -33,7 +33,7 @@ struct btrfs_bio;
> /* Maximum length of compressed data stored on disk */
> #define BTRFS_MAX_COMPRESSED (SZ_128K)
> #define BTRFS_MAX_COMPRESSED_PAGES (BTRFS_MAX_COMPRESSED / PAGE_SIZE)
> -static_assert((BTRFS_MAX_COMPRESSED % PAGE_SIZE) == 0);
> +static_assert((BTRFS_MAX_COMPRESSED % PAGE_SIZE_MAX) == 0);
>
> /* Maximum size of data before compression */
> #define BTRFS_MAX_UNCOMPRESSED (SZ_128K)
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -1144,7 +1144,7 @@ next:
> }
>
> #define CLUSTER_SIZE (SZ_256K)
> -static_assert(PAGE_ALIGNED(CLUSTER_SIZE));
> +static_assert(IS_ALIGNED(CLUSTER_SIZE, PAGE_SIZE_MAX));
>
> /*
> * Defrag one contiguous target range.
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -89,7 +89,7 @@ enum {
> int __init extent_buffer_init_cachep(void);
> void __cold extent_buffer_free_cachep(void);
>
> -#define INLINE_EXTENT_BUFFER_PAGES (BTRFS_MAX_METADATA_BLOCKSIZE / PAGE_SIZE)
> +#define INLINE_EXTENT_BUFFER_PAGES (BTRFS_MAX_METADATA_BLOCKSIZE / PAGE_SIZE_MIN)

While this works, I'm not sure if you would want to have 2 separate macros; 1
for worst-case static allocation, and 1 for dynamic allocation and iterating. I
could imagine if you allocate PAGE_SIZE_MAX pages into the worst case number of
slots that would increase memory. I'm not familiar with the code so don't know
if this is a problem in practice. Certainly what you have done is much simpler
if acceptable.

> struct extent_buffer {
> u64 start;
> u32 len;
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -100,7 +100,7 @@ enum scrub_stripe_flags {
> SCRUB_STRIPE_FLAG_NO_REPORT,
> };
>
> -#define SCRUB_STRIPE_PAGES (BTRFS_STRIPE_LEN / PAGE_SIZE)
> +#define SCRUB_STRIPE_PAGES (BTRFS_STRIPE_LEN / PAGE_SIZE_MIN)

Same comment.

Thanks,
Ryan

>
> /*
> * Represent one contiguous range with a length of BTRFS_STRIPE_LEN.
> --- a/include/linux/raid/pq.h
> +++ b/include/linux/raid/pq.h
> @@ -12,7 +12,7 @@
>
> #include <linux/blkdev.h>
>
> -extern const char raid6_empty_zero_page[PAGE_SIZE];
> +extern const char raid6_empty_zero_page[PAGE_SIZE_MAX];
>
> #else /* ! __KERNEL__ */
> /* Used for testing in user space */
> @@ -39,7 +39,7 @@ typedef uint64_t u64;
> #ifndef PAGE_SHIFT
> # define PAGE_SHIFT 12
> #endif
> -extern const char raid6_empty_zero_page[PAGE_SIZE];
> +extern const char raid6_empty_zero_page[PAGE_SIZE_MAX];
>
> #define __init
> #define __exit
> --- a/lib/raid6/algos.c
> +++ b/lib/raid6/algos.c
> @@ -19,7 +19,7 @@
> #include <linux/module.h>
> #include <linux/gfp.h>
> /* In .bss so it's zeroed */
> -const char raid6_empty_zero_page[PAGE_SIZE] __attribute__((aligned(256)));
> +const char raid6_empty_zero_page[PAGE_SIZE_MAX] __attribute__((aligned(256)));
> EXPORT_SYMBOL(raid6_empty_zero_page);
> #endif
>
>
> Petr T