Re: [dpdk-dev] [PATCH v3 1/7] lib/librte_ether: modify the structures for fdir new modes

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Oct 22, 2015 at 03:11:36PM +0800, Wenzhuo Lu wrote:
> Define the new modes and modify the filter and mask structures for
> the mac vlan and tunnel modes.
> 
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Hi Wenzhuo,

couple of stylistic comments below, which would help with patch review, especially
with regards to checking for ABI issues.

/Bruce

> ---
>  lib/librte_ether/rte_eth_ctrl.h | 69 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_eth_ctrl.h b/lib/librte_ether/rte_eth_ctrl.h
> index 26b7b33..078faf9 100644
> --- a/lib/librte_ether/rte_eth_ctrl.h
> +++ b/lib/librte_ether/rte_eth_ctrl.h
> @@ -248,6 +248,17 @@ enum rte_eth_tunnel_type {
>  };
>  
>  /**
> + *  Flow Director setting modes: none, signature or perfect.
> + */
> +enum rte_fdir_mode {
> +	RTE_FDIR_MODE_NONE  = 0, /**< Disable FDIR support. */
> +	RTE_FDIR_MODE_SIGNATURE, /**< Enable FDIR signature filter mode. */
> +	RTE_FDIR_MODE_PERFECT,   /**< Enable FDIR perfect filter mode for IP. */
> +	RTE_FDIR_MODE_PERFECT_MAC_VLAN, /**< Enable FDIR filter mode - MAC VLAN. */
> +	RTE_FDIR_MODE_PERFECT_TUNNEL,   /**< Enable FDIR filter mode - tunnel. */
> +};
> +

Why is this structure definition moved in the file, it makes seeing the
diff vs the old version difficult.

> +/**
>   * filter type of tunneling packet
>   */
>  #define ETH_TUNNEL_FILTER_OMAC  0x01 /**< filter by outer MAC addr */
> @@ -377,18 +388,46 @@ struct rte_eth_sctpv6_flow {
>  };
>  
>  /**
> + * A structure used to define the input for MAC VLAN flow
> + */
> +struct rte_eth_mac_vlan_flow {
> +	struct ether_addr mac_addr;  /**< Mac address to match. */
> +};
> +
> +/**
> + * Tunnel type for flow director.
> + */
> +enum rte_eth_fdir_tunnel_type {
> +	RTE_FDIR_TUNNEL_TYPE_NVGRE = 0,
> +	RTE_FDIR_TUNNEL_TYPE_VXLAN,
> +	RTE_FDIR_TUNNEL_TYPE_UNKNOWN,
> +};
> +
> +/**
> + * A structure used to define the input for tunnel flow, now it's VxLAN or
> + * NVGRE
> + */
> +struct rte_eth_tunnel_flow {
> +	enum rte_eth_fdir_tunnel_type tunnel_type; /**< Tunnel type to match. */
> +	uint32_t tunnel_id;                        /**< Tunnel ID to match. TNI, VNI... */
> +	struct ether_addr mac_addr;                /**< Mac address to match. */
> +};
> +
> +/**
>   * An union contains the inputs for all types of flow
>   */
>  union rte_eth_fdir_flow {
> -	struct rte_eth_l2_flow     l2_flow;
> -	struct rte_eth_udpv4_flow  udp4_flow;
> -	struct rte_eth_tcpv4_flow  tcp4_flow;
> -	struct rte_eth_sctpv4_flow sctp4_flow;
> -	struct rte_eth_ipv4_flow   ip4_flow;
> -	struct rte_eth_udpv6_flow  udp6_flow;
> -	struct rte_eth_tcpv6_flow  tcp6_flow;
> -	struct rte_eth_sctpv6_flow sctp6_flow;
> -	struct rte_eth_ipv6_flow   ipv6_flow;
> +	struct rte_eth_l2_flow         l2_flow;
> +	struct rte_eth_udpv4_flow      udp4_flow;
> +	struct rte_eth_tcpv4_flow      tcp4_flow;
> +	struct rte_eth_sctpv4_flow     sctp4_flow;
> +	struct rte_eth_ipv4_flow       ip4_flow;
> +	struct rte_eth_udpv6_flow      udp6_flow;
> +	struct rte_eth_tcpv6_flow      tcp6_flow;
> +	struct rte_eth_sctpv6_flow     sctp6_flow;
> +	struct rte_eth_ipv6_flow       ipv6_flow;
> +	struct rte_eth_mac_vlan_flow   mac_vlan_flow;
> +	struct rte_eth_tunnel_flow     tunnel_flow;

Can you please minimize the whitespace changes here. It looks in the diff
like you are replacing the entire set of entries, but on closer inspection
it looks like you are just adding in two extra lines.

>  };
>  
>  /**
> @@ -465,6 +504,9 @@ struct rte_eth_fdir_masks {
>  	struct rte_eth_ipv6_flow   ipv6_mask;
>  	uint16_t src_port_mask;
>  	uint16_t dst_port_mask;
> +	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
> +	uint32_t tunnel_id_mask;  /** tunnel ID mask */
> +	uint8_t tunnel_type_mask;
>  };
>  
>  /**
> @@ -515,15 +557,6 @@ struct rte_eth_fdir_flex_conf {
>  	/**< Flex mask configuration for each flow type */
>  };
>  
> -/**
> - *  Flow Director setting modes: none, signature or perfect.
> - */
> -enum rte_fdir_mode {
> -	RTE_FDIR_MODE_NONE      = 0, /**< Disable FDIR support. */
> -	RTE_FDIR_MODE_SIGNATURE,     /**< Enable FDIR signature filter mode. */
> -	RTE_FDIR_MODE_PERFECT,       /**< Enable FDIR perfect filter mode. */
> -};
> -
>  #define UINT32_BIT (CHAR_BIT * sizeof(uint32_t))
>  #define RTE_FLOW_MASK_ARRAY_SIZE \
>  	(RTE_ALIGN(RTE_ETH_FLOW_MAX, UINT32_BIT)/UINT32_BIT)
> -- 
> 1.9.3
>