* Re: [RFC] batman-adv: always assume 2-byte packet alignment
[not found] <0272f809d25b41fcab4ec5c4b66014f5e341d424.1516792897.git.mschiffer@universe-factory.net>
@ 2018-01-24 13:40 ` Sven Eckelmann
2018-02-03 8:40 ` [B.A.T.M.A.N.] " Sven Eckelmann
0 siblings, 1 reply; 2+ messages in thread
From: Sven Eckelmann @ 2018-01-24 13:40 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Matthias Schiffer, David Miller, netdev
[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]
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_ */
>
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread