[dpdk-dev] net: introduce big and little endian types

  This commit introduces new rte_{le,be}{16,32,64}_t types and updates
rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
accordingly.

Specific big/little endian types avoid uncertainty and conversion mistakes.

No ABI change since these are simply typedefs to the original types.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 .../common/include/generic/rte_byteorder.h         | 31 +++++++++++-------
 lib/librte_net/rte_arp.h                           | 15 +++++----
 lib/librte_net/rte_ether.h                         | 10 +++---
 lib/librte_net/rte_gre.h                           | 30 ++++++++---------
 lib/librte_net/rte_icmp.h                          | 11 ++++---
 lib/librte_net/rte_ip.h                            | 38 +++++++++++-----------
 lib/librte_net/rte_net.c                           | 10 +++---
 lib/librte_net/rte_sctp.h                          |  9 ++---
 lib/librte_net/rte_tcp.h                           | 19 ++++++-----
 lib/librte_net/rte_udp.h                           |  9 ++---
 10 files changed, 97 insertions(+), 85 deletions(-)
  

Hi Neilo,

> 
> This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> accordingly.
> 
> Specific big/little endian types avoid uncertainty and conversion mistakes.
> 
> No ABI change since these are simply typedefs to the original types.

It seems like quite a lot of changes...
Could you probably explain what will be the benefit in return?
Konstantin

> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  .../common/include/generic/rte_byteorder.h         | 31 +++++++++++-------
>  lib/librte_net/rte_arp.h                           | 15 +++++----
>  lib/librte_net/rte_ether.h                         | 10 +++---
>  lib/librte_net/rte_gre.h                           | 30 ++++++++---------
>  lib/librte_net/rte_icmp.h                          | 11 ++++---
>  lib/librte_net/rte_ip.h                            | 38 +++++++++++-----------
>  lib/librte_net/rte_net.c                           | 10 +++---
>  lib/librte_net/rte_sctp.h                          |  9 ++---
>  lib/librte_net/rte_tcp.h                           | 19 ++++++-----
>  lib/librte_net/rte_udp.h                           |  9 ++---
>  10 files changed, 97 insertions(+), 85 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
> index e00bccb..059c2a5 100644
> --- a/lib/librte_eal/common/include/generic/rte_byteorder.h
> +++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
> @@ -75,6 +75,13 @@
>  #define RTE_BYTE_ORDER RTE_LITTLE_ENDIAN
>  #endif
> 
> +typedef uint16_t rte_be16_t;
> +typedef uint32_t rte_be32_t;
> +typedef uint64_t rte_be64_t;
> +typedef uint16_t rte_le16_t;
> +typedef uint32_t rte_le32_t;
> +typedef uint64_t rte_le64_t;
> +
>  /*
>   * An internal function to swap bytes in a 16-bit value.
>   *
> @@ -143,65 +150,65 @@ static uint64_t rte_bswap64(uint64_t x);
>  /**
>   * Convert a 16-bit value from CPU order to little endian.
>   */
> -static uint16_t rte_cpu_to_le_16(uint16_t x);
> +static rte_le16_t rte_cpu_to_le_16(uint16_t x);
> 
>  /**
>   * Convert a 32-bit value from CPU order to little endian.
>   */
> -static uint32_t rte_cpu_to_le_32(uint32_t x);
> +static rte_le32_t rte_cpu_to_le_32(uint32_t x);
> 
>  /**
>   * Convert a 64-bit value from CPU order to little endian.
>   */
> -static uint64_t rte_cpu_to_le_64(uint64_t x);
> +static rte_le64_t rte_cpu_to_le_64(uint64_t x);
> 
> 
>  /**
>   * Convert a 16-bit value from CPU order to big endian.
>   */
> -static uint16_t rte_cpu_to_be_16(uint16_t x);
> +static rte_be16_t rte_cpu_to_be_16(uint16_t x);
> 
>  /**
>   * Convert a 32-bit value from CPU order to big endian.
>   */
> -static uint32_t rte_cpu_to_be_32(uint32_t x);
> +static rte_be32_t rte_cpu_to_be_32(uint32_t x);
> 
>  /**
>   * Convert a 64-bit value from CPU order to big endian.
>   */
> -static uint64_t rte_cpu_to_be_64(uint64_t x);
> +static rte_be64_t rte_cpu_to_be_64(uint64_t x);
> 
> 
>  /**
>   * Convert a 16-bit value from little endian to CPU order.
>   */
> -static uint16_t rte_le_to_cpu_16(uint16_t x);
> +static uint16_t rte_le_to_cpu_16(rte_le16_t x);
> 
>  /**
>   * Convert a 32-bit value from little endian to CPU order.
>   */
> -static uint32_t rte_le_to_cpu_32(uint32_t x);
> +static uint32_t rte_le_to_cpu_32(rte_le32_t x);
> 
>  /**
>   * Convert a 64-bit value from little endian to CPU order.
>   */
> -static uint64_t rte_le_to_cpu_64(uint64_t x);
> +static uint64_t rte_le_to_cpu_64(rte_le64_t x);
> 
> 
>  /**
>   * Convert a 16-bit value from big endian to CPU order.
>   */
> -static uint16_t rte_be_to_cpu_16(uint16_t x);
> +static uint16_t rte_be_to_cpu_16(rte_be16_t x);
> 
>  /**
>   * Convert a 32-bit value from big endian to CPU order.
>   */
> -static uint32_t rte_be_to_cpu_32(uint32_t x);
> +static uint32_t rte_be_to_cpu_32(rte_be32_t x);
> 
>  /**
>   * Convert a 64-bit value from big endian to CPU order.
>   */
> -static uint64_t rte_be_to_cpu_64(uint64_t x);
> +static uint64_t rte_be_to_cpu_64(rte_be64_t x);
> 
>  #endif /* __DOXYGEN__ */
> 
> diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
> index 1836418..95f123e 100644
> --- a/lib/librte_net/rte_arp.h
> +++ b/lib/librte_net/rte_arp.h
> @@ -40,6 +40,7 @@
> 
>  #include <stdint.h>
>  #include <rte_ether.h>
> +#include <rte_byteorder.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -50,22 +51,22 @@ extern "C" {
>   */
>  struct arp_ipv4 {
>  	struct ether_addr arp_sha;  /**< sender hardware address */
> -	uint32_t          arp_sip;  /**< sender IP address */
> +	rte_be32_t        arp_sip;  /**< sender IP address */
>  	struct ether_addr arp_tha;  /**< target hardware address */
> -	uint32_t          arp_tip;  /**< target IP address */
> +	rte_be32_t        arp_tip;  /**< target IP address */
>  } __attribute__((__packed__));
> 
>  /**
>   * ARP header.
>   */
>  struct arp_hdr {
> -	uint16_t arp_hrd;    /* format of hardware address */
> +	rte_be16_t arp_hrd;  /* format of hardware address */
>  #define ARP_HRD_ETHER     1  /* ARP Ethernet address format */
> 
> -	uint16_t arp_pro;    /* format of protocol address */
> -	uint8_t  arp_hln;    /* length of hardware address */
> -	uint8_t  arp_pln;    /* length of protocol address */
> -	uint16_t arp_op;     /* ARP opcode (command) */
> +	rte_be16_t arp_pro;  /* format of protocol address */
> +	uint8_t    arp_hln;  /* length of hardware address */
> +	uint8_t    arp_pln;  /* length of protocol address */
> +	rte_be16_t arp_op;   /* ARP opcode (command) */
>  #define	ARP_OP_REQUEST    1 /* request to resolve address */
>  #define	ARP_OP_REPLY      2 /* response to previous request */
>  #define	ARP_OP_REVREQUEST 3 /* request proto addr given hardware */
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index ff3d065..159e061 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -300,7 +300,7 @@ ether_format_addr(char *buf, uint16_t size,
>  struct ether_hdr {
>  	struct ether_addr d_addr; /**< Destination address. */
>  	struct ether_addr s_addr; /**< Source address. */
> -	uint16_t ether_type;      /**< Frame type. */
> +	rte_be16_t ether_type;    /**< Frame type. */
>  } __attribute__((__packed__));
> 
>  /**
> @@ -309,8 +309,8 @@ struct ether_hdr {
>   * of the encapsulated frame.
>   */
>  struct vlan_hdr {
> -	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
> -	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
> +	rte_be16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
> +	rte_be16_t eth_proto;/**< Ethernet type of encapsulated frame. */
>  } __attribute__((__packed__));
> 
>  /**
> @@ -319,8 +319,8 @@ struct vlan_hdr {
>   * Reserved fields (24 bits and 8 bits)
>   */
>  struct vxlan_hdr {
> -	uint32_t vx_flags; /**< flag (8) + Reserved (24). */
> -	uint32_t vx_vni;   /**< VNI (24) + Reserved (8). */
> +	rte_be32_t vx_flags; /**< flag (8) + Reserved (24). */
> +	rte_be32_t vx_vni;   /**< VNI (24) + Reserved (8). */
>  } __attribute__((__packed__));
> 
>  /* Ethernet frame types */
> diff --git a/lib/librte_net/rte_gre.h b/lib/librte_net/rte_gre.h
> index 46568ff..b651af0 100644
> --- a/lib/librte_net/rte_gre.h
> +++ b/lib/librte_net/rte_gre.h
> @@ -45,23 +45,23 @@ extern "C" {
>   */
>  struct gre_hdr {
>  #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> -	uint16_t res2:4; /**< Reserved */
> -	uint16_t s:1;    /**< Sequence Number Present bit */
> -	uint16_t k:1;    /**< Key Present bit */
> -	uint16_t res1:1; /**< Reserved */
> -	uint16_t c:1;    /**< Checksum Present bit */
> -	uint16_t ver:3;  /**< Version Number */
> -	uint16_t res3:5; /**< Reserved */
> +	uint16_t res2:4;  /**< Reserved */
> +	uint16_t s:1;     /**< Sequence Number Present bit */
> +	uint16_t k:1;     /**< Key Present bit */
> +	uint16_t res1:1;  /**< Reserved */
> +	uint16_t c:1;     /**< Checksum Present bit */
> +	uint16_t ver:3;   /**< Version Number */
> +	uint16_t res3:5;  /**< Reserved */
>  #elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> -	uint16_t c:1;    /**< Checksum Present bit */
> -	uint16_t res1:1; /**< Reserved */
> -	uint16_t k:1;    /**< Key Present bit */
> -	uint16_t s:1;    /**< Sequence Number Present bit */
> -	uint16_t res2:4; /**< Reserved */
> -	uint16_t res3:5; /**< Reserved */
> -	uint16_t ver:3;  /**< Version Number */
> +	uint16_t c:1;     /**< Checksum Present bit */
> +	uint16_t res1:1;  /**< Reserved */
> +	uint16_t k:1;     /**< Key Present bit */
> +	uint16_t s:1;     /**< Sequence Number Present bit */
> +	uint16_t res2:4;  /**< Reserved */
> +	uint16_t res3:5;  /**< Reserved */
> +	uint16_t ver:3;   /**< Version Number */
>  #endif
> -	uint16_t proto;  /**< Protocol Type */
> +	rte_be16_t proto; /**< Protocol Type */
>  } __attribute__((__packed__));
> 
>  #ifdef __cplusplus
> diff --git a/lib/librte_net/rte_icmp.h b/lib/librte_net/rte_icmp.h
> index 8b287f6..81bd907 100644
> --- a/lib/librte_net/rte_icmp.h
> +++ b/lib/librte_net/rte_icmp.h
> @@ -74,6 +74,7 @@
>   */
> 
>  #include <stdint.h>
> +#include <rte_byteorder.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -83,11 +84,11 @@ extern "C" {
>   * ICMP Header
>   */
>  struct icmp_hdr {
> -	uint8_t  icmp_type;   /* ICMP packet type. */
> -	uint8_t  icmp_code;   /* ICMP packet code. */
> -	uint16_t icmp_cksum;  /* ICMP packet checksum. */
> -	uint16_t icmp_ident;  /* ICMP packet identifier. */
> -	uint16_t icmp_seq_nb; /* ICMP packet sequence number. */
> +	uint8_t  icmp_type;     /* ICMP packet type. */
> +	uint8_t  icmp_code;     /* ICMP packet code. */
> +	rte_be16_t icmp_cksum;  /* ICMP packet checksum. */
> +	rte_be16_t icmp_ident;  /* ICMP packet identifier. */
> +	rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */
>  } __attribute__((__packed__));
> 
>  /* ICMP packet types */
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> index 4491b86..6f7da36 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -93,14 +93,14 @@ extern "C" {
>  struct ipv4_hdr {
>  	uint8_t  version_ihl;		/**< version and header length */
>  	uint8_t  type_of_service;	/**< type of service */
> -	uint16_t total_length;		/**< length of packet */
> -	uint16_t packet_id;		/**< packet ID */
> -	uint16_t fragment_offset;	/**< fragmentation offset */
> +	rte_be16_t total_length;	/**< length of packet */
> +	rte_be16_t packet_id;		/**< packet ID */
> +	rte_be16_t fragment_offset;	/**< fragmentation offset */
>  	uint8_t  time_to_live;		/**< time to live */
>  	uint8_t  next_proto_id;		/**< protocol ID */
> -	uint16_t hdr_checksum;		/**< header checksum */
> -	uint32_t src_addr;		/**< source address */
> -	uint32_t dst_addr;		/**< destination address */
> +	rte_be16_t hdr_checksum;	/**< header checksum */
> +	rte_be32_t src_addr;		/**< source address */
> +	rte_be32_t dst_addr;		/**< destination address */
>  } __attribute__((__packed__));
> 
>  /** Create IPv4 address */
> @@ -340,11 +340,11 @@ static inline uint16_t
>  rte_ipv4_phdr_cksum(const struct ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
>  {
>  	struct ipv4_psd_header {
> -		uint32_t src_addr; /* IP address of source host. */
> -		uint32_t dst_addr; /* IP address of destination host. */
> -		uint8_t  zero;     /* zero. */
> -		uint8_t  proto;    /* L4 protocol type. */
> -		uint16_t len;      /* L4 length. */
> +		rte_be32_t src_addr; /* IP address of source host. */
> +		rte_be32_t dst_addr; /* IP address of destination host. */
> +		uint8_t    zero;     /* zero. */
> +		uint8_t    proto;    /* L4 protocol type. */
> +		rte_be16_t len;      /* L4 length. */
>  	} psd_hdr;
> 
>  	psd_hdr.src_addr = ipv4_hdr->src_addr;
> @@ -398,12 +398,12 @@ rte_ipv4_udptcp_cksum(const struct ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>   * IPv6 Header
>   */
>  struct ipv6_hdr {
> -	uint32_t vtc_flow;     /**< IP version, traffic class & flow label. */
> -	uint16_t payload_len;  /**< IP packet length - includes sizeof(ip_header). */
> -	uint8_t  proto;        /**< Protocol, next header. */
> -	uint8_t  hop_limits;   /**< Hop limits. */
> -	uint8_t  src_addr[16]; /**< IP address of source host. */
> -	uint8_t  dst_addr[16]; /**< IP address of destination host(s). */
> +	rte_be32_t vtc_flow;     /**< IP version, traffic class & flow label. */
> +	rte_be16_t payload_len;  /**< IP packet length - includes sizeof(ip_header). */
> +	uint8_t    proto;        /**< Protocol, next header. */
> +	uint8_t    hop_limits;   /**< Hop limits. */
> +	uint8_t    src_addr[16]; /**< IP address of source host. */
> +	uint8_t    dst_addr[16]; /**< IP address of destination host(s). */
>  } __attribute__((__packed__));
> 
>  /**
> @@ -427,8 +427,8 @@ rte_ipv6_phdr_cksum(const struct ipv6_hdr *ipv6_hdr, uint64_t ol_flags)
>  {
>  	uint32_t sum;
>  	struct {
> -		uint32_t len;   /* L4 length. */
> -		uint32_t proto; /* L4 protocol - top 3 bytes must be zero */
> +		rte_be32_t len;   /* L4 length. */
> +		rte_be32_t proto; /* L4 protocol - top 3 bytes must be zero */
>  	} psd_hdr;
> 
>  	psd_hdr.proto = (ipv6_hdr->proto << 24);
> diff --git a/lib/librte_net/rte_net.c b/lib/librte_net/rte_net.c
> index a8c7aff..9014ca5 100644
> --- a/lib/librte_net/rte_net.c
> +++ b/lib/librte_net/rte_net.c
> @@ -153,8 +153,8 @@ ptype_inner_l4(uint8_t proto)
> 
>  /* get the tunnel packet type if any, update proto and off. */
>  static uint32_t
> -ptype_tunnel(uint16_t *proto, const struct rte_mbuf *m,
> -	uint32_t *off)
> +ptype_tunnel(rte_be16_t *proto, const struct rte_mbuf *m,
> +	     uint32_t *off)
>  {
>  	switch (*proto) {
>  	case IPPROTO_GRE: {
> @@ -208,8 +208,8 @@ ip4_hlen(const struct ipv4_hdr *hdr)
> 
>  /* parse ipv6 extended headers, update offset and return next proto */
>  static uint16_t
> -skip_ip6_ext(uint16_t proto, const struct rte_mbuf *m, uint32_t *off,
> -	int *frag)
> +skip_ip6_ext(rte_be16_t proto, const struct rte_mbuf *m, uint32_t *off,
> +	     int *frag)
>  {
>  	struct ext_hdr {
>  		uint8_t next_hdr;
> @@ -261,7 +261,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  	struct ether_hdr eh_copy;
>  	uint32_t pkt_type = RTE_PTYPE_L2_ETHER;
>  	uint32_t off = 0;
> -	uint16_t proto;
> +	rte_be16_t proto;
> 
>  	if (hdr_lens == NULL)
>  		hdr_lens = &local_hdr_lens;
> diff --git a/lib/librte_net/rte_sctp.h b/lib/librte_net/rte_sctp.h
> index 688e126..8c646c7 100644
> --- a/lib/librte_net/rte_sctp.h
> +++ b/lib/librte_net/rte_sctp.h
> @@ -81,15 +81,16 @@ extern "C" {
>  #endif
> 
>  #include <stdint.h>
> +#include <rte_byteorder.h>
> 
>  /**
>   * SCTP Header
>   */
>  struct sctp_hdr {
> -	uint16_t src_port; /**< Source port. */
> -	uint16_t dst_port; /**< Destin port. */
> -	uint32_t tag;      /**< Validation tag. */
> -	uint32_t cksum;    /**< Checksum. */
> +	rte_be16_t src_port; /**< Source port. */
> +	rte_be16_t dst_port; /**< Destin port. */
> +	rte_be32_t tag;      /**< Validation tag. */
> +	rte_le32_t cksum;    /**< Checksum. */
>  } __attribute__((__packed__));
> 
>  #ifdef __cplusplus
> diff --git a/lib/librte_net/rte_tcp.h b/lib/librte_net/rte_tcp.h
> index 28b61e6..545d4ab 100644
> --- a/lib/librte_net/rte_tcp.h
> +++ b/lib/librte_net/rte_tcp.h
> @@ -77,6 +77,7 @@
>   */
> 
>  #include <stdint.h>
> +#include <rte_byteorder.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -86,15 +87,15 @@ extern "C" {
>   * TCP Header
>   */
>  struct tcp_hdr {
> -	uint16_t src_port;  /**< TCP source port. */
> -	uint16_t dst_port;  /**< TCP destination port. */
> -	uint32_t sent_seq;  /**< TX data sequence number. */
> -	uint32_t recv_ack;  /**< RX data acknowledgement sequence number. */
> -	uint8_t  data_off;  /**< Data offset. */
> -	uint8_t  tcp_flags; /**< TCP flags */
> -	uint16_t rx_win;    /**< RX flow control window. */
> -	uint16_t cksum;     /**< TCP checksum. */
> -	uint16_t tcp_urp;   /**< TCP urgent pointer, if any. */
> +	rte_be16_t src_port;  /**< TCP source port. */
> +	rte_be16_t dst_port;  /**< TCP destination port. */
> +	rte_be32_t sent_seq;  /**< TX data sequence number. */
> +	rte_be32_t recv_ack;  /**< RX data acknowledgement sequence number. */
> +	uint8_t    data_off;  /**< Data offset. */
> +	uint8_t    tcp_flags; /**< TCP flags */
> +	rte_be16_t rx_win;    /**< RX flow control window. */
> +	rte_be16_t cksum;     /**< TCP checksum. */
> +	rte_be16_t tcp_urp;   /**< TCP urgent pointer, if any. */
>  } __attribute__((__packed__));
> 
>  #ifdef __cplusplus
> diff --git a/lib/librte_net/rte_udp.h b/lib/librte_net/rte_udp.h
> index bc5be4a..89fdded 100644
> --- a/lib/librte_net/rte_udp.h
> +++ b/lib/librte_net/rte_udp.h
> @@ -77,6 +77,7 @@
>   */
> 
>  #include <stdint.h>
> +#include <rte_byteorder.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -86,10 +87,10 @@ extern "C" {
>   * UDP Header
>   */
>  struct udp_hdr {
> -	uint16_t src_port;    /**< UDP source port. */
> -	uint16_t dst_port;    /**< UDP destination port. */
> -	uint16_t dgram_len;   /**< UDP datagram length */
> -	uint16_t dgram_cksum; /**< UDP datagram checksum */
> +	rte_be16_t src_port;    /**< UDP source port. */
> +	rte_be16_t dst_port;    /**< UDP destination port. */
> +	rte_be16_t dgram_len;   /**< UDP datagram length */
> +	rte_be16_t dgram_cksum; /**< UDP datagram checksum */
>  } __attribute__((__packed__));
> 
>  #ifdef __cplusplus
> --
> 2.1.4
  

On Mon, Dec 05, 2016 at 10:09:05AM +0000, Ananyev, Konstantin wrote:
> Hi Neilo,
> 
> > 
> > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > accordingly.
> > 
> > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > 
> > No ABI change since these are simply typedefs to the original types.
> 
> It seems like quite a lot of changes...
> Could you probably explain what will be the benefit in return?
> Konstantin

Hi Konstantin,

The benefit is to provide documented byte ordering for data types
software is manipulating to determine when network to CPU (or CPU to
network) conversion must be performed.

Regards,
  

Hi Neilo,


Hi Neilo,
> > >
> > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > accordingly.
> > >
> > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > >
> > > No ABI change since these are simply typedefs to the original types.
> >
> > It seems like quite a lot of changes...
> > Could you probably explain what will be the benefit in return?
> > Konstantin
> 
> Hi Konstantin,
> 
> The benefit is to provide documented byte ordering for data types
> software is manipulating to determine when network to CPU (or CPU to
> network) conversion must be performed.

Ok, but is it really worth it?
User can still make a mistake and forget to call ntoh()/hton() at some particular place.
From other side most people do know that network protocols headers are usually in BE format. 
I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
based on these special types or so.
Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
(and might be  in some others too) to be consistent?
Konstantin

> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND
  

On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> Hi Neilo,
> 
> 
> Hi Neilo,
> > > >
> > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > accordingly.
> > > >
> > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > >
> > > > No ABI change since these are simply typedefs to the original types.
> > >
> > > It seems like quite a lot of changes...
> > > Could you probably explain what will be the benefit in return?
> > > Konstantin
> > 
> > Hi Konstantin,
> > 
> > The benefit is to provide documented byte ordering for data types
> > software is manipulating to determine when network to CPU (or CPU to
> > network) conversion must be performed.
> 
> Ok, but is it really worth it?
> User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> From other side most people do know that network protocols headers are usually in BE format. 
> I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> based on these special types or so.
> Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> (and might be  in some others too) to be consistent?
> Konstantin
> 

I actually quite like this patch as I think it will help make things
clear when the user is possibly doing something wrong. I don't think we
need to globally change all PMDs to use the types, though.

One thing I'm wondering though, is if we might want to take this
further. For little endian environments, we could define the big endian
types as structs using typedefs, and similarly the le types on be
platforms, so that assigning from the non-native type to the native one
without a transformation function would cause a compiler error.

/Bruce
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Tuesday, December 6, 2016 11:55 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> 
> On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > Hi Neilo,
> >
> >
> > Hi Neilo,
> > > > >
> > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > accordingly.
> > > > >
> > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > >
> > > > > No ABI change since these are simply typedefs to the original types.
> > > >
> > > > It seems like quite a lot of changes...
> > > > Could you probably explain what will be the benefit in return?
> > > > Konstantin
> > >
> > > Hi Konstantin,
> > >
> > > The benefit is to provide documented byte ordering for data types
> > > software is manipulating to determine when network to CPU (or CPU to
> > > network) conversion must be performed.
> >
> > Ok, but is it really worth it?
> > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > From other side most people do know that network protocols headers are usually in BE format.
> > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > based on these special types or so.
> > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > (and might be  in some others too) to be consistent?
> > Konstantin
> >
> 
> I actually quite like this patch as I think it will help make things
> clear when the user is possibly doing something wrong. I don't think we
> need to globally change all PMDs to use the types, though.

Ok, so where do you believe we should draw a line?
Why let say inside lib/librte_net people should use these typedefs, but
inside drivers/net/ixgbe they don't?
 
> 
> One thing I'm wondering though, is if we might want to take this
> further. For little endian environments, we could define the big endian
> types as structs using typedefs, and similarly the le types on be
> platforms, so that assigning from the non-native type to the native one
> without a transformation function would cause a compiler error.

Not sure I understand you here.
Could you possibly provide some example?

Konstantin
  

Hi Konstantin, Bruce,

On Tue, Dec 06, 2016 at 11:55:02AM +0000, Bruce Richardson wrote:
> On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > Hi Neilo,
> > 
> > 
> > Hi Neilo,
> > > > >
> > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > accordingly.
> > > > >
> > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > >
> > > > > No ABI change since these are simply typedefs to the original types.
> > > >
> > > > It seems like quite a lot of changes...
> > > > Could you probably explain what will be the benefit in return?
> > > > Konstantin
> > > 
> > > Hi Konstantin,
> > > 
> > > The benefit is to provide documented byte ordering for data types
> > > software is manipulating to determine when network to CPU (or CPU to
> > > network) conversion must be performed.
> > 
> > Ok, but is it really worth it?
> > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > From other side most people do know that network protocols headers are usually in BE format. 
> > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > based on these special types or so.
> > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > (and might be  in some others too) to be consistent?
> > Konstantin
> > 
> 
> I actually quite like this patch as I think it will help make things
> clear when the user is possibly doing something wrong. I don't think we
> need to globally change all PMDs to use the types, though.

I agree, at least APIs should use this, PMDs can do as they want.

> One thing I'm wondering though, is if we might want to take this
> further. For little endian environments, we could define the big endian
> types as structs using typedefs, and similarly the le types on be
> platforms, so that assigning from the non-native type to the native one
> without a transformation function would cause a compiler error.
> 
> /Bruce

If I understand you correctly, this will break hton like functions which
expects an uint*_t not a structure.
  

On Tue, Dec 06, 2016 at 02:14:17PM +0100, Nélio Laranjeiro wrote:
> Hi Konstantin, Bruce,
> 
> On Tue, Dec 06, 2016 at 11:55:02AM +0000, Bruce Richardson wrote:
> > On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > > Hi Neilo,
> > > 
> > > 
> > > Hi Neilo,
> > > > > >
> > > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > > accordingly.
> > > > > >
> > > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > > >
> > > > > > No ABI change since these are simply typedefs to the original types.
> > > > >
> > > > > It seems like quite a lot of changes...
> > > > > Could you probably explain what will be the benefit in return?
> > > > > Konstantin
> > > > 
> > > > Hi Konstantin,
> > > > 
> > > > The benefit is to provide documented byte ordering for data types
> > > > software is manipulating to determine when network to CPU (or CPU to
> > > > network) conversion must be performed.
> > > 
> > > Ok, but is it really worth it?
> > > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > > From other side most people do know that network protocols headers are usually in BE format. 
> > > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > > based on these special types or so.
> > > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > > (and might be  in some others too) to be consistent?
> > > Konstantin
> > > 
> > 
> > I actually quite like this patch as I think it will help make things
> > clear when the user is possibly doing something wrong. I don't think we
> > need to globally change all PMDs to use the types, though.
> 
> I agree, at least APIs should use this, PMDs can do as they want.
> 
> > One thing I'm wondering though, is if we might want to take this
> > further. For little endian environments, we could define the big endian
> > types as structs using typedefs, and similarly the le types on be
> > platforms, so that assigning from the non-native type to the native one
> > without a transformation function would cause a compiler error.
> > 
> > /Bruce
> 
> If I understand you correctly, this will break hton like functions which
> expects an uint*_t not a structure.
> 
Yes, it would break the standard ones, which is the downside of doing
this. We could try "fixing" that with a macro, but that too won't always
work. It's a question of whether the additional safety given by having
the compiler flag an error on an invalid assignment, e.g. of a big-endian
value to a native-little endian value, is worth having to change existing
code using htons to use e.g. rte_htons. Given the cost of changing a lot of
existing code, it may just not be worthwhile, but I thought I'd suggest
it anyway as a way of even better guaranteeing endian-ness safety.

/Bruce
  

On Tue, Dec 06, 2016 at 12:41:00PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Tuesday, December 6, 2016 11:55 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> > 
> > On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > > Hi Neilo,
> > >
> > >
> > > Hi Neilo,
> > > > > >
> > > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > > accordingly.
> > > > > >
> > > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > > >
> > > > > > No ABI change since these are simply typedefs to the original types.
> > > > >
> > > > > It seems like quite a lot of changes...
> > > > > Could you probably explain what will be the benefit in return?
> > > > > Konstantin
> > > >
> > > > Hi Konstantin,
> > > >
> > > > The benefit is to provide documented byte ordering for data types
> > > > software is manipulating to determine when network to CPU (or CPU to
> > > > network) conversion must be performed.
> > >
> > > Ok, but is it really worth it?
> > > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > > From other side most people do know that network protocols headers are usually in BE format.
> > > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > > based on these special types or so.
> > > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > > (and might be  in some others too) to be consistent?
> > > Konstantin
> > >
> > 
> > I actually quite like this patch as I think it will help make things
> > clear when the user is possibly doing something wrong. I don't think we
> > need to globally change all PMDs to use the types, though.
> 
> Ok, so where do you believe we should draw a line?
> Why let say inside lib/librte_net people should use these typedefs, but
> inside drivers/net/ixgbe they don't?

Because those are not public APIs. It would be great if driver writers
used the typedefs, but I don't think it should be mandatory.

>  
> > 
> > One thing I'm wondering though, is if we might want to take this
> > further. For little endian environments, we could define the big endian
> > types as structs using typedefs, and similarly the le types on be
> > platforms, so that assigning from the non-native type to the native one
> > without a transformation function would cause a compiler error.
> 
> Not sure I understand you here.
> Could you possibly provide some example?
> 
typedef struct {
	short val;
} rte_be16_t;

That way if you try to assign a value of type rte_be16_t to a uint16_t
variable you'll get a compiler error, unless you use an appropriate
conversion function. In short, it changes things from not just looking
wrong - which is the main purpose of Neilo's patchset - to actually
making it incorrect from the compiler's point of view too.

/Bruce
  

> On Dec 5, 2016, at 6:06 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:

> 

> On Mon, Dec 05, 2016 at 10:09:05AM +0000, Ananyev, Konstantin wrote:

>> Hi Neilo,

>> 

>>> 

>>> This commit introduces new rte_{le,be}{16,32,64}_t types and updates

>>> rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures

>>> accordingly.

>>> 

>>> Specific big/little endian types avoid uncertainty and conversion mistakes.

>>> 

>>> No ABI change since these are simply typedefs to the original types.

>> 

>> It seems like quite a lot of changes...

>> Could you probably explain what will be the benefit in return?

>> Konstantin

> 

> Hi Konstantin,

> 

> The benefit is to provide documented byte ordering for data types

> software is manipulating to determine when network to CPU (or CPU to

> network) conversion must be performed.


Why can we not just document the variables with doxygen as to the BE or LE expected type. Adding a new type is not going to solve the problem of someone using it incorrectly. In the function header doc it should state the expected type.

Adding yet another type is just going to confuse people as some drivers or code may never get changed. Also some drivers are common to many other systems, which means we would have to move the typedefs over to those systems like Linux and FreeBSD for the common sections.

> 

> Regards,

> 

> -- 

> Nélio Laranjeiro

> 6WIND


Regards,
Keith
  

> 
> On Tue, Dec 06, 2016 at 12:41:00PM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Tuesday, December 6, 2016 11:55 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> > >
> > > On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:
> > > > Hi Neilo,
> > > >
> > > >
> > > > Hi Neilo,
> > > > > > >
> > > > > > > This commit introduces new rte_{le,be}{16,32,64}_t types and updates
> > > > > > > rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures
> > > > > > > accordingly.
> > > > > > >
> > > > > > > Specific big/little endian types avoid uncertainty and conversion mistakes.
> > > > > > >
> > > > > > > No ABI change since these are simply typedefs to the original types.
> > > > > >
> > > > > > It seems like quite a lot of changes...
> > > > > > Could you probably explain what will be the benefit in return?
> > > > > > Konstantin
> > > > >
> > > > > Hi Konstantin,
> > > > >
> > > > > The benefit is to provide documented byte ordering for data types
> > > > > software is manipulating to determine when network to CPU (or CPU to
> > > > > network) conversion must be performed.
> > > >
> > > > Ok, but is it really worth it?
> > > > User can still make a mistake and forget to call ntoh()/hton() at some particular place.
> > > > From other side most people do know that network protocols headers are usually in BE format.
> > > > I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis
> > > > based on these special types or so.
> > > > Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs
> > > > (and might be  in some others too) to be consistent?
> > > > Konstantin
> > > >
> > >
> > > I actually quite like this patch as I think it will help make things
> > > clear when the user is possibly doing something wrong. I don't think we
> > > need to globally change all PMDs to use the types, though.
> >
> > Ok, so where do you believe we should draw a line?
> > Why let say inside lib/librte_net people should use these typedefs, but
> > inside drivers/net/ixgbe they don't?
> 
> Because those are not public APIs. It would be great if driver writers
> used the typedefs, but I don't think it should be mandatory.

Ok, so only public API would have to use these typedefs when appropriate, correct?
I still think that the effort to make these changes and keep this rule outweighs the benefit,
but ok if everyone else think it is useful - I wouldn't object too much. 

> 
> >
> > >
> > > One thing I'm wondering though, is if we might want to take this
> > > further. For little endian environments, we could define the big endian
> > > types as structs using typedefs, and similarly the le types on be
> > > platforms, so that assigning from the non-native type to the native one
> > > without a transformation function would cause a compiler error.
> >
> > Not sure I understand you here.
> > Could you possibly provide some example?
> >
> typedef struct {
> 	short val;
> } rte_be16_t;

Hmm, so:
uint32_t x = rte_be_to_cpu_32(1);
would suddenly stop compiling?
That definitely looks like an ABI breakage to me.
Konstantin

> 
> That way if you try to assign a value of type rte_be16_t to a uint16_t
> variable you'll get a compiler error, unless you use an appropriate
> conversion function. In short, it changes things from not just looking
> wrong - which is the main purpose of Neilo's patchset - to actually
> making it incorrect from the compiler's point of view too.
> 
> /Bruce
  

> On Dec 6, 2016, at 8:45 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

> 

> 

> 

>> 

>> On Tue, Dec 06, 2016 at 12:41:00PM +0000, Ananyev, Konstantin wrote:

>>> 

>>> 

>>>> -----Original Message-----

>>>> From: Richardson, Bruce

>>>> Sent: Tuesday, December 6, 2016 11:55 AM

>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>

>>>> Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo

>>>> <wenzhuo.lu@intel.com>; Adrien Mazarguil <adrien.mazarguil@6wind.com>

>>>> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types

>>>> 

>>>> On Tue, Dec 06, 2016 at 11:23:42AM +0000, Ananyev, Konstantin wrote:

>>>>> Hi Neilo,

>>>>> 

>>>>> 

>>>>> Hi Neilo,

>>>>>>>> 

>>>>>>>> This commit introduces new rte_{le,be}{16,32,64}_t types and updates

>>>>>>>> rte_{le,be,cpu}_to_{le,be,cpu}_*() and network header structures

>>>>>>>> accordingly.

>>>>>>>> 

>>>>>>>> Specific big/little endian types avoid uncertainty and conversion mistakes.

>>>>>>>> 

>>>>>>>> No ABI change since these are simply typedefs to the original types.

>>>>>>> 

>>>>>>> It seems like quite a lot of changes...

>>>>>>> Could you probably explain what will be the benefit in return?

>>>>>>> Konstantin

>>>>>> 

>>>>>> Hi Konstantin,

>>>>>> 

>>>>>> The benefit is to provide documented byte ordering for data types

>>>>>> software is manipulating to determine when network to CPU (or CPU to

>>>>>> network) conversion must be performed.

>>>>> 

>>>>> Ok, but is it really worth it?

>>>>> User can still make a mistake and forget to call ntoh()/hton() at some particular place.

>>>>> From other side most people do know that network protocols headers are usually in BE format.

>>>>> I would understand the effort, if we'll have some sort of tool that would do some sort of static code analysis

>>>>> based on these special types or so.

>>>>> Again, does it mean that we should go and change uint32_t to rte_le_32 inside all Intel PMDs

>>>>> (and might be  in some others too) to be consistent?

>>>>> Konstantin

>>>>> 

>>>> 

>>>> I actually quite like this patch as I think it will help make things

>>>> clear when the user is possibly doing something wrong. I don't think we

>>>> need to globally change all PMDs to use the types, though.

>>> 

>>> Ok, so where do you believe we should draw a line?

>>> Why let say inside lib/librte_net people should use these typedefs, but

>>> inside drivers/net/ixgbe they don't?

>> 

>> Because those are not public APIs. It would be great if driver writers

>> used the typedefs, but I don't think it should be mandatory.

> 

> Ok, so only public API would have to use these typedefs when appropriate, correct?

> I still think that the effort to make these changes and keep this rule outweighs the benefit,

> but ok if everyone else think it is useful - I wouldn't object too much. 


I believe the effort and advantages to this change have no real benefit when you can document the type in the function header. Adding a structure around the simple type just adds more typing and still will be difficult to manage even if it gives some compiler checking. The change would not prevent someone putting a BE value into a LE variable, right?

I would not like to see this type of change when documentation would be enough here. Breaking the ABI is a big thing here for a large number of APIs. We keep breaking the ABI and we need to stop doing it on every release of DPDK.

> 

>> 

>>> 

>>>> 

>>>> One thing I'm wondering though, is if we might want to take this

>>>> further. For little endian environments, we could define the big endian

>>>> types as structs using typedefs, and similarly the le types on be

>>>> platforms, so that assigning from the non-native type to the native one

>>>> without a transformation function would cause a compiler error.

>>> 

>>> Not sure I understand you here.

>>> Could you possibly provide some example?

>>> 

>> typedef struct {

>> 	short val;

>> } rte_be16_t;

> 

> Hmm, so:

> uint32_t x = rte_be_to_cpu_32(1);

> would suddenly stop compiling?

> That definitely looks like an ABI breakage to me.

> Konstantin

> 

>> 

>> That way if you try to assign a value of type rte_be16_t to a uint16_t

>> variable you'll get a compiler error, unless you use an appropriate

>> conversion function. In short, it changes things from not just looking

>> wrong - which is the main purpose of Neilo's patchset - to actually

>> making it incorrect from the compiler's point of view too.

>> 

>> /Bruce


Regards,
Keith
  

Hi all,

Being a big fan of strong typing, I really like the concept of explicit endian types. Especially if type mismatches can be caught at compile time.

However, I think it is too late! That train left the station when the rest of the world - including libraries and headers that might be linked with a DPDK application - decided to use implicit big endian types for network protocols, and has been doing so for decades. And, with all respect, I don't think the DPDK community has the momentum required to change this tradition outside the community.

Furthermore: If not enforced throughout DPDK (and beyond), it might confuse more than it helps.


Med venlig hilsen / kind regards
- Morten Brørup
  

Hi all,

On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> Hi all,
> 
> Being a big fan of strong typing, I really like the concept of
> explicit endian types. Especially if type mismatches can be caught at
> compile time.

+1,

> However, I think it is too late! That train left the station when the
> rest of the world - including libraries and headers that might be
> linked with a DPDK application - decided to use implicit big endian
> types for network protocols, and has been doing so for decades. And,
> with all respect, I don't think the DPDK community has the momentum
> required to change this tradition outside the community.

I don't think, those types can be use from now on to help new API to
expose explicitly the type they are handling.  For older ones, it can
come in a second step, even if there are not so numerous.  Only few of
them touches the network types.

> Furthermore: If not enforced throughout DPDK (and beyond), it might
> confuse more than it helps.

The current situation is more confusing,  nobody at any layer can rely
on a precise information, at each function entry we need to verify if
the callee has already handled the job.  The only solution is to browse
the code to have this information.

Think about any function manipulating network headers (like flow director
or rte_flow) from the API down to the PMD, it may take a lot of time to
know at the end if the data is CPU or network ordered, with those types
it takes less than a second.

Regards,
  

> On Dec 6, 2016, at 10:28 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:

> 

> Hi all,

> 

> On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:

>> Hi all,

>> 

>> Being a big fan of strong typing, I really like the concept of

>> explicit endian types. Especially if type mismatches can be caught at

>> compile time.

> 

> +1,

> 

>> However, I think it is too late! That train left the station when the

>> rest of the world - including libraries and headers that might be

>> linked with a DPDK application - decided to use implicit big endian

>> types for network protocols, and has been doing so for decades. And,

>> with all respect, I don't think the DPDK community has the momentum

>> required to change this tradition outside the community.

> 

> I don't think, those types can be use from now on to help new API to

> expose explicitly the type they are handling.  For older ones, it can

> come in a second step, even if there are not so numerous.  Only few of

> them touches the network types.

> 

>> Furthermore: If not enforced throughout DPDK (and beyond), it might

>> confuse more than it helps.

> 

> The current situation is more confusing,  nobody at any layer can rely

> on a precise information, at each function entry we need to verify if

> the callee has already handled the job.  The only solution is to browse

> the code to have this information.

> 

> Think about any function manipulating network headers (like flow director

> or rte_flow) from the API down to the PMD, it may take a lot of time to

> know at the end if the data is CPU or network ordered, with those types

> it takes less than a second.


Still Documentation should handle this problem without code and ABI changes.

> 

> Regards,

> 

> -- 

> Nélio Laranjeiro

> 6WIND


Regards,
Keith
  

> -----Original Message-----

> From: Wiles, Keith

> Sent: Tuesday, December 6, 2016 4:32 PM

> To: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>

> Cc: Morten Brørup <mb@smartsharesystems.com>; Ananyev, Konstantin

> <konstantin.ananyev@intel.com>; Richardson, Bruce

> <bruce.richardson@intel.com>; DPDK <dev@dpdk.org>; Olivier Matz

> <olivier.matz@6wind.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien

> Mazarguil <adrien.mazarguil@6wind.com>

> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types

> 

> 

> > On Dec 6, 2016, at 10:28 AM, Nélio Laranjeiro

> <nelio.laranjeiro@6wind.com> wrote:

> >

> > Hi all,

> >

> > On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:

> >> Hi all,

> >>

> >> Being a big fan of strong typing, I really like the concept of

> >> explicit endian types. Especially if type mismatches can be caught at

> >> compile time.

> >

> > +1,

> >

> >> However, I think it is too late! That train left the station when the

> >> rest of the world - including libraries and headers that might be

> >> linked with a DPDK application - decided to use implicit big endian

> >> types for network protocols, and has been doing so for decades. And,

> >> with all respect, I don't think the DPDK community has the momentum

> >> required to change this tradition outside the community.

> >

> > I don't think, those types can be use from now on to help new API to

> > expose explicitly the type they are handling.  For older ones, it can

> > come in a second step, even if there are not so numerous.  Only few of

> > them touches the network types.

> >

> >> Furthermore: If not enforced throughout DPDK (and beyond), it might

> >> confuse more than it helps.

> >

> > The current situation is more confusing,  nobody at any layer can rely

> > on a precise information, at each function entry we need to verify if

> > the callee has already handled the job.  The only solution is to

> > browse the code to have this information.

> >

> > Think about any function manipulating network headers (like flow

> > director or rte_flow) from the API down to the PMD, it may take a lot

> > of time to know at the end if the data is CPU or network ordered, with

> > those types it takes less than a second.

> 

> Still Documentation should handle this problem without code and ABI

> changes.

> 


While my additional suggestion of compiler-enforced endian correctness may break the ABI (though even that is not certain since parameters would be the same size, just from a compiler syntax analysis side the result would be different, I think), if I'm reading it correctly, Neilo's original suggestion of just using typedefs for big or little endian won't affect the ABI as the underlying types are exactly the same as before. Only the type name has changed to document for the user the endianness the expected data is to be. In effect, the original suggestion is a documentation patch - just the code is the documentation.

Regards,
/Bruce
  

> -----Original Message-----
> From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> Sent: Tuesday, December 6, 2016 4:28 PM
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Wiles, Keith <keith.wiles@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> 
> Hi all,
> 
> On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> > Hi all,
> >
> > Being a big fan of strong typing, I really like the concept of
> > explicit endian types. Especially if type mismatches can be caught at
> > compile time.
> 
> +1,
> 
> > However, I think it is too late! That train left the station when the
> > rest of the world - including libraries and headers that might be
> > linked with a DPDK application - decided to use implicit big endian
> > types for network protocols, and has been doing so for decades. And,
> > with all respect, I don't think the DPDK community has the momentum
> > required to change this tradition outside the community.
> 
> I don't think, those types can be use from now on to help new API to
> expose explicitly the type they are handling.  For older ones, it can
> come in a second step, even if there are not so numerous.  Only few of
> them touches the network types.
> 
> > Furthermore: If not enforced throughout DPDK (and beyond), it might
> > confuse more than it helps.
> 
> The current situation is more confusing,  nobody at any layer can rely
> on a precise information, at each function entry we need to verify if
> the callee has already handled the job.  The only solution is to browse
> the code to have this information.
> 
> Think about any function manipulating network headers (like flow director
> or rte_flow) from the API down to the PMD, it may take a lot of time to
> know at the end if the data is CPU or network ordered, with those types
> it takes less than a second.

Hmm, suppose I have such piece of code:

struct ipv4_hdr iph;
iph.total_length = val0;
iph.packet_id = val1;
...
iph.dst_addr = valn;

hton_whole_ipv4_hdr_at_once(&iph);

How I suppose to indicate via these new types that 
hton_whole_ipv4_hdr_at_once() expects ipv4_hdr fields in host byte order?
Other than just putting it into the doc?
Konstantin
  

On Tue, Dec 06, 2016 at 05:00:05PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > Sent: Tuesday, December 6, 2016 4:28 PM
> > To: Morten Brørup <mb@smartsharesystems.com>
> > Cc: Wiles, Keith <keith.wiles@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; dev@dpdk.org; Olivier Matz <olivier.matz@6wind.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> > Mazarguil <adrien.mazarguil@6wind.com>
> > Subject: Re: [dpdk-dev] [PATCH] net: introduce big and little endian types
> > 
> > Hi all,
> > 
> > On Tue, Dec 06, 2016 at 04:34:07PM +0100, Morten Brørup wrote:
> > > Hi all,
> > >
> > > Being a big fan of strong typing, I really like the concept of
> > > explicit endian types. Especially if type mismatches can be caught at
> > > compile time.
> > 
> > +1,
> > 
> > > However, I think it is too late! That train left the station when the
> > > rest of the world - including libraries and headers that might be
> > > linked with a DPDK application - decided to use implicit big endian
> > > types for network protocols, and has been doing so for decades. And,
> > > with all respect, I don't think the DPDK community has the momentum
> > > required to change this tradition outside the community.
> > 
> > I don't think, those types can be use from now on to help new API to
> > expose explicitly the type they are handling.  For older ones, it can
> > come in a second step, even if there are not so numerous.  Only few of
> > them touches the network types.
> > 
> > > Furthermore: If not enforced throughout DPDK (and beyond), it might
> > > confuse more than it helps.
> > 
> > The current situation is more confusing,  nobody at any layer can rely
> > on a precise information, at each function entry we need to verify if
> > the callee has already handled the job.  The only solution is to browse
> > the code to have this information.
> > 
> > Think about any function manipulating network headers (like flow director
> > or rte_flow) from the API down to the PMD, it may take a lot of time to
> > know at the end if the data is CPU or network ordered, with those types
> > it takes less than a second.
> 
> Hmm, suppose I have such piece of code:
> 
> struct ipv4_hdr iph;
> iph.total_length = val0;
> iph.packet_id = val1;
> ...
> iph.dst_addr = valn;
> 
> hton_whole_ipv4_hdr_at_once(&iph);
> 
> How I suppose to indicate via these new types that 
> hton_whole_ipv4_hdr_at_once() expects ipv4_hdr fields in host byte order?
> Other than just putting it into the doc?
> Konstantin
> 
You aren't.  Your implication is correct in that, if you have a struct that
needs to store data in either cpu endian order or in network endian order, its
best to leave endian tagging out of the type, because it will by definition be
wrong at least 50% of the team, leading to more confusion than it rectifies

That isn't to say that endian coded types don't have a place.  They're often
fairly useful when reading hardware registers and the like, or if a speciic bit
of data is to be extracted from a network payload.  But in the use case above,
no, its not useful.

The best idea it seems is to provide the types, as well as endian-typed and
untyped variants to the endian conversion routines, so that developers can
choose what works best for them.  Since the conversions are all macros with
casts anyway, it should be a no harm no foul addition in my mind.

To be clear, I don't support the patch as it exists, but I would support a
variant that just offered the types and conversion routines.

Neil
 
> 
> 
> 
>
  

Hi all,

Following previous discussions, I would like to gather requirements for
v2, currently we have:

1. Introduction of new typedefs.
2. Modification of network headers.
3. Modification of rte_*_to_*() functions.

Point 1. seems not to be an issue, everyone seems to agree on the fact
having those types could help to document some parts of the code.

Point 2. does not cause any ABI change as it is only a documentation
commit, not sure if anyone disagrees about this.

Point 3. documentation commit most people are uncomfortable with.
I propose to drop it from v2.

Any objection to this plan?
  

> On Dec 8, 2016, at 3:30 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:

> 

> Hi all,

> 

> Following previous discussions, I would like to gather requirements for

> v2, currently we have:

> 

> 1. Introduction of new typedefs.

> 2. Modification of network headers.

> 3. Modification of rte_*_to_*() functions.

> 

> Point 1. seems not to be an issue, everyone seems to agree on the fact

> having those types could help to document some parts of the code.


I never stated these new types were useful in any way, I still believe documentation of the code is the better solution then forcing yet another restriction in submitting patches. 

> 

> Point 2. does not cause any ABI change as it is only a documentation

> commit, not sure if anyone disagrees about this.


I guess no ABI change is done, but I feel it should be as the developer now need to adjust his to reflex these new type even if the compiler does not complain.

> 

> Point 3. documentation commit most people are uncomfortable with.


Not sure what this one is stating, but I whole heartily believe documentation of the code is the best way forward.

The main reasons are:
 - We do not need to add yet another type to DPDK to make the patch process even more restrictive.
 - The new types do not add any type of checking for the compiler and the developer can still get it wrong.
- If any common code used in other platform (say Linux kernel driver) we have to include these new types in that environment.
 - Documentation is the best solution IMO to resolve these types of issues and it does not require any new types or code changes in DPDK or developers code.

Sorry, I strongly disagree with this patch in any form expect documentation changes.

> I propose to drop it from v2.

> 

> Any objection to this plan?

> 

> -- 

> Nélio Laranjeiro

> 6WIND


Regards,
Keith
  

On Thu, Dec 08, 2016 at 10:30:05AM +0100, Nélio Laranjeiro wrote:
> Hi all,
> 
> Following previous discussions, I would like to gather requirements for
> v2, currently we have:
> 
> 1. Introduction of new typedefs.
> 2. Modification of network headers.
> 3. Modification of rte_*_to_*() functions.
> 
> Point 1. seems not to be an issue, everyone seems to agree on the fact
> having those types could help to document some parts of the code.
> 
No objection here

> Point 2. does not cause any ABI change as it is only a documentation
> commit, not sure if anyone disagrees about this.
> 
I have an objection here, and I think it was stated by others previously.  While
its fine to offer endian encoded types so that developers can use them
expediently, I don't like the idea of coding them into network headers
specifically.  I assert that because network headers represent multiple views of
network data (both network byte order if the data is taken off the wire and cpu
byte order if its translated.  To implement such a network header change
efficiently what you would need is something like the following:

struct rte_ip_network_hdr {
	rte_le_u32 dst;
	rte_le_u32 src;
	...
}; 

struct rte_ip_cpu_hdr {
	rte_cpu_u32 dst;
	rte_cpu_u32 src;
	...
};

where rte_cpu_* is defined to a big endian or little endian type based on the
cpu being targeted.  

Then of course you need to define translation macros to do all the appropriate
conversions convieniently (or you need to do specific translations on the
network byte order as needed, which may lead to lots of repeated conversions).
Regardless, this seems to be unscalable. Endian types are the sort of thing that
you should only use sparingly, not by default.

> Point 3. documentation commit most people are uncomfortable with.
> I propose to drop it from v2.
> 
> Any objection to this plan?
> 
> -- 
> Nélio Laranjeiro
> 6WIND
>
  

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Thursday, December 8, 2016 3:07 PM
> To: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; Wiles,
> Keith <keith.wiles@intel.com>; Morten Brørup <mb@smartsharesystems.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>
> Subject: Re: [PATCH] net: introduce big and little endian types
> 
> On Thu, Dec 08, 2016 at 10:30:05AM +0100, Nélio Laranjeiro wrote:
> > Hi all,
> >
> > Following previous discussions, I would like to gather requirements for
> > v2, currently we have:
> >
> > 1. Introduction of new typedefs.
> > 2. Modification of network headers.
> > 3. Modification of rte_*_to_*() functions.
> >
> > Point 1. seems not to be an issue, everyone seems to agree on the fact
> > having those types could help to document some parts of the code.
> >
> No objection here
> 
> > Point 2. does not cause any ABI change as it is only a documentation
> > commit, not sure if anyone disagrees about this.
> >
> I have an objection here, and I think it was stated by others previously.  While
> its fine to offer endian encoded types so that developers can use them
> expediently, I don't like the idea of coding them into network headers
> specifically.  I assert that because network headers represent multiple views of
> network data (both network byte order if the data is taken off the wire and cpu
> byte order if its translated.  To implement such a network header change
> efficiently what you would need is something like the following:
> 
> struct rte_ip_network_hdr {
> 	rte_le_u32 dst;
> 	rte_le_u32 src;
> 	...
> };
> 
> struct rte_ip_cpu_hdr {
> 	rte_cpu_u32 dst;
> 	rte_cpu_u32 src;
> 	...
> };
> 
> where rte_cpu_* is defined to a big endian or little endian type based on the
> cpu being targeted.
> 
> Then of course you need to define translation macros to do all the appropriate
> conversions convieniently (or you need to do specific translations on the
> network byte order as needed, which may lead to lots of repeated conversions).
> Regardless, this seems to be unscalable. Endian types are the sort of thing that
> you should only use sparingly, not by default.

+1

> 
> > Point 3. documentation commit most people are uncomfortable with.
> > I propose to drop it from v2.
> >
> > Any objection to this plan?
> >
> > --
> > Nélio Laranjeiro
> > 6WIND
> >
  

2016-12-08 13:59, Wiles, Keith:
> 
> > On Dec 8, 2016, at 3:30 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> > 
> > Hi all,
> > 
> > Following previous discussions, I would like to gather requirements for
> > v2, currently we have:
> > 
> > 1. Introduction of new typedefs.
> > 2. Modification of network headers.
> > 3. Modification of rte_*_to_*() functions.
> > 
> > Point 1. seems not to be an issue, everyone seems to agree on the fact
> > having those types could help to document some parts of the code.
> 
> I never stated these new types were useful in any way, I still believe documentation of the code is the better solution then forcing yet another restriction in submitting patches. 

It would not be a restriction, just a help for those wanting to document
some tricky parts by using these types.

I see 2 usages:

- in a struct:

rte_be32_t speed; /**< 0 for speed negotiation */
instead of
uint32_t speed; /**< [big endian] 0 for speed negotiation */

- in a function:

rte_be32_t decode_speed (void *);
[...]
speed = rte_be_to_cpu_32(decode_speed());

It is difficult to reject something which could help a bit.
Do you really think it would bring some confusion to have some code
using these endianed-types?