Re: [dpdk-dev] [PATCH v2 6/6] mbuf: flatten struct vlan_macip into mbuf struct

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Bruce,

Just small typos below.

On 08/28/2014 05:42 PM, Bruce Richardson wrote:
> The vlan_macip structure combined a vlan tag id with l2 and l3 headers
> lengths for tracking offloads. However, this structure was only used as
> a unit by the e1000 and ixgbe drivers, not generally.
> 
> This patch removes the structure from the mbuf header and places the
> fields into the mbuf structure directly at the required point, without
> any net effect on the structure layout. This allows us to treat the vlan
> tags and header length fields as separate for future mbuf changes. The
> drivers which were written to use the combined structure still do so,
> using a driver-local definition of it.
> 
> Changes in V2:
> * None
> 
> [...]
> 
> diff --git a/lib/librte_pmd_e1000/em_rxtx.c b/lib/librte_pmd_e1000/em_rxtx.c
> index 074c9b3..4f46bdf 100644
> --- a/lib/librte_pmd_e1000/em_rxtx.c
> +++ b/lib/librte_pmd_e1000/em_rxtx.c
> @@ -144,13 +144,34 @@ enum {
>  	EM_CTX_NUM  = 1, /**< CTX NUM */
>  };
>  
> +/** Offload features */
> +union em_vlan_macip {
> +	uint32_t data;
> +	struct {
> +		uint16_t l3_len:9; /**< L3 (IP) Header Length. */
> +		uint16_t l2_len:7; /**< L2 (MAC) Header Length. */
> +		uint16_t vlan_tci;
> +		/**< VLAN Tag Control Identifier (CPU order). */
> +	} f;
> +};
> +
> +/*
> + * Compare mask for vlan_macip_len.data,
> + * should be in sync with em_vlan_macip.f layout.
> + * */
> +#define TX_VLAN_CMP_MASK        0xFFFF0000  /**< VLAN length - 16-bits. */
> +#define TX_MAC_LEN_CMP_MASK     0x0000FE00  /**< MAC length - 7-bits. */
> +#define TX_IP_LEN_CMP_MASK      0x000001FF  /**< IP  length - 9-bits. */
> +/**< MAC+IP  length. */
> +#define TX_MACIP_LEN_CMP_MASK   (TX_MAC_LEN_CMP_MASK | TX_IP_LEN_CMP_MASK)
> +

I think the doxygen syntax "/**<" is only used for comments that are
after the name. It was already like this before your patch (in
rte_mbuf.h), but maybe it's a good occasion to fix this typo.
There is the same in igb and ixgbe.

>  /**
>   * Structure to check if new context need be built
>   */
>  struct em_ctx_info {
>  	uint16_t flags;               /**< ol_flags related to context build. */
>  	uint32_t cmp_mask;            /**< compare mask */
> -	union rte_vlan_macip hdrlen;  /**< L2 and L3 header lenghts */
> +	union em_vlan_macip hdrlen;  /**< L2 and L3 header lenghts */
>  };

The comment is not aligned with the others.

Acked-by: Olivier Matz <olivier.matz@6wind.com>