| Message ID | 20201028151637.1734130-4-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: renesas: Cleanups and improvements | expand |
On Wed, Oct 28, 2020 at 4:16 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > On arm64, pointer size and alignment is 64-bit, hence a 4-byte hole is > present in between the enum_id and name members of the sh_pfc_pin > structure. Get rid of this hole by sorting the structure's members by > decreasing size. > > This saves up to 1.5 KiB per enabled SoC, and reduces the size of a > kernel including support for all R-Car Gen3 SoCs by more than 10 KiB. > > This has no size impact on SH and arm32. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/pinctrl/renesas/sh_pfc.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/renesas/sh_pfc.h b/drivers/pinctrl/renesas/sh_pfc.h > index eff1bb872325ef3a..3b390dffacb4910d 100644 > --- a/drivers/pinctrl/renesas/sh_pfc.h > +++ b/drivers/pinctrl/renesas/sh_pfc.h > @@ -34,10 +34,10 @@ enum { > #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) > > struct sh_pfc_pin { > - u16 pin; > - u16 enum_id; > const char *name; > unsigned int configs; > + u16 pin; > + u16 enum_id; > }; Hehehe :D The compiler people have something that is called "premature optimization" which is when you try to outsmart the compiler. So since you have metrics on this you have obviously outsmarted the ARM64 compiler (I guess GCC). What I'm thinking is that some compiler person should look at this and say that "yeah sometimes you have to do that". In this case I suppose the compiler really isn't allowed to reshuffle struct members in memory since there is plenty of code that relies on them being laid out strictly in the order they are defined into the struct. So this is really necessary. Second I think it warrants a comment in the code to be careful with aligning structs on 64bit boundaries? Yours, Linus Walleij
Hi Linus, On Thu, Nov 5, 2020 at 10:52 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Oct 28, 2020 at 4:16 PM Geert Uytterhoeven > <geert+renesas@glider.be> wrote: > > On arm64, pointer size and alignment is 64-bit, hence a 4-byte hole is > > present in between the enum_id and name members of the sh_pfc_pin > > structure. Get rid of this hole by sorting the structure's members by > > decreasing size. > > > > This saves up to 1.5 KiB per enabled SoC, and reduces the size of a > > kernel including support for all R-Car Gen3 SoCs by more than 10 KiB. > > > > This has no size impact on SH and arm32. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/pinctrl/renesas/sh_pfc.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pinctrl/renesas/sh_pfc.h b/drivers/pinctrl/renesas/sh_pfc.h > > index eff1bb872325ef3a..3b390dffacb4910d 100644 > > --- a/drivers/pinctrl/renesas/sh_pfc.h > > +++ b/drivers/pinctrl/renesas/sh_pfc.h > > @@ -34,10 +34,10 @@ enum { > > #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) > > > > struct sh_pfc_pin { > > - u16 pin; > > - u16 enum_id; > > const char *name; > > unsigned int configs; > > + u16 pin; > > + u16 enum_id; > > }; > > Hehehe :D > > The compiler people have something that is called "premature optimization" > which is when you try to outsmart the compiler. > > So since you have metrics on this you have obviously outsmarted the > ARM64 compiler (I guess GCC). > > What I'm thinking is that some compiler person should look at this > and say that "yeah sometimes you have to do that". In this case > I suppose the compiler really isn't allowed to reshuffle struct members > in memory since there is plenty of code that relies on them being > laid out strictly in the order they are defined into the struct. So this > is really necessary. The compiler is not allowed to reorder the members (FWIW, this might be a description of hardware register layout). > Second I think it warrants a comment in the code to be careful with > aligning structs on 64bit boundaries? IMHO that's overkill: if we go that route, we have to add such comments to every structure that contains members of different sizes... Gr{oetje,eeting}s, Geert
diff --git a/drivers/pinctrl/renesas/sh_pfc.h b/drivers/pinctrl/renesas/sh_pfc.h index eff1bb872325ef3a..3b390dffacb4910d 100644 --- a/drivers/pinctrl/renesas/sh_pfc.h +++ b/drivers/pinctrl/renesas/sh_pfc.h @@ -34,10 +34,10 @@ enum { #define SH_PFC_PIN_CFG_NO_GPIO (1 << 31) struct sh_pfc_pin { - u16 pin; - u16 enum_id; const char *name; unsigned int configs; + u16 pin; + u16 enum_id; }; #define SH_PFC_PIN_GROUP_ALIAS(alias, n) \
On arm64, pointer size and alignment is 64-bit, hence a 4-byte hole is present in between the enum_id and name members of the sh_pfc_pin structure. Get rid of this hole by sorting the structure's members by decreasing size. This saves up to 1.5 KiB per enabled SoC, and reduces the size of a kernel including support for all R-Car Gen3 SoCs by more than 10 KiB. This has no size impact on SH and arm32. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/pinctrl/renesas/sh_pfc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)