Re: [B.A.T.M.A.N.] [RFC] batman-adv: always assume 2-byte packet alignment

[Sven Eckelmann <sven@narfation.org>]

[-- 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 --]