NIC drivers generally try to ensure that the "network header" is aligned to a 4-byte boundary. This is not always possible: When Ethernet frames are encapsulated in other packets with 4-byte aligned headers, the inner Ethernet header will have 4-byte alignment, and in consequence, the inner network header is aligned to 2, but not to 4 bytes.
Most parts of batman-adv only care about 2-byte alignment; in particular, no unaligned accesses occur in performance-critical paths that handle actual payload data. This is not true for OGM handling: the seqno and crc fields are accessed as 32-bit values. To avoid these unaligned accesses, this patch reduces the expected packet alignment to 2 bytes for all of batadv's packet types.
As no unaligned accesses existed on the performance-critical paths anyways, this chance does have any (positive or negative) effect on performance, but it still makes sense to avoid these accesses to prevent log noise when examining other unaligned accesses in the kernel while batman-adv is active.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net --- include/uapi/linux/batadv_packet.h | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h index daefd728..894d8d2f 100644 --- a/include/uapi/linux/batadv_packet.h +++ b/include/uapi/linux/batadv_packet.h @@ -196,8 +196,6 @@ struct batadv_bla_claim_dst { __be16 group; /* group id */ };
-#pragma pack() - /** * struct batadv_ogm_packet - ogm (routing protocol) packet * @packet_type: batman-adv packet type, part of the general header @@ -222,9 +220,6 @@ struct batadv_ogm_packet { __u8 reserved; __u8 tq; __be16 tvlv_len; - /* __packed is not needed as the struct size is divisible by 4, - * and the largest data type in this struct has a size of 4. - */ };
#define BATADV_OGM_HLEN sizeof(struct batadv_ogm_packet) @@ -249,9 +244,6 @@ struct batadv_ogm2_packet { __u8 orig[ETH_ALEN]; __be16 tvlv_len; __be32 throughput; - /* __packed is not needed as the struct size is divisible by 4, - * and the largest data type in this struct has a size of 4. - */ };
#define BATADV_OGM2_HLEN sizeof(struct batadv_ogm2_packet) @@ -405,7 +397,6 @@ struct batadv_icmp_packet_rr { * misalignment of the payload after the ethernet header. It may also lead to * leakage of information when the padding it not initialized before sending. */ -#pragma pack(2)
/** * struct batadv_unicast_packet - unicast packet for network payload @@ -533,8 +524,6 @@ struct batadv_coded_packet { __be16 coded_len; };
-#pragma pack() - /** * struct batadv_unicast_tvlv_packet - generic unicast packet with tvlv payload * @packet_type: batman-adv packet type, part of the general header @@ -641,4 +630,6 @@ struct batadv_tvlv_mcast_data { __u8 reserved[3]; };
+#pragma pack() + #endif /* _UAPI_LINUX_BATADV_PACKET_H_ */
On Mittwoch, 24. Januar 2018 12:21:37 CET Matthias Schiffer wrote:
NIC drivers generally try to ensure that the "network header" is aligned to a 4-byte boundary. This is not always possible: When Ethernet frames are encapsulated in other packets with 4-byte aligned headers, the inner Ethernet header will have 4-byte alignment, and in consequence, the inner network header is aligned to 2, but not to 4 bytes.
Most parts of batman-adv only care about 2-byte alignment; in particular, no unaligned accesses occur in performance-critical paths that handle actual payload data. This is not true for OGM handling: the seqno and crc fields are accessed as 32-bit values. To avoid these unaligned accesses, this patch reduces the expected packet alignment to 2 bytes for all of batadv's packet types.
As no unaligned accesses existed on the performance-critical paths anyways, this chance does have any (positive or negative) effect on performance, but it still makes sense to avoid these accesses to prevent log noise when examining other unaligned accesses in the kernel while batman-adv is active.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net
include/uapi/linux/batadv_packet.h | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
I know your intentions and I understand the problem. But there is the chance that David Miller will reject this patch - like he did it some years ago with a similar (not the same) patch:
"I'm not applying this, please try work to implement this more acceptably first." [1]
But maybe he has now some other opinion because the unaligned problem is caused by the encapsulation in VXLAN or maybe he has a better idea. At least VXLAN encap stuff should affect a lot more net code than batman-adv.
Kind regards, Sven
[1] https://patchwork.ozlabs.org/patch/295596/
diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h index daefd728..894d8d2f 100644 --- a/include/uapi/linux/batadv_packet.h +++ b/include/uapi/linux/batadv_packet.h @@ -196,8 +196,6 @@ struct batadv_bla_claim_dst { __be16 group; /* group id */ };
-#pragma pack()
/**
- struct batadv_ogm_packet - ogm (routing protocol) packet
- @packet_type: batman-adv packet type, part of the general header
@@ -222,9 +220,6 @@ struct batadv_ogm_packet { __u8 reserved; __u8 tq; __be16 tvlv_len;
- /* __packed is not needed as the struct size is divisible by 4,
* and the largest data type in this struct has a size of 4.*/};
#define BATADV_OGM_HLEN sizeof(struct batadv_ogm_packet) @@ -249,9 +244,6 @@ struct batadv_ogm2_packet { __u8 orig[ETH_ALEN]; __be16 tvlv_len; __be32 throughput;
- /* __packed is not needed as the struct size is divisible by 4,
* and the largest data type in this struct has a size of 4.*/};
#define BATADV_OGM2_HLEN sizeof(struct batadv_ogm2_packet) @@ -405,7 +397,6 @@ struct batadv_icmp_packet_rr {
- misalignment of the payload after the ethernet header. It may also lead to
- leakage of information when the padding it not initialized before sending.
*/ -#pragma pack(2)
/**
- struct batadv_unicast_packet - unicast packet for network payload
@@ -533,8 +524,6 @@ struct batadv_coded_packet { __be16 coded_len; };
-#pragma pack()
/**
- struct batadv_unicast_tvlv_packet - generic unicast packet with tvlv payload
- @packet_type: batman-adv packet type, part of the general header
@@ -641,4 +630,6 @@ struct batadv_tvlv_mcast_data { __u8 reserved[3]; };
+#pragma pack()
#endif /* _UAPI_LINUX_BATADV_PACKET_H_ */
On Mittwoch, 24. Januar 2018 14:40:03 CET Sven Eckelmann wrote: [...]
I know your intentions and I understand the problem. But there is the chance that David Miller will reject this patch - like he did it some years ago with a similar (not the same) patch:
"I'm not applying this, please try work to implement this more acceptably first." [1]But maybe he has now some other opinion because the unaligned problem is caused by the encapsulation in VXLAN or maybe he has a better idea. At least VXLAN encap stuff should affect a lot more net code than batman-adv.
[...]
Looks like we have to assume that David has nothing against the patch and we should get the patch integrated.
Affected are any kind of access to the 32 bit values:
* &batadv_ogm_packet->seqno * &batadv_ogm2_packet->seqno * &batadv_ogm2_packet->throughput * &batadv_elp_packet->seqno * &batadv_elp_packet->elp_interval * &batadv_icmp_tp_packet->seqno * &batadv_icmp_tp_packet->timestamp * &batadv_tvlv_gateway_data->bandwidth_down * &batadv_tvlv_gateway_data->bandwidth_up * &batadv_tvlv_tt_vlan_data->crc
I personally would love to hear that there is a better way to fix this problem. But either this or something like the (out of tree) hacks from OpenWrt [1] seem to be required to work around this problem.
Kind regards, Sven
[1] https://github.com/openwrt/openwrt/blob/master/target/linux/ar71xx/patches-4...
On Mittwoch, 24. Januar 2018 12:21:37 CET Matthias Schiffer wrote:
NIC drivers generally try to ensure that the "network header" is aligned to a 4-byte boundary. This is not always possible: When Ethernet frames are encapsulated in other packets with 4-byte aligned headers, the inner Ethernet header will have 4-byte alignment, and in consequence, the inner network header is aligned to 2, but not to 4 bytes.
Most parts of batman-adv only care about 2-byte alignment; in particular, no unaligned accesses occur in performance-critical paths that handle actual payload data. This is not true for OGM handling: the seqno and crc fields are accessed as 32-bit values. To avoid these unaligned accesses, this patch reduces the expected packet alignment to 2 bytes for all of batadv's packet types.
As no unaligned accesses existed on the performance-critical paths anyways, this chance does have any (positive or negative) effect on performance, but it still makes sense to avoid these accesses to prevent log noise when examining other unaligned accesses in the kernel while batman-adv is active.
Signed-off-by: Matthias Schiffer mschiffer@universe-factory.net
include/uapi/linux/batadv_packet.h | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
As discussed earlier, this was applied as 0509fc0c128b [1] after David Miller pulled it in net-next.
Kind regards, Sven
[1] https://git.open-mesh.org/batman-adv.git/commit/0509fc0c128ba2891770305c57c9...
b.a.t.m.a.n@lists.open-mesh.org
-
Matthias Schiffer -
Sven Eckelmann