| Message ID | 20191224141032.34544-1-abrodkin@synopsys.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | arch/arc: Explicitly set "max-page-size" for GNU LD | expand |
On Tue, 24 Dec 2019 17:10:32 +0300 Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote: > Back in the day we relied on a default value that used to be 8KiB > and it worked perfectly fine for ARC's default 8KiB page as well as > 4 KiB ones, but not for 16 KiB, see [1] for more details. > > So that we fixed by setting "max-page-size" if 16KiB pages are in use by > commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages"). > > But as Yann very rightfully mentioned here [2] we should be setting this > thing explicitly for all page sizes because: > 1. Defaults might change unexpectedly > 2. Explicitly set stuff is better understood > 3. We act similarly to all settings but not only addressing some corner cases > > [1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0 > [2] https://patchwork.ozlabs.org/patch/1212544/#2330647 > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Yann E. MORIN <yann.morin.1998@free.fr> I have applied to master, with one change (see below), and I have one suggestion (see below as well). > -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages > -# it needs to be overridden. > +# Explicitly set LD's "max-page-size" instead of relying on some defaults > +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 > +endif > + > +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 > +endif > + > ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 > endif Since these options are mutually exclusive: ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 endif is a bit more compact, so I changed to use this before committing. Also, we already have an option called BR2_ARC_PAGE_SIZE, which contains 4K, 8K or 16K, so I thought if we could use it here as well. I think we can, but I'd like to hear from Yann about this, because it uses bash arithmetic, which I'm not sure we're allowed to use: ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024))) Best regards, Thomas
Thomas, Alexey, All, On 2019-12-25 22:36 +0100, Thomas Petazzoni spake thusly: > On Tue, 24 Dec 2019 17:10:32 +0300 > Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote: > > Back in the day we relied on a default value that used to be 8KiB > > and it worked perfectly fine for ARC's default 8KiB page as well as > > 4 KiB ones, but not for 16 KiB, see [1] for more details. > > > > So that we fixed by setting "max-page-size" if 16KiB pages are in use by > > commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages"). > > > > But as Yann very rightfully mentioned here [2] we should be setting this > > thing explicitly for all page sizes because: > > 1. Defaults might change unexpectedly > > 2. Explicitly set stuff is better understood > > 3. We act similarly to all settings but not only addressing some corner cases > > > > [1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0 > > [2] https://patchwork.ozlabs.org/patch/1212544/#2330647 > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Yann E. MORIN <yann.morin.1998@free.fr> > > > -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages > > -# it needs to be overridden. > > +# Explicitly set LD's "max-page-size" instead of relying on some defaults > > +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 > > +endif > > + > > +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 > > +endif > > + > > ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 > > endif > > Since these options are mutually exclusive: > > ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 > else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 > else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 > endif > > is a bit more compact, so I changed to use this before committing. Agreed. > Also, we already have an option called BR2_ARC_PAGE_SIZE, which > contains 4K, 8K or 16K, so I thought if we could use it here as well. I > think we can, but I'd like to hear from Yann about this, because it > uses bash arithmetic, which I'm not sure we're allowed to use: > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024))) We can do bash arithmetics, because we are guaranteed we use bash as our shell, see: https://git.buildroot.org/buildroot/tree/Makefile#n30 But BR2_ARC_PAGE_SIZE is a string, so it is double-quoted, which would break the above. I was also contemplating a similar solution, but I was not too happy with this trick to drop the suffix. I would have prefered something like: config BR2_ARC_PAGE_SIZE_KB int default 4 if BR2_ARC_PAGE_SIZE_4K default 8 if BR2_ARC_PAGE_SIZE_8K default 16 if BR2_ARC_PAGE_SIZE_16K and then: ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024))) and in package/uclibc/uclibc.mk: UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K ... and even that intermediate variable could have been dropped altogether, to directly write: $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config) And that would have been a bit nicer, I believe. Regards, Yann E. MORIN. > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Yann, Thomas, [snip] > > > -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages > > > -# it needs to be overridden. > > > +# Explicitly set LD's "max-page-size" instead of relying on some defaults > > > +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) > > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 > > > +endif > > > + > > > +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) > > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 > > > +endif > > > + > > > ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) > > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 > > > endif > > > > Since these options are mutually exclusive: > > > > ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 > > else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 > > else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 > > endif > > > > is a bit more compact, so I changed to use this before committing. > > Agreed. > > > Also, we already have an option called BR2_ARC_PAGE_SIZE, which > > contains 4K, 8K or 16K, so I thought if we could use it here as well. I > > think we can, but I'd like to hear from Yann about this, because it > > uses bash arithmetic, which I'm not sure we're allowed to use: > > > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst > %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024))) > > We can do bash arithmetics, because we are guaranteed we use bash as our > shell, see: https://urldefense.proofpoint.com/v2/url?u=https- > 3A__git.buildroot.org_buildroot_tree_Makefile- > 23n30&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=Z74rKOsEpOUneCxYxwKcXHwrAmW8mJhNZjcWnDDJI2U&m=G0xH- > XYcb2iYOg2bibx_AguTrgrtfR3aD7pVbOW09Q0&s=_K73mNAFEXxJcdkCnKJRzs-VXkSvcFKryO2ZYmXj1LI&e= > > But BR2_ARC_PAGE_SIZE is a string, so it is double-quoted, which would > break the above. > > I was also contemplating a similar solution, but I was not too happy > with this trick to drop the suffix. I would have prefered something > like: > > config BR2_ARC_PAGE_SIZE_KB > int > default 4 if BR2_ARC_PAGE_SIZE_4K > default 8 if BR2_ARC_PAGE_SIZE_8K > default 16 if BR2_ARC_PAGE_SIZE_16K > > and then: > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024))) > > and in package/uclibc/uclibc.mk: > > UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K > > ... and even that intermediate variable could have been dropped > altogether, to directly write: > > $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config) > > And that would have been a bit nicer, I believe. Agree. So, Yann [as usually] are you OK if I implement that improvement? -Alexey
Alexey, All, On 2019-12-27 09:00 +0000, Alexey Brodkin spake thusly: [--SNIP--] > > I was also contemplating a similar solution, but I was not too happy > > with this trick to drop the suffix. I would have prefered something > > like: > > > > config BR2_ARC_PAGE_SIZE_KB > > int > > default 4 if BR2_ARC_PAGE_SIZE_4K > > default 8 if BR2_ARC_PAGE_SIZE_8K > > default 16 if BR2_ARC_PAGE_SIZE_16K > > > > and then: > > > > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024))) > > > > and in package/uclibc/uclibc.mk: > > > > UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K > > > > ... and even that intermediate variable could have been dropped > > altogether, to directly write: > > > > $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config) > > > > And that would have been a bit nicer, I believe. > > Agree. So, Yann [as usually] are you OK if I implement that improvement? As you wish! ;-) Regards, Yann E. MORIN.
diff --git a/arch/arch.mk.arc b/arch/arch.mk.arc index 9c04a85a8d..7eb1b95aa7 100644 --- a/arch/arch.mk.arc +++ b/arch/arch.mk.arc @@ -5,8 +5,15 @@ ifeq ($(BR2_ARC_ATOMIC_EXT),y) ARCH_TOOLCHAIN_WRAPPER_OPTS = -matomic endif -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages -# it needs to be overridden. +# Explicitly set LD's "max-page-size" instead of relying on some defaults +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y) +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096 +endif + +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y) +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192 +endif + ifeq ($(BR2_ARC_PAGE_SIZE_16K),y) ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384 endif
Back in the day we relied on a default value that used to be 8KiB and it worked perfectly fine for ARC's default 8KiB page as well as 4 KiB ones, but not for 16 KiB, see [1] for more details. So that we fixed by setting "max-page-size" if 16KiB pages are in use by commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages"). But as Yann very rightfully mentioned here [2] we should be setting this thing explicitly for all page sizes because: 1. Defaults might change unexpectedly 2. Explicitly set stuff is better understood 3. We act similarly to all settings but not only addressing some corner cases [1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0 [2] https://patchwork.ozlabs.org/patch/1212544/#2330647 Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Yann E. MORIN <yann.morin.1998@free.fr> --- arch/arch.mk.arc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)