netdev - RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Jun 2017 07:08:54 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "Kalderon, Michal" <Michal.Kalderon@...ium.com>
Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx

> > +struct qed_ll2_tx_pkt_info {
> > +	u8 num_of_bds;
> > +	u16 vlan;
> > +	u8 bd_flags;
> > +	u16 l4_hdr_offset_w;	/* from start of packet */
> > +	enum qed_ll2_tx_dest tx_dest;
> > +	enum qed_ll2_roce_flavor_type qed_roce_flavor;
> > +	dma_addr_t first_frag;
> > +	u16 first_frag_len;
> > +	bool enable_ip_cksum;
> > +	bool enable_l4_cksum;
> > +	bool calc_ip_len;
> > +	void *cookie;
> > +};
> > +
> 
> This layout is extremely inefficient, with lots of padding in between struct
> members.
> 
> Group small u8 members and u16 members together so that they consume
> full 32-bit areas so you can eliminate all of the padding.

While I can mend the holes, the total structure size doesn't really change:

struct qed_ll2_tx_pkt_info {
        u8                         num_of_bds;           /*     0     1 */

        /* XXX 1 byte hole, try to pack */

        u16                        vlan;                 /*     2     2 */
        u8                         bd_flags;             /*     4     1 */

        /* XXX 1 byte hole, try to pack */

        u16                        l4_hdr_offset_w;      /*     6     2 */
        enum qed_ll2_tx_dest       tx_dest;              /*     8     4 */
        enum qed_ll2_roce_flavor_type qed_roce_flavor;   /*    12     4 */
        dma_addr_t                 first_frag;           /*    16     8 */
        u16                        first_frag_len;       /*    24     2 */
        bool                       enable_ip_cksum;      /*    26     1 */
        bool                       enable_l4_cksum;      /*    27     1 */
        bool                       calc_ip_len;          /*    28     1 */

        /* XXX 3 bytes hole, try to pack */

        void *                     cookie;               /*    32     8 */

        /* size: 40, cachelines: 1, members: 12 */
        /* sum members: 35, holes: 3, sum holes: 5 */
        /* last cacheline: 40 bytes */
};

Becomes:

struct qed_ll2_tx_pkt_info {
        void *                     cookie;               /*     0     8 */
        dma_addr_t                 first_frag;           /*     8     8 */
        enum qed_ll2_tx_dest       tx_dest;              /*    16     4 */
        enum qed_ll2_roce_flavor_type qed_roce_flavor;   /*    20     4 */
        u16                        vlan;                 /*    24     2 */
        u16                        l4_hdr_offset_w;      /*    26     2 */
        u16                        first_frag_len;       /*    28     2 */
        u8                         num_of_bds;           /*    30     1 */
        u8                         bd_flags;             /*    31     1 */
        bool                       enable_ip_cksum;      /*    32     1 */
        bool                       enable_l4_cksum;      /*    33     1 */
        bool                       calc_ip_len;          /*    34     1 */

        /* size: 40, cachelines: 1, members: 12 */
        /* padding: 5 */
        /* last cacheline: 40 bytes */
};

I'm going to send the changed version in V2 as as there's no harm to it,
[+ we *can* reduce the size of qed_ll2_comp_rx_data you commented
for patch #2 in series]

But one thing I thought of asking - do we consider layouts of relatively
insignificant structures to be some golden coding standard?