Re: [PATCH 1/2] device.h: pack struct dev_links_info

[Greg Kroah-Hartman]

On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > The dev_links_info structure has 4 bytes of padding at the end of it
> > when embedded in struct device (which is the only place it lives).  To
> > help reduce the size of struct device pack this structure so we can take
> > advantage of the hole with later structure reorganizations.
> > 
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  include/linux/device.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 6cb4640b6160..b63165276a09 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -884,7 +884,7 @@ struct dev_links_info {
> >  	struct list_head suppliers;
> >  	struct list_head consumers;
> >  	enum dl_dev_state status;
> > -};
> > +} __packed;
> 
> This seems like a bad idea. You're changing the alignment of these
> fields to one byte, something which may cause the compiler to generate
> less efficient code to deal with unaligned accesses (even if they happen
> to currently be naturally aligned in struct device).

No, all this changes is the trailing "space" is gone.  The alignment of
the fields did not change at all as they are all naturally aligned
(list_head is just 2 pointers).

Here's the pahole output before and after this patch:

Before:
struct dev_links_info {
        struct list_head           suppliers;            /*     0    16 */
        struct list_head           consumers;            /*    16    16 */
        enum dl_dev_state          status;               /*    32     4 */

        /* size: 40, cachelines: 1, members: 3 */
        /* padding: 4 */
        /* last cacheline: 40 bytes */
};

After:
struct dev_links_info {
        struct list_head           suppliers;            /*     0    16 */
        struct list_head           consumers;            /*    16    16 */
        enum dl_dev_state          status;               /*    32     4 */

        /* size: 36, cachelines: 1, members: 3 */
        /* last cacheline: 36 bytes */
};


So this allows us to save 4 bytes in struct device by putting something in that
trailing "hole" that can be aligned with it better (i.e. an integer or
something else).

thanks,

greg k-h