From: Yuval Mintz <Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Date: Thu, 8 Jun 2017 19:13:16 +0300
quoted hunk
@@ -67,6 +79,21 @@ struct qed_ll2_stats {
u64 sent_bcast_pkts;
};
+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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
quoted
+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?
From: David Miller
Sent: 09 June 2017 00:24
From: Yuval Mintz <redacted>
Date: Thu, 8 Jun 2017 19:13:16 +0300
quoted
@@ -67,6 +79,21 @@ struct qed_ll2_stats {
u64 sent_bcast_pkts;
};
+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.
I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8)
variables is likely to generate extra code (on non-x86).
You are using 32 bits for the 'enum' - I bet the values fit in 8 bits,
so aren't really worried about size.
If size did matter you can easily get the above down to 32 bytes.
David
-----Original Message-----
From: David Laight [mailto:David.Laight@ACULAB.COM]
Sent: Friday, June 09, 2017 10:28 AM
To: 'David Miller' <davem@davemloft.net>; Mintz, Yuval
[off-list ref]
Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Kalderon, Michal
[off-list ref]
Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: David Miller
quoted
Sent: 09 June 2017 00:24
From: Yuval Mintz <redacted>
Date: Thu, 8 Jun 2017 19:13:16 +0300
quoted
@@ -67,6 +79,21 @@ struct qed_ll2_stats {
u64 sent_bcast_pkts;
};
+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.
I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8)
variables is likely to generate extra code (on non-x86).
You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so aren't
really worried about size.
If size did matter you can easily get the above down to 32 bytes.
You're right, and that's exactly the point - since this is not data-path critical
I don't see why the size/efficiency should matter [greatly].