Re: [PATCH 3/8] pinctrl: renesas: Reorder struct sh_pfc_pin to remove hole

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Linus,

On Thu, Nov 5, 2020 at 10:52 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Oct 28, 2020 at 4:16 PM Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds